linux-kernel - Re: [PATCH 2/6] Driver core: Fix memory leak on bus

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]

Date:	Fri, 23 Jul 2010 18:19:23 -0700
From:	Patrick Pannuto <ppannuto@...eaurora.org>
To:	Greg KH <greg@...ah.com>
CC:	linux-kernel@...r.kernel.org, Greg Kroah-Hartman <gregkh@...e.de>,
	Emese Revfy <re.emese@...il.com>,
	Stephen Hemminger <shemminger@...tta.com>,
	Jens Axboe <jens.axboe@...cle.com>
Subject: Re: [PATCH 2/6] Driver core: Fix memory leak on bus_register error
 path

On 07/22/2010 04:41 PM, Greg KH wrote:
> On Thu, Jul 22, 2010 at 03:09:02PM -0700, Patrick Pannuto wrote:
>> There is a subtle memory leak in driver core error path.
>> Consider the simplified view of bus_register (drivers/base/bus.c):
>>
>> priv = kzalloc...
>> kobject_set_name(&priv->subsys.kobj,...) <== allocate in priv->subsys.kobj.name
>> if kset_register(&priv->subsys) FAILS:
> 
> Why would this fail?
> 

This is not a likely failure path at all, but (from my understanding), it
is possible:

kset_register {
   kobject_add_internal {
      create_dir()

is the most likely candidate to fail, mostly likely for EEXIST due to something
else screwy going on. Regardless of how likely it is to fail, it *is* possible,
otherwise, what is the point of checking the return code and having an error
path? If the error path exists (and a panic is not eminent), we shouldn't leak
memory on it IMHO.

>>  
>>  	retval = kset_register(&priv->subsys);
>> -	if (retval)
>> +	if (retval) {
>> +		kfree(priv->subsys.kobj.name);
> 
> I don't think we want to bury the logic of how kobject names are handled
> up here in the bus code, right?  Shouldn't the subsys kobject name be
> able to be cleaned up on its own somehow instead?
> 

So, my first instinct was to use kobject_cleanup, but a few lines above:

	priv->subsys.kobj.ktype = &bus_ktype;

and bus_ktype's definition, with the notable absence of a release method:

static struct kobj_type bus_ktype = {
	.sysfs_ops	= &bus_sysfs_ops,
};

which in kobject_cleanup would yield:

	struct kobj_type *t = get_ktype(kobj);

	if (t && !t->release)
		pr_debug("kobject: '%s' (%p): does not have a release() "
			 "function, it is broken and must be fixed.\n",
			 kobject_name(kobj), kobj);

(if I understand everything correctly)

I have no idea what would constitute a proper 'release' method in this
context, thus I did not write one (and am hoping this patchset would
motivate those who know more than me to write one, or indicate to me how
to write one, if that would be the correct course of action)


Sorry if any of this is trivial / obvious / incorrect; it's my first time
in this code at all, and kobject and friends aren't the easiest to
comprehend on first glance :)

-pat
--
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/