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: <Y35x9oawn/i+nuV3@shredder>
Date:   Wed, 23 Nov 2022 21:18:14 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Yang Yingliang <yangyingliang@...wei.com>
Cc:     Leon Romanovsky <leon@...nel.org>,
        Jakub Kicinski <kuba@...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, Nov 23, 2022 at 04:34:49PM +0800, Yang Yingliang wrote:
> 
> On 2022/11/23 15:41, Leon Romanovsky wrote:
> > On Wed, Nov 23, 2022 at 02:40:24PM +0800, Yang Yingliang wrote:
> > > On 2022/11/23 4:27, Jakub Kicinski wrote:
> > > > On Tue, 22 Nov 2022 21:04:29 +0200 Leon Romanovsky wrote:
> > > > > > > Please fix nsim instead of devlink.
> > > > > > I think if devlink is not registered, it can not be get and used, but there
> > > > > > is no API for driver to check this, can I introduce a new helper name
> > > > > > devlink_is_registered() for driver using.
> > > > > There is no need in such API as driver calls to devlink_register() and
> > > > > as such it knows when devlink is registered.
> > > > > 
> > > > > This UAF is nsim specific issue. Real devices have single .probe()
> > > > > routine with serialized registration flow. None of them will use
> > > > > devlink_is_registered() call.
> > > > Agreed, the fix is to move the register call back.
> > > > Something along the lines of the untested patch below?
> > > > Yang Yingliang would you be able to turn that into a real patch?
> > > > 
> > > > diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> > > > index e14686594a71..26602d5fe0a2 100644
> > > > --- a/drivers/net/netdevsim/dev.c
> > > > +++ b/drivers/net/netdevsim/dev.c
> > > > @@ -1566,12 +1566,15 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
> > > >    	err = devlink_params_register(devlink, nsim_devlink_params,
> > > >    				      ARRAY_SIZE(nsim_devlink_params));
> > > >    	if (err)
> > > > -		goto err_dl_unregister;
> > > > +		goto err_resource_unregister;
> > > >    	nsim_devlink_set_params_init_values(nsim_dev, devlink);
> > > > +	/* here, because params API still expect devlink to be unregistered */
> > > > +	devl_register(devlink);
> > > > +
> > > devlink_set_features() called at last in probe() also needs devlink is not
> > > registered.
> > > >    	err = nsim_dev_dummy_region_init(nsim_dev, devlink);
> > > >    	if (err)
> > > > -		goto err_params_unregister;
> > > > +		goto err_dl_unregister;
> > > >    	err = nsim_dev_traps_init(devlink);
> > > >    	if (err)
> > > > @@ -1610,7 +1613,6 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
> > > >    	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
> > > >    	devlink_set_features(devlink, DEVLINK_F_RELOAD);
> > > >    	devl_unlock(devlink);
> > > > -	devlink_register(devlink);
> > > >    	return 0;
> > > >    err_hwstats_exit:
> > > > @@ -1629,10 +1631,11 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
> > > >    	nsim_dev_traps_exit(devlink);
> > > >    err_dummy_region_exit:
> > > >    	nsim_dev_dummy_region_exit(nsim_dev);
> > > > -err_params_unregister:
> > > > +err_dl_unregister:
> > > > +	devl_unregister(devlink);
> > > It races with dev_ethtool():
> > > dev_ethtool
> > >    devlink_try_get()
> > >                                  nsim_drv_probe
> > >                                  devl_lock()
> > >      devl_lock()
> > >                                  devlink_unregister()
> > >                                    devlink_put()
> > >                                    wait_for_completion() <- the refcount is
> > > got in dev_ethtool, it causes ABBA deadlock
> > But all these races are nsim specific ones.
> > Can you please explain why devlink.[c|h] MUST be changed and nsim can't
> > be fixed?
> 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.