[PATCH -next] nvmem: core: Fix memleak in nvmem_register()

[PATCH -next] nvmem: core: Fix memleak in nvmem_register()

[Gaosheng Cui]

dev_set_name will alloc memory for nvmem->dev.kobj.name in
nvmem_register, when nvmem_validate_keepouts failed, nvmem's
memory will be freed and return, but nobody will free memory
for nvmem->dev.kobj.name, there will be memleak, so using
kfree_const(nvmem->dev.kobj.name) to fix it.

Fixes: de0534df9347 ("nvmem: core: fix error handling while validating keepout regions")
Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
---
 drivers/nvmem/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 1e3c754efd0d..6c75c9afa5f3 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -833,6 +833,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		rval = nvmem_validate_keepouts(nvmem);
 		if (rval) {
 			ida_free(&nvmem_ida, nvmem->id);
+			kfree_const(nvmem->dev.kobj.name);
 			kfree(nvmem);
 			return ERR_PTR(rval);
 		}
-- 
2.25.1

Re: [PATCH -next] nvmem: core: Fix memleak in nvmem_register()

[Greg KH]

On Sat, Jul 16, 2022 at 03:53:52PM +0800, Gaosheng Cui wrote:
> dev_set_name will alloc memory for nvmem->dev.kobj.name in
> nvmem_register, when nvmem_validate_keepouts failed, nvmem's
> memory will be freed and return, but nobody will free memory
> for nvmem->dev.kobj.name, there will be memleak, so using
> kfree_const(nvmem->dev.kobj.name) to fix it.
> 
> Fixes: de0534df9347 ("nvmem: core: fix error handling while validating keepout regions")
> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
>  drivers/nvmem/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 1e3c754efd0d..6c75c9afa5f3 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -833,6 +833,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  		rval = nvmem_validate_keepouts(nvmem);
>  		if (rval) {
>  			ida_free(&nvmem_ida, nvmem->id);
> +			kfree_const(nvmem->dev.kobj.name);
>  			kfree(nvmem);

Something is really wrong, you should never be touching the name pointer
of a kobject directly like this.  Also the device structure itself
should be cleaning up the memory, not a kfree.  So this feels wrong...

greg k-h

Re: [PATCH -next] nvmem: core: Fix memleak in nvmem_register()

[cuigaosheng]

Thanks for your time.

I have made the v2 patch and moving dev_set_name after nvmem_validate_keepouts
to fix the issue, hope you can provide some advice.

Thanks.

在 2022/7/16 16:23, Greg KH 写道:
> On Sat, Jul 16, 2022 at 03:53:52PM +0800, Gaosheng Cui wrote:
>> dev_set_name will alloc memory for nvmem->dev.kobj.name in
>> nvmem_register, when nvmem_validate_keepouts failed, nvmem's
>> memory will be freed and return, but nobody will free memory
>> for nvmem->dev.kobj.name, there will be memleak, so using
>> kfree_const(nvmem->dev.kobj.name) to fix it.
>>
>> Fixes: de0534df9347 ("nvmem: core: fix error handling while validating keepout regions")
>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
>> ---
>>   drivers/nvmem/core.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 1e3c754efd0d..6c75c9afa5f3 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -833,6 +833,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>   		rval = nvmem_validate_keepouts(nvmem);
>>   		if (rval) {
>>   			ida_free(&nvmem_ida, nvmem->id);
>> +			kfree_const(nvmem->dev.kobj.name);
>>   			kfree(nvmem);
> Something is really wrong, you should never be touching the name pointer
> of a kobject directly like this.  Also the device structure itself
> should be cleaning up the memory, not a kfree.  So this feels wrong...
>
> greg k-h
> .

Re: [PATCH -next] nvmem: core: Fix memleak in nvmem_register()

[Srinivas Kandagatla]

On 16/07/2022 09:23, Greg KH wrote:
> On Sat, Jul 16, 2022 at 03:53:52PM +0800, Gaosheng Cui wrote:
>> dev_set_name will alloc memory for nvmem->dev.kobj.name in
>> nvmem_register, when nvmem_validate_keepouts failed, nvmem's
>> memory will be freed and return, but nobody will free memory
>> for nvmem->dev.kobj.name, there will be memleak, so using
>> kfree_const(nvmem->dev.kobj.name) to fix it.
>>
>> Fixes: de0534df9347 ("nvmem: core: fix error handling while validating keepout regions")
>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
>> ---
>>   drivers/nvmem/core.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 1e3c754efd0d..6c75c9afa5f3 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -833,6 +833,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>   		rval = nvmem_validate_keepouts(nvmem);
>>   		if (rval) {
>>   			ida_free(&nvmem_ida, nvmem->id);
>> +			kfree_const(nvmem->dev.kobj.name);
>>   			kfree(nvmem);
> 
> Something is really wrong, you should never be touching the name pointer
> of a kobject directly like this.  Also the device structure itself
> should be cleaning up the memory, not a kfree.  So this feels wrong...

The problem is that at this point we have not device_intialize() the dev 
yet, so there is no refcount setup for kobject yet.

I see two drivers (drivers/nvme/core.c and drivers/base/node.c)  doing 
something similar.

Most common (99.99%) usage pattern is

dev_set_name(...)
ret = device_register(..);
if (ret < 0)
	put_device(..)

but in this particular case we have:

dev_set_name(...)
..
ret = do_some_error_checks()
if (ret < 0)
	return ret;
...
ret = device_register(..);
if (ret < 0)
	put_device(..)

One solution is to follow the most used pattern and move the checks 
after device register and let device core take care of freeing this 
kobj.name as part of its cleanup routine.

--srini

> 
> greg k-h