Date: Fri, 20 Jan 2023 08:24:26 -0500 From: Shawn Webb <shawn.webb@hardenedbsd.org> To: Alexander Chernikov <melifaro@freebsd.org> Cc: Alan Somers <asomers@freebsd.org>, "Danilo G. Baio" <dbaio@freebsd.org>, dev-commits-src-all@freebsd.org Subject: Re: git: 2c24ad3377a6 - main - ifconfig: abort if loading a module fails other than for ENOENT Message-ID: <20230120132426.isyeq3bqpaeoymdb@mutt-hbsd> In-Reply-To: <1E9FAE83-B5C2-4E1F-8D04-CF4F477F76C7@freebsd.org> References: <202301091857.309Iv87L068285@gitrepo.freebsd.org> <2f4e4ccf-b19a-4f8f-a9e0-72298e500d7c@app.fastmail.com> <CAOtMX2hv182P2HTAPkbYDZiwNxkV2-C%2BWp2%2BL0SpfDpqn2Zccw@mail.gmail.com> <1E9FAE83-B5C2-4E1F-8D04-CF4F477F76C7@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--yjlfnrth6j5yzmaq Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 20, 2023 at 11:04:33AM +0000, Alexander Chernikov wrote: >=20 >=20 > > On 19 Jan 2023, at 17:11, Alan Somers <asomers@freebsd.org> wrote: > >=20 > > On Thu, Jan 19, 2023 at 7:03 AM Danilo G. Baio <dbaio@freebsd.org> wrot= e: > >>=20 > >>=20 > >>=20 > >> On Mon, Jan 9, 2023, at 15:57, Alan Somers wrote: > >>> The branch main has been updated by asomers: > >>>=20 > >>> URL: > >>> https://cgit.FreeBSD.org/src/commit/?id=3D2c24ad3377a6f584e484656db83= 90e4eb7cfc119 > >>>=20 > >>> 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 > >>>=20 > >>> ifconfig: abort if loading a module fails other than for ENOENT > >>>=20 > >>> 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 clo= ned > >>> interface in a vnet jail. But ignore ENOENT, because sometimes ifc= onfig > >>> can't correctly guess the name of the required kernel module. > >>>=20 > >>> 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(-) > >>>=20 > >>> 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) > >>> } > >>> } > >>>=20 > >>> - /* > >>> - * Try to load the module. But ignore failures, because ifconf= ig 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); > >>> + } > >>> + } > >>> } > >>>=20 > >>> static struct cmd basic_cmds[] =3D { > >>=20 > >>=20 > >> Hi. > >>=20 > >> I have a jail with vnet where the interface is renamed that stopped wo= rking after this. > >>=20 > >> from inside the jail: > >>=20 > >> root@test:/ # ifconfig > >> lo0: flags=3D8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384 > >> options=3D680003<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=3D21<PERFORMNUD,AUTO_LINKLOCAL> > >> vnet0b_test: flags=3D8862<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric = 0 mtu 1500 > >> options=3D8<VLAN_MTU> > >> ether 02:27:72:a7:28:0b > >> groups: epair > >> media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>) > >> status: active > >> nd6 options=3D29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL> > >>=20 > >> root@test:/ # ifconfig vnet0b_test > >> ifconfig: kldload(if_vnet): Operation not permitted > >>=20 > >>=20 > >> If I don't rename the interface, that works. > >>=20 > >> jail.conf: > >>=20 > >> test { > >> vnet; > >> $index =3D "0"; > >> vnet.interface =3D "vnet${index}b_${name}"; > >> exec.prestart +=3D "ifconfig epair${index} create"; > >> exec.prestart +=3D "ifconfig ${bridge} addm epair${index}a"; > >> exec.prestart +=3D "ifconfig epair${index}a up name vnet${index}a_${= name}"; > >> exec.prestart +=3D "ifconfig epair${index}b up name vnet${index}b_${= name}"; > >> exec.poststop +=3D "ifconfig ${bridge} deletem vnet${index}a_${name}= "; > >> exec.poststop +=3D "ifconfig vnet${index}a_${name} destroy"; > >> devfs_ruleset =3D "11"; # add path 'bpf*' unhide (devfs.rules) > >> } > >>=20 > >> That's a bit odd, I know, could be using description instead. > >>=20 > >> Just reporting. > >>=20 > >> Regards. > >> -- > >> Danilo G. Baio > >=20 > > 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 b= e 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 a= ll. Kernel can take care of trying to load the required modules, checking t= he privileges. > I=E2=80=99m considering adding such code for the netlink-based interface = creation. An interesting problem unique to HardenedBSD is that since the kld* syscalls are hardened such that unprivileged users cannot use them at all (so kldfind(2)/kldstat(8) are completely nonfunctional), this breaks even read-only operations with ifconfig when specifying the interface. Meaning, `ifconfig` works, but `ifconfig em0` does not, when run as an unprivileged user. I'm of the opinion that read-only operations (like `ifconfig em0`) should be read-only in every sense. Kernel state should be preserved unmodified. The change I made in HardenedBSD is rather simple: force -n to be enabled by default for all cases. Though, I don't think that's likely the right solution for FreeBSD. It seems natural that FreeBSD would want to take a more permissive route. https://git.hardenedbsd.org/hardenedbsd/HardenedBSD/-/commit/671eb92efc2c9e= ef485194e443f7fa8102b2fe97 Thanks, --=20 Shawn Webb Cofounder / Security Engineer HardenedBSD https://git.hardenedbsd.org/hardenedbsd/pubkeys/-/raw/master/Shawn_Webb/03A= 4CBEBB82EA5A67D9F3853FF2E67A277F8E1FA.pub.asc --yjlfnrth6j5yzmaq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEA6TL67gupaZ9nzhT/y5nonf44foFAmPKlgIACgkQ/y5nonf4 4fouWA/9GUglzAeiEWtseHOgPSehp70J66FxrfVGeJ0dW9fhQ2akm8jaVsOyzNzg ZyqgJRbul6vzmy+lkkrfssCb+Hyhcz5blzZq07eG7s+ovp7wMUlpPobDa7Bh3u00 V6vaZiZz2RRhUMM0jAUQKsp86KocAOseQ37ijkOk7eX6JZJM/eRaF15cH03KkssS 03jdO2d8EiJG94ximcD0XcwIr7XWcOzbOwbUpjLZBPAnDpd7Z+U+catf9uFtWbkD oPkXZj7bfIb0TFYhbX34Bymh+3gQPzA1FdUbWthz/+tK44yclV86UavmS57a2POh /WOM322QuWjiKUWoCLZTNFGP26g+8NmpmQk+fqnC8GgNGhdq/1j7vkpwWneQQ7Ic vlAYQ+WrAikPLtZ5PM+7f3RLpNTfLqlxuTmlGGRzHy6BbyPwu/VlQN6Y1S+uJLtL ZSFROLIMyWFXSofNyApkZUapCUQTv8EwkBQlIl8G9lLAGjvaEe4WBxROQIK0eCeR XLPAaOi4MVJkKlS0CUTVANpaAAJb02Ip+9gYp+FEtEO5bMbvJEato2elQq1G0Agh AKfdmX858TM7aXckBwGS54LurYBGBAcxhNeNdvNxBjjgIU14N4QMwBylklzUQqYz OLcAE9vJeLVdIp/tC0zSwg+bJFTHKlRZ5GIxN5TCL4bJMXziCUs= =lSUQ -----END PGP SIGNATURE----- --yjlfnrth6j5yzmaq--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20230120132426.isyeq3bqpaeoymdb>