* [PATCH 1/3] kobject : kobject_set_name_vargs leak fix @ 2009-05-11 6:16 Dave Young 2009-05-12 16:29 ` Greg KH 2009-05-14 12:04 ` Dave Young 0 siblings, 2 replies; 5+ messages in thread From: Dave Young @ 2009-05-11 6:16 UTC (permalink / raw) To: Greg KH, Linux Kernel Mailing List 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; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] kobject : kobject_set_name_vargs leak fix 2009-05-11 6:16 [PATCH 1/3] kobject : kobject_set_name_vargs leak fix Dave Young @ 2009-05-12 16:29 ` Greg KH 2009-05-14 12:04 ` Dave Young 1 sibling, 0 replies; 5+ messages in thread From: Greg KH @ 2009-05-12 16:29 UTC (permalink / raw) To: Dave Young; +Cc: Greg KH, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] kobject : kobject_set_name_vargs leak fix 2009-05-11 6:16 [PATCH 1/3] kobject : kobject_set_name_vargs leak fix Dave Young 2009-05-12 16:29 ` Greg KH @ 2009-05-14 12:04 ` Dave Young 2009-05-14 12:38 ` Ming Lei 1 sibling, 1 reply; 5+ messages in thread From: Dave Young @ 2009-05-14 12:04 UTC (permalink / raw) To: Greg KH, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] kobject : kobject_set_name_vargs leak fix 2009-05-14 12:04 ` Dave Young @ 2009-05-14 12:38 ` Ming Lei 2009-05-14 12:59 ` Dave Young 0 siblings, 1 reply; 5+ messages in thread From: Ming Lei @ 2009-05-14 12:38 UTC (permalink / raw) To: Dave Young; +Cc: Greg KH, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] kobject : kobject_set_name_vargs leak fix 2009-05-14 12:38 ` Ming Lei @ 2009-05-14 12:59 ` Dave Young 0 siblings, 0 replies; 5+ messages in thread From: Dave Young @ 2009-05-14 12:59 UTC (permalink / raw) To: Ming Lei; +Cc: Greg KH, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-05-14 12:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-05-11 6:16 [PATCH 1/3] kobject : kobject_set_name_vargs leak fix Dave Young 2009-05-12 16:29 ` Greg KH 2009-05-14 12:04 ` Dave Young 2009-05-14 12:38 ` Ming Lei 2009-05-14 12:59 ` Dave Young
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox