* [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu()
@ 2022-11-11 14:59 Yang Yingliang
2022-11-14 11:23 ` Frederic Barrat
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Yang Yingliang @ 2022-11-11 14:59 UTC (permalink / raw)
To: linuxppc-dev; +Cc: fbarrat, gregkh, ajd, arnd, yangyingliang
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;
+ }
rc = ocxl_sysfs_register_afu(info);
if (rc)
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu()
2022-11-11 14:59 [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu() Yang Yingliang
@ 2022-11-14 11:23 ` Frederic Barrat
2022-11-14 11:46 ` Yang Yingliang
2022-11-21 5:52 ` Andrew Donnellan
2022-11-30 9:24 ` Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Frederic Barrat @ 2022-11-14 11:23 UTC (permalink / raw)
To: Yang Yingliang, linuxppc-dev; +Cc: gregkh, ajd, arnd
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;
+ }
Fred
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu()
2022-11-14 11:23 ` Frederic Barrat
@ 2022-11-14 11:46 ` Yang Yingliang
2022-11-14 12:04 ` Frederic Barrat
0 siblings, 1 reply; 6+ messages in thread
From: Yang Yingliang @ 2022-11-14 11:46 UTC (permalink / raw)
To: Frederic Barrat, linuxppc-dev; +Cc: gregkh, ajd, arnd, yangyingliang
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
>
> .
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu()
2022-11-14 11:46 ` Yang Yingliang
@ 2022-11-14 12:04 ` Frederic Barrat
0 siblings, 0 replies; 6+ messages in thread
From: Frederic Barrat @ 2022-11-14 12:04 UTC (permalink / raw)
To: Yang Yingliang, linuxppc-dev; +Cc: gregkh, ajd, arnd
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu()
2022-11-11 14:59 [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu() Yang Yingliang
2022-11-14 11:23 ` Frederic Barrat
@ 2022-11-21 5:52 ` Andrew Donnellan
2022-11-30 9:24 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Andrew Donnellan @ 2022-11-21 5:52 UTC (permalink / raw)
To: Yang Yingliang, linuxppc-dev; +Cc: fbarrat, gregkh, arnd
On Fri, 2022-11-11 at 22:59 +0800, 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>
Thanks for the fix - as you point out, put_device() should clean
everything up that needs cleaning up.
Acked-by: Andrew Donnellan <ajd@linux.ibm.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;
> + }
>
> rc = ocxl_sysfs_register_afu(info);
> if (rc)
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu()
2022-11-11 14:59 [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu() Yang Yingliang
2022-11-14 11:23 ` Frederic Barrat
2022-11-21 5:52 ` Andrew Donnellan
@ 2022-11-30 9:24 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2022-11-30 9:24 UTC (permalink / raw)
To: Yang Yingliang, linuxppc-dev; +Cc: fbarrat, gregkh, ajd, arnd
On Fri, 11 Nov 2022 22:59:29 +0800, 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().
>
> [...]
Applied to powerpc/next.
[1/1] misc: ocxl: fix possible name leak in ocxl_file_register_afu()
https://git.kernel.org/powerpc/c/295faa17722a11cac8dbf51e4c9f9405a5e07ef1
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-30 9:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-11 14:59 [PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu() Yang Yingliang
2022-11-14 11:23 ` Frederic Barrat
2022-11-14 11:46 ` Yang Yingliang
2022-11-14 12:04 ` Frederic Barrat
2022-11-21 5:52 ` Andrew Donnellan
2022-11-30 9:24 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).