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

[Yang Yingliang <yangyingliang@huawei.com>]

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;
+
         return &ns->nsim_dev_port->devlink_port;
  }

Thanks,
Yang
>
> Thanks
> .