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

> On 19 Jan 2023, at 17:11, Alan Somers <asomers@freebsd.org> wrote:
> 
> 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.
I think another question is that if if should be done by ifconfig(8) at all. Kernel can take care of trying to load the required modules, checking the privileges.
I’m considering adding such code for the netlink-based interface creation.
>