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

[Jiri Pirko <jiri@resnulli.us>]

Wed, Nov 30, 2022 at 03:18:26AM CET, kuba@kernel.org wrote:
>On Tue, 29 Nov 2022 09:31:40 +0100 Jiri Pirko wrote:
>> >Cool. Do you also agree with doing proper refcounting for the devlink
>> >instance struct and the liveness check after locking the instance?  
>> 
>> Could you elaborate a bit more? I missed that in the thread and can't
>> find it. Why do we need it?
>
>Look at the __devlink_free() and changes 
>to devlink_compat_flash_update() here:
>
>https://lore.kernel.org/netdev/20211030231254.2477599-3-kuba@kernel.org/

**)
I see. With the change I suggest, meaning doing
devlink_port_register/unregister() and netdev_register/unregister only
for registered devlink instance, you don't need this at all. When you
hit this compat callback, the netdevice is there and therefore devlink
instance is registered for sure.


>
>The model I had in mind (a year ago when it all started) was that 
>the driver takes the devlink instance lock around its entire init path,
>including the registration of the instance. This way the devlink
>instance is never visible "half initialized". I mean - it's "visible"
>as in you can see a notification over netlink before init is done but
>you can't access it until the init in the driver is completed and it
>releases the instance lock.

What is "half-initialized"? Take devlink reload flow for instance. There
are multiple things removed/readded, like devlink_port and related
netdevice. No problem there.

I think that we really need to go a step back and put the
devlink_register at the point of init flow where all things that are
"static" during register lifetime are initialized, the rest would be
initialized later on. This would make things aligned with
devlink_reload() and would make possible to share init/fini/reload
code in drivers.


>
>For that to work and to avoid ordering issues with netdev we need to
>allow unregistering a devlink instance before all references are gone.

References of what? devlink instance, put by devlink_put()?


>
>So we atomically look up and take a reference on a devlink instance.
>Then take its lock. Then under the instance lock we check if it's still
>registered.

As mentioned above (**), I don't think this is needed.


>
>For devlink core that's not a problem it's all hidden in the helpers.