Re: git: 2c24ad3377a6 - main - ifconfig: abort if loading a module fails other than for ENOENT

On Thu, Jan 19, 2023 at 7:03 AM Danilo G. Baio <dbaio@freebsd.org> wrote:
>
>
>
> On Mon, Jan 9, 2023, at 15:57, Alan Somers wrote:
> > The branch main has been updated by asomers:
> >
> > URL:
> > https://cgit.FreeBSD.org/src/commit/?id=2c24ad3377a6f584e484656db8390e4eb7cfc119
> >
> > commit 2c24ad3377a6f584e484656db8390e4eb7cfc119
> > Author:     Alan Somers <asomers@FreeBSD.org>
> > AuthorDate: 2022-12-26 02:06:21 +0000
> > Commit:     Alan Somers <asomers@FreeBSD.org>
> > CommitDate: 2023-01-10 02:56:18 +0000
> >
> >     ifconfig: abort if loading a module fails other than for ENOENT
> >
> >     If "ifconfig create" tries to load a kernel module, and the module
> >     exists but can't be loaded, fail the command with a useful error
> >     message.  This is helpful, for example, when trying to create a cloned
> >     interface in a vnet jail.  But ignore ENOENT, because sometimes ifconfig
> >     can't correctly guess the name of the required kernel module.
> >
> >     MFC after:      2 weeks
> >     Reviewed by:    jhb
> >     Differential Revision: https://reviews.freebsd.org/D37873
> > ---
> >  sbin/ifconfig/ifconfig.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/sbin/ifconfig/ifconfig.c b/sbin/ifconfig/ifconfig.c
> > index 462d543125c4..120207a6927e 100644
> > --- a/sbin/ifconfig/ifconfig.c
> > +++ b/sbin/ifconfig/ifconfig.c
> > @@ -1719,11 +1719,19 @@ ifmaybeload(const char *name)
> >               }
> >       }
> >
> > -     /*
> > -      * Try to load the module.  But ignore failures, because ifconfig can't
> > -      * infer the names of all drivers (eg mlx4en(4)).
> > -      */
> > -     (void) kldload(ifkind);
> > +     /* Try to load the module. */
> > +     if (kldload(ifkind) < 0) {
> > +             switch (errno) {
> > +             case ENOENT:
> > +                     /*
> > +                      * Ignore ENOENT, because ifconfig can't infer the
> > +                      * names of all drivers (eg mlx4en(4)).
> > +                      */
> > +                     break;
> > +             default:
> > +                     err(1, "kldload(%s)", ifkind);
> > +             }
> > +     }
> >  }
> >
> >  static struct cmd basic_cmds[] = {
>
>
> Hi.
>
> I have a jail with vnet where the interface is renamed that stopped working after this.
>
> from inside the jail:
>
> root@test:/ # ifconfig
> lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384
>         options=680003<RXCSUM,TXCSUM,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
>         inet6 ::1 prefixlen 128
>         inet6 fe80::1%lo0 prefixlen 64 scopeid 0x10
>         inet 127.0.0.1 netmask 0xff000000
>         groups: lo
>         nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>
> vnet0b_test: flags=8862<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
>         options=8<VLAN_MTU>
>         ether 02:27:72:a7:28:0b
>         groups: epair
>         media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>)
>         status: active
>         nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
>
> root@test:/ # ifconfig vnet0b_test
> ifconfig: kldload(if_vnet): Operation not permitted
>
>
> If I don't rename the interface, that works.
>
> jail.conf:
>
> test {
>     vnet;
>     $index = "0";
>     vnet.interface = "vnet${index}b_${name}";
>     exec.prestart += "ifconfig epair${index} create";
>     exec.prestart += "ifconfig ${bridge} addm epair${index}a";
>     exec.prestart += "ifconfig epair${index}a up name vnet${index}a_${name}";
>     exec.prestart += "ifconfig epair${index}b up name vnet${index}b_${name}";
>     exec.poststop += "ifconfig ${bridge} deletem vnet${index}a_${name}";
>     exec.poststop += "ifconfig vnet${index}a_${name} destroy";
>     devfs_ruleset = "11"; # add path 'bpf*' unhide (devfs.rules)
> }
>
> That's a bit odd, I know, could be using description instead.
>
> Just reporting.
>
> Regards.
> --
> Danilo G. Baio

Ugh, it looks like kldload(2) is doing the privilege check before the
file existence check.  I'm not sure of the best solution:
* Change kern_kldload to check for file existence first.  This would
ring some alarm bells among security folks, and it isn't totally easy
to do, either.
* Change ifconfig(8) to do an existence check of its own.  This would be ugly.
* Change ifconfig(8) so that it doesn't attempt to load modules when
just listing an interface.  This might be incomplete, but is probably
worth doing anyway.