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 for Android: free password hash cracker in your pocket

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

Message-Id: <09c5c383-cd98-4044-9155-cbbd7ab41e85@app.fastmail.com>
Date:   Thu, 24 Nov 2022 07:56:51 +0200
From:   "Leon Romanovsky" <leon@...nel.org>
To:     "Jakub Kicinski" <kuba@...nel.org>,
        "Ido Schimmel" <idosch@...sch.org>
Cc:     "Yang Yingliang" <yangyingliang@...wei.com>,
        netdev@...r.kernel.org, jiri@...dia.com,
        "David Miller" <davem@...emloft.net>, edumazet@...gle.com,
        "Paolo Abeni" <pabeni@...hat.com>
Subject: Re: [PATCH net] net: devlink: fix UAF in devlink_compat_running_version()



On Thu, Nov 24, 2022, at 04:18, 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.

Before you are doing that, let's remind why we do ordering:
1. It was in happy times when some commands were locked and some not. It caused to very exciting things like controlled uaf, e.t.c.
2. We wanted to provide an access from user space after everything devlink related is initialized. 
3. Allow sane debuggability as some drivers called to devlink_register and unregister in random places.

The locking (item 1) was fixed.

Sent from mobile.