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

[Jacob Keller <jacob.e.keller@intel.com>]

On 11/23/2022 11:18 AM, Ido Schimmel wrote:
> 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.
> 

Shouldn't devlink_free never free until after all references drop? 
That's how typical reference count works. I guess its just assuming that 
unregister will prevent this but....

> 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.
> 

We used to register devlink before netdev, but this was changed at some 
point because we wanted to register it late after things had been setup.

> 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.

Yea that seems weird. I would expect ethtool to get to devlink via 
netdev? (especially now with the way we changed netdev to hold the devlink?)

Hmm.