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

[Leon Romanovsky <leon@kernel.org>]

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?

Thanks