[PATCH 1/3] kobject : kobject_set_name_vargs leak fix

[PATCH 1/3] kobject : kobject_set_name_vargs leak fix

[Dave Young]

kobject_set_name_vargs will leak the old_name when return -ENOMEM,
move the kfree(old_name) before the return path.

Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
---
lib/kobject.c |    2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff -uprN linux.old/lib/kobject.c linux.new/lib/kobject.c
--- linux.old/lib/kobject.c	2009-05-11 13:59:01.000000000 +0800
+++ linux.new/lib/kobject.c	2009-05-11 13:59:34.000000000 +0800
@@ -221,6 +221,7 @@ int kobject_set_name_vargs(struct kobjec
 	if (kobj->name && !fmt)
 		return 0;

+	kfree(old_name);
 	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
 	if (!kobj->name)
 		return -ENOMEM;
@@ -229,7 +230,6 @@ int kobject_set_name_vargs(struct kobjec
 	while ((s = strchr(kobj->name, '/')))
 		s[0] = '!';

-	kfree(old_name);
 	return 0;
 }

Re: [PATCH 1/3] kobject : kobject_set_name_vargs leak fix

[Greg KH]

On Mon, May 11, 2009 at 02:16:36PM +0800, Dave Young wrote:
> kobject_set_name_vargs will leak the old_name when return -ENOMEM,
> move the kfree(old_name) before the return path.
> 
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
> ---
> lib/kobject.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -uprN linux.old/lib/kobject.c linux.new/lib/kobject.c
> --- linux.old/lib/kobject.c	2009-05-11 13:59:01.000000000 +0800
> +++ linux.new/lib/kobject.c	2009-05-11 13:59:34.000000000 +0800
> @@ -221,6 +221,7 @@ int kobject_set_name_vargs(struct kobjec
>  	if (kobj->name && !fmt)
>  		return 0;
> 
> +	kfree(old_name);
>  	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
>  	if (!kobj->name)
>  		return -ENOMEM;
> @@ -229,7 +230,6 @@ int kobject_set_name_vargs(struct kobjec
>  	while ((s = strchr(kobj->name, '/')))
>  		s[0] = '!';
> 
> -	kfree(old_name);
>  	return 0;
>  }

No, it would be safer to put the kobj->name pointer back to old_name if
the kvasprintf() call failed.  That way the caller can properly clean up
if needed.

Care to respin this?

thanks,

greg k-h

Re: [PATCH 1/3] kobject : kobject_set_name_vargs leak fix

[Dave Young]

On Mon, May 11, 2009 at 2:16 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> kobject_set_name_vargs will leak the old_name when return -ENOMEM,
> move the kfree(old_name) before the return path.

Hi, greg

I rethought about this problem, does such issue exist really? I means
that kobject->name != NULL scenario.

there's following comments of this function:

 * This sets the name of the kobject.  If you have already added the
 * kobject to the system, you must call kobject_rename() in order to
 * change the name of the kobject

So what if something like :

if (kobject->name) {
 WARN(1, KERN_WARNING, "there's name for kobject already!");
 return -EINVAL;
}

if (!fmt)
 return 0;

>
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
> ---
> lib/kobject.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff -uprN linux.old/lib/kobject.c linux.new/lib/kobject.c
> --- linux.old/lib/kobject.c     2009-05-11 13:59:01.000000000 +0800
> +++ linux.new/lib/kobject.c     2009-05-11 13:59:34.000000000 +0800
> @@ -221,6 +221,7 @@ int kobject_set_name_vargs(struct kobjec
>        if (kobj->name && !fmt)
>                return 0;
>
> +       kfree(old_name);
>        kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
>        if (!kobj->name)
>                return -ENOMEM;
> @@ -229,7 +230,6 @@ int kobject_set_name_vargs(struct kobjec
>        while ((s = strchr(kobj->name, '/')))
>                s[0] = '!';
>
> -       kfree(old_name);
>        return 0;
>  }
>



-- 
Regards
dave

Re: [PATCH 1/3] kobject : kobject_set_name_vargs leak fix

[Ming Lei]

2009/5/14 Dave Young <hidave.darkstar@gmail.com>:
> On Mon, May 11, 2009 at 2:16 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>> kobject_set_name_vargs will leak the old_name when return -ENOMEM,
>> move the kfree(old_name) before the return path.
>
> Hi, greg
>
> I rethought about this problem, does such issue exist really? I means
> that kobject->name != NULL scenario.
>
> there's following comments of this function:
>
>  * This sets the name of the kobject.  If you have already added the
>  * kobject to the system, you must call kobject_rename() in order to
>  * change the name of the kobject
>
> So what if something like :
>
> if (kobject->name) {
>  WARN(1, KERN_WARNING, "there's name for kobject already!");
>  return -EINVAL;
> }
>
> if (!fmt)
>  return 0;
>
>>
>> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
>> ---
>> lib/kobject.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff -uprN linux.old/lib/kobject.c linux.new/lib/kobject.c
>> --- linux.old/lib/kobject.c     2009-05-11 13:59:01.000000000 +0800
>> +++ linux.new/lib/kobject.c     2009-05-11 13:59:34.000000000 +0800
>> @@ -221,6 +221,7 @@ int kobject_set_name_vargs(struct kobjec
>>        if (kobj->name && !fmt)
>>                return 0;
>>
>> +       kfree(old_name);
>>        kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
>>        if (!kobj->name)
>>                return -ENOMEM;

IMO, Greg means it is better that the fix should be :
        if (!kobj->name) {
                kobj->name = old_name;
                return -ENOMEM;
        }

Right?

>> @@ -229,7 +230,6 @@ int kobject_set_name_vargs(struct kobjec
>>        while ((s = strchr(kobj->name, '/')))
>>                s[0] = '!';
>>
>> -       kfree(old_name);
>>        return 0;
>>  }
>>
>
>
>
> --
> Regards
> dave
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Lei Ming

Re: [PATCH 1/3] kobject : kobject_set_name_vargs leak fix

[Dave Young]

On Thu, May 14, 2009 at 8:38 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> 2009/5/14 Dave Young <hidave.darkstar@gmail.com>:
>> On Mon, May 11, 2009 at 2:16 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>>> kobject_set_name_vargs will leak the old_name when return -ENOMEM,
>>> move the kfree(old_name) before the return path.
>>
>> Hi, greg
>>
>> I rethought about this problem, does such issue exist really? I means
>> that kobject->name != NULL scenario.
>>
>> there's following comments of this function:
>>
>>  * This sets the name of the kobject.  If you have already added the
>>  * kobject to the system, you must call kobject_rename() in order to
>>  * change the name of the kobject
>>
>> So what if something like :
>>
>> if (kobject->name) {
>>  WARN(1, KERN_WARNING, "there's name for kobject already!");
>>  return -EINVAL;
>> }
>>
>> if (!fmt)
>>  return 0;
>>
>>>
>>> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
>>> ---
>>> lib/kobject.c |    2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff -uprN linux.old/lib/kobject.c linux.new/lib/kobject.c
>>> --- linux.old/lib/kobject.c     2009-05-11 13:59:01.000000000 +0800
>>> +++ linux.new/lib/kobject.c     2009-05-11 13:59:34.000000000 +0800
>>> @@ -221,6 +221,7 @@ int kobject_set_name_vargs(struct kobjec
>>>        if (kobj->name && !fmt)
>>>                return 0;
>>>
>>> +       kfree(old_name);
>>>        kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
>>>        if (!kobj->name)
>>>                return -ENOMEM;
>
> IMO, Greg means it is better that the fix should be :
>        if (!kobj->name) {
>                kobj->name = old_name;
>                return -ENOMEM;
>        }
>
> Right?

Hm, it's better against my original patch. I have gave up the approach.

It does not make sense for (!old_name) case , furthermore as my reply
kobject with name set before should not come into this function,
kobject_rename should be used instead.

>
>>> @@ -229,7 +230,6 @@ int kobject_set_name_vargs(struct kobjec
>>>        while ((s = strchr(kobj->name, '/')))
>>>                s[0] = '!';
>>>
>>> -       kfree(old_name);
>>>        return 0;
>>>  }
>>>
>>
>>
>>
>> --
>> Regards
>> dave
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
>
>
> --
> Lei Ming
>



-- 
Regards
dave