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

[Jiri Pirko <jiri@resnulli.us>]

Wed, Nov 30, 2022 at 05:46:59PM CET, kuba@kernel.org wrote:
>On Wed, 30 Nov 2022 12:42:39 +0100 Jiri Pirko wrote:
>> >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.
>
>If you move devlink registration up it has to be under the instance
>lock, otherwise we're back to reload problems. That implies unregister
>should be under the lock too. But then we can't wait for refs in
>unregister. Perhaps I don't understand the suggestion.

I unlock for register and for the rest of the init I lock again.


>
>> >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.
>
>Yes, but reload is under the instance lock, so nothing can mess with 
>a device in a transitional state.

Sure, that is what I want to do too. To be under instance lock.

>
>> 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.
>
>Yes, I agree that the move should be done but I don't think its
>sufficient.
>
>> >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()?
>
>Yes.
>
>> >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.
>
>But it is, please just let me do it and make the bugs stop 😭

Why exactly is it needed? I don't see it, pardon my ignorance :)

Let me send the RFC of the change tomorrow, you'll see what I mean.