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: <20221129181826.79cef64c@kernel.org>
Date:   Tue, 29 Nov 2022 18:18:26 -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 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/

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.

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

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.

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