Re: [PATCH net] net: devlink: fix UAF in devlink_compat_running_version()

[Jiri Pirko <jiri@resnulli.us>]

Tue, Nov 29, 2022 at 09:44:48AM CET, leon@kernel.org wrote:
>On Mon, Nov 28, 2022 at 02:52:00PM +0100, Jiri Pirko wrote:
>> Mon, Nov 28, 2022 at 12:50:15PM CET, leon@kernel.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@idosch.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.
>
>And why don't you fix your new commit dca56c3038c3 ("net: expose devlink port over rtnetlink")
>to do not return devlink instance unless it is registered?
>
>Why is it correct to expose devlink port with not ready to use devlink
>instance?

It is not, but:
Devlink port which is "parent" of the netdev is registered. The netdev
is created with devlink_port registered and that it guaranteed to not
change during netdev lifetime. Therefore, it would be weird to have 2
RTnetlink events:
1. event of netdev being created without devlink port
2. event of netdev with devlink port
If that is what you suggest.

I'm working on a patchset that is making sure that the flow is always
1) devlink_register & netlink event
2) devlink_port_register & netlink event
3) netdev_register & netlink event

Always the same. That means during init, during reload, during port
split.