netdev - Re: [PATCH net] net: devlink: fix UAF in devlink_compat_running

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221130084659.618a8d60@kernel.org>
Date:   Wed, 30 Nov 2022 08:46:59 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Ido Schimmel <idosch@...sch.org>,
        Yang Yingliang <yangyingliang@...wei.com>,
        Leon Romanovsky <leon@...nel.org>, 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 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.

> >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.

> 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 😭