lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y4S7XENL7TgIEtPA@nanopsycho> Date: Mon, 28 Nov 2022 14:52:00 +0100 From: Jiri Pirko <jiri@...nulli.us> To: Leon Romanovsky <leon@...nel.org> Cc: Ido Schimmel <idosch@...sch.org>, Jakub Kicinski <kuba@...nel.org>, Yang Yingliang <yangyingliang@...wei.com>, netdev@...r.kernel.org, jiri@...dia.com, davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com Subject: Re: [PATCH net] net: devlink: fix UAF in devlink_compat_running_version() Mon, Nov 28, 2022 at 12:50:15PM CET, leon@...nel.org wrote: >On Mon, Nov 28, 2022 at 10:58:58AM +0100, Jiri Pirko wrote: >> Mon, Nov 28, 2022 at 10:20:53AM CET, idosch@...sch.org wrote: >> >On Wed, Nov 23, 2022 at 06:18:00PM -0800, Jakub Kicinski wrote: >> >> On Wed, 23 Nov 2022 21:18:14 +0200 Ido Schimmel wrote: >> >> > > I used the fix code proposed by Jakub, but it didn't work correctly, so >> >> > > I tried to correct and improve it, and need some devlink helper. >> >> > > >> >> > > Anyway, it is a nsim problem, if we want fix this without touch devlink, >> >> > > I think we can add a 'registered' field in struct nsim_dev, and it can be >> >> > > checked in nsim_get_devlink_port() like this: >> >> > >> >> > I read the discussion and it's not clear to me why this is a netdevsim >> >> > specific problem. The fundamental problem seems to be that it is >> >> > possible to hold a reference on a devlink instance before it's >> >> > registered and that devlink_free() will free the instance regardless of >> >> > its current reference count because it expects devlink_unregister() to >> >> > block. In this case, the instance was never registered, so >> >> > devlink_unregister() is not called. >> >> > >> >> > ethtool was able to get a reference on the devlink instance before it >> >> > was registered because netdevsim registers its netdevs before >> >> > registering its devlink instance. However, netdevsim is not the only one >> >> > doing this: funeth, ice, prestera, mlx4, mlxsw, nfp and potentially >> >> > others do the same thing. >> >> > >> >> > When you think about it, it's strange that it's even possible for >> >> > ethtool to reach the driver when the netdev used in the request is long >> >> > gone, but it's not holding a reference on the netdev (it's holding a >> >> > reference on the devlink instance instead) and >> >> > devlink_compat_running_version() is called without RTNL. >> >> >> >> Indeed. We did a bit of a flip-flop with the devlink locking rules >> >> and the fact that the instance is reachable before it is registered >> >> is a leftover from a previous restructuring :( >> >> >> >> Hence my preference to get rid of the ordering at the driver level >> >> than to try to patch it up in the code. Dunno if that's convincing. >> > >> >I don't have a good solution, but changing all the drivers to register >> >their netdevs after the devlink instance is going to be quite painful >> >and too big for 'net'. I feel like the main motivation for this is the >> >ethtool compat stuff, which is not very convincing IMO. I'm quite happy >> >with the current flow where drivers call devlink_register() at the end >> >of their probe. >> > >> >Regarding a solution for the current crash, assuming we agree it's not a >> >netdevsim specific problem, I think the current fix [1] is OK. Note that >> >while it fixes the crash, it potentially creates other (less severe) >> >problems. After user space receives RTM_NEWLINK notification it will >> >need to wait for a certain period of time before issuing >> >'ETHTOOL_GDRVINFO' as otherwise it will not get the firmware version. I >> >guess it's not a big deal for drivers that only register one netdev >> >since they will very quickly follow with devlink_register(), but the >> >race window is larger for drivers that need to register many netdevs, >> >for either physical switch or eswitch ports. >> > >> >Long term, we either need to find a way to make the ethtool compat stuff >> >work correctly or just get rid of it and have affected drivers implement >> >the relevant ethtool operations instead of relying on devlink. >> > >> >[1] https://lore.kernel.org/netdev/20221122121048.776643-1-yangyingliang@huawei.com/ >> >> I just had a call with Ido. We both think that this might be a good >> solution for -net to avoid the use after free. >> >> For net-next, we eventually should change driver init flows to register >> devlink instance first and only after that register devlink_port and >> related netdevice. The ordering is important for the userspace app. For >> example the init flow: >> <- RTnetlink new netdev event >> app sees devlink_port handle in IFLA_DEVLINK_PORT >> -> query devlink instance using this handle >> <- ENODEV >> >> The instance is not registered yet. > >This is supposed to be handled by devlink_notify_register() which sends >"delayed" notifications after devlink_register() is called. > >Unless something is broken, the scenario above shouldn't happen. Nope, RTnetlink message for new netdev is not handled by that. It is sent right away. > >> >> So we need to make sure all devlink_port_register() calls are happening >> after devlink_register(). This is aligned with the original flow before >> devlink_register() was moved by Leon. Also it is aligned with devlink >> reload and devlink port split flows. >> > >I don't know what it means. What I mean is that during port split, devlink instance is registered. During port creation and removal during reload, devlink instance is registered. We should maintain the same ordering during init/fini I believe. > >Thanks
Powered by blists - more mailing lists