Date:   Tue, 29 Nov 2022 10:44:48 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
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()

On Mon, Nov 28, 2022 at 02:52:00PM +0100, Jiri Pirko wrote:
> 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.

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?

Thanks

Powered by blists - more mailing lists