From: Jiri Pirko <jiri@resnulli.us>
To: Leon Romanovsky <leon@kernel.org>
Cc: Ido Schimmel <idosch@idosch.org>,
Jakub Kicinski <kuba@kernel.org>,
Yang Yingliang <yangyingliang@huawei.com>,
netdev@vger.kernel.org, jiri@nvidia.com, davem@davemloft.net,
edumazet@google.com, pabeni@redhat.com
Subject: Re: [PATCH net] net: devlink: fix UAF in devlink_compat_running_version()
Date: Mon, 28 Nov 2022 14:52:00 +0100 [thread overview]
Message-ID: <Y4S7XENL7TgIEtPA@nanopsycho> (raw)
In-Reply-To: <Y4Sgd6fqcfL5c/vg@unreal>
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.
>
>>
>> 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
next prev parent reply other threads:[~2022-11-28 13:52 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-22 12:10 [PATCH net] net: devlink: fix UAF in devlink_compat_running_version() Yang Yingliang
2022-11-22 14:32 ` Leon Romanovsky
2022-11-22 15:25 ` Yang Yingliang
2022-11-22 19:04 ` Leon Romanovsky
2022-11-22 20:27 ` Jakub Kicinski
2022-11-23 1:50 ` Yang Yingliang
2022-11-23 6:40 ` Yang Yingliang
2022-11-23 7:41 ` Leon Romanovsky
2022-11-23 8:34 ` Yang Yingliang
2022-11-23 9:33 ` Leon Romanovsky
2022-11-23 19:18 ` Ido Schimmel
2022-11-24 2:18 ` Jakub Kicinski
2022-11-24 5:56 ` Leon Romanovsky
2022-11-28 9:20 ` Ido Schimmel
2022-11-28 9:58 ` Jiri Pirko
2022-11-28 11:50 ` Leon Romanovsky
2022-11-28 13:52 ` Jiri Pirko [this message]
2022-11-29 8:44 ` Leon Romanovsky
2022-11-29 9:05 ` Jiri Pirko
2022-11-29 11:20 ` Leon Romanovsky
2022-11-29 11:44 ` Jiri Pirko
2022-11-28 18:20 ` Jakub Kicinski
2022-11-29 8:31 ` Jiri Pirko
2022-11-30 2:18 ` Jakub Kicinski
2022-11-30 8:54 ` Leon Romanovsky
2022-11-30 11:32 ` Jiri Pirko
2022-11-30 16:36 ` Jakub Kicinski
2022-11-30 11:42 ` Jiri Pirko
2022-11-30 16:46 ` Jakub Kicinski
2022-11-30 17:00 ` Jiri Pirko
2022-11-30 17:20 ` Jakub Kicinski
2022-11-30 19:20 ` Leon Romanovsky
2022-12-01 8:40 ` Jiri Pirko
2022-12-01 10:05 ` Leon Romanovsky
2022-12-01 12:20 ` Jiri Pirko
2022-12-01 8:39 ` Jiri Pirko
2022-11-30 22:25 ` Jacob Keller
2022-11-24 2:20 ` Jakub Kicinski
2022-11-24 2:47 ` Jakub Kicinski
2022-11-24 7:28 ` Yang Yingliang
2022-11-28 10:01 ` Jiri Pirko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y4S7XENL7TgIEtPA@nanopsycho \
--to=jiri@resnulli.us \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=idosch@idosch.org \
--cc=jiri@nvidia.com \
--cc=kuba@kernel.org \
--cc=leon@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=yangyingliang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).