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
| ||
|
Date: Wed, 23 Nov 2022 11:33:14 +0200 From: Leon Romanovsky <leon@...nel.org> To: Yang Yingliang <yangyingliang@...wei.com> Cc: 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: > --- a/drivers/net/netdevsim/netdev.c > +++ b/drivers/net/netdevsim/netdev.c > @@ -242,6 +242,9 @@ static struct devlink_port *nsim_get_devlink_port(struct > net_device *dev) > { > struct netdevsim *ns = netdev_priv(dev); > > + if (!ns->nsim_dev->devlink_registered) > + return NULL; Something like that, but you need to make sure that this check is protected by some sort of the locking. > + > return &ns->nsim_dev_port->devlink_port; > } > > Thanks, > Yang > > > > Thanks > > .
Powered by blists - more mailing lists