linux-kernel - Re: [PATCH] kobject_set_name

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090627093359.GA3342@localdomain.by>
Date:	Sat, 27 Jun 2009 12:33:59 +0300
From:	Sergey Senozhatsky <sergey.senozhatsky@...l.by>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...l.by>,
	Greg KH <gregkh@...e.de>, Kay Sievers <kay.sievers@...y.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kobject_set_name_vargs memory leak

On (06/26/09 19:10), Eric W. Biederman wrote:
> Date: Fri, 26 Jun 2009 19:10:08 -0700
> From: "Eric W. Biederman" <ebiederm@...ssion.com>
> To: Sergey Senozhatsky <sergey.senozhatsky@...l.by>
> Cc: Greg KH <gregkh@...e.de>, Kay Sievers <kay.sievers@...y.org>,
> 	linux-kernel@...r.kernel.org
> Subject: Re: [PATCH] kobject_set_name_vargs memory leak
> User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux)
> 
> Sergey Senozhatsky <sergey.senozhatsky@...l.by> writes:
> 
> > On (06/26/09 07:49), Greg KH wrote:
> >> We've been through this before (search lkml archives).  If kvasprintf
> >> fails, then we don't want to free old_name, as the caller might want to
> >> do something with it.
> >> 
> >> Or something along those lines, I can't remember the exact reasoning
> >> this early in the morning.
> >> 
> >> Kay, do you remember?
> >>
> > I found.
> > http://lkml.org/lkml/2009/5/11/11
> >
> >>kobject with name set before should not come into this function,
> >>kobject_rename should be used instead.
> >
> > It's just would be safer to kfree or restore I guess.
> 
> Yes.  There does seem to be a good point in there that the code should be:
> BUG_ON(kobj->name);
> 

> And otherwise simply not handle old_name at all.
> 

I run with:

	if (kobj->name) {
		printk(KERN_ERR "name:%s  fmt:%s\n", kobj->name, fmt);
		dump_stack();
	}

I seems ok not to handle old_name.

>Dave Young
>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


Greg, Kay, Eric, what do you think?


diff --git a/lib/kobject.c b/lib/kobject.c
index b512b74..fd1983a 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -215,7 +215,6 @@ static int kobject_add_internal(struct kobject *kobj)
 int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 				  va_list vargs)
 {
-	const char *old_name = kobj->name;
 	char *s;
 
 	if (kobj->name && !fmt)


> Eric
> 

	Sergey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/