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

[Frederic Barrat <fbarrat@linux.ibm.com>]

On 14/11/2022 12:46, Yang Yingliang wrote:
> 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.


You're right, I went too fast and the patch is correct.
So:
Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred