Re: [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu()

[Yang Yingliang <yangyingliang@huawei.com>]

Hi,

On 2022/11/14 19:23, Frederic Barrat wrote:
>
>
> On 11/11/2022 15:59, Yang Yingliang wrote:
>> If device_register() returns error in ocxl_file_register_afu(),
>> the name allocated by dev_set_name() need be freed. As comment
>> of device_register() says, it should use put_device() to give
>> up the reference in the error path. So fix this by calling
>> put_device(), then the name can be freed in kobject_cleanup(),
>> and info is freed in info_release().
>>
>> Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl 
>> backend & frontend")
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>>   drivers/misc/ocxl/file.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
>> index d46dba2df5a1..452d5777a0e4 100644
>> --- a/drivers/misc/ocxl/file.c
>> +++ b/drivers/misc/ocxl/file.c
>> @@ -541,8 +541,11 @@ int ocxl_file_register_afu(struct ocxl_afu *afu)
>>           goto err_put;
>>         rc = device_register(&info->dev);
>> -    if (rc)
>> -        goto err_put;
>> +    if (rc) {
>> +        free_minor(info);
>> +        put_device(&info->dev);
>> +        return rc;
>> +    }
>
>
> While I agree that a put_device() is needed on that error path, the 
> fix above is not correct as it forgets to release the afu reference 
> and the memory allocated in info. That was taken care of by the jump 
> to the err_put label, so it should be kept. Something like:
>
> -    if (rc)
> +    if (rc) {
> +        put_device((&info->dev);
>          goto err_put;
> +    }
The 'info' and the reference is released in info_release().

Here is call chain:
put_device()
   kobject_release()
     kobject_cleanup()
       device_release()
         info_release()

static void info_release(struct device *dev)
{
         struct ocxl_file_info *info = container_of(dev, struct 
ocxl_file_info, dev);

         ocxl_afu_put(info->afu);
         kfree(info);
}
So it don't need jump to the error label in this case.

Thanks,
Yang
>
>
>   Fred
>
> .