linux-kernel - Re: [PATCH 10/14] scsi: osd: fix device

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] [day] [month] [year] [list]

Message-ID: <4C978C74.1040104@panasas.com>
Date:	Mon, 20 Sep 2010 18:31:48 +0200
From:	Boaz Harrosh <bharrosh@...asas.com>
To:	James Bottomley <James.Bottomley@...e.de>
CC:	Greg KH <gregkh@...e.de>, Dan Carpenter <error27@...il.com>,
	Vasiliy Kulikov <segooon@...il.com>,
	kernel-janitors@...r.kernel.org,
	Benny Halevy <bhalevy@...asas.com>, Tejun Heo <tj@...nel.org>,
	Arnd Bergmann <arnd@...db.de>, osd-dev@...n-osd.org,
	linux-scsi@...r.kernel.org, cornelia.huck@...ibm.com,
	linux-kernel@...r.kernel.org, Kay Sievers <kay.sievers@...y.org>
Subject: Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

On 09/20/2010 05:55 PM, James Bottomley wrote:
> On Mon, 2010-09-20 at 17:42 +0200, Boaz Harrosh wrote:
>> I think I have a compromise. If it is indeed the dev_set_name() leak
>> then we can just deallocate the name on the error return path. Therefore
>> any drivers that have the device embedded and rely on it been freed without
>> calling _put will be fine as before. And these calling _put will be fine
>> as well.
>>
>> See below
>> Boaz
>> ---
>> git diff --stat -p -M drivers/base/core.c
>>  drivers/base/core.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index d1b2c9a..054fac2 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -1068,6 +1068,7 @@ done:
>>  	if (parent)
>>  		put_device(parent);
>>  name_error:
>> +	kfree(dev->kobj.name);
>>  	kfree(dev->p);
>>  	dev->p = NULL;
>>  	goto done;
> 
> That's not really good enough ... it will result in a double free
> because the final put (if there is one) will free the name again.
> 
> This would work, but it feels a bit wrong to be poking at the kobject
> from within the driver core ... perhaps a kobject_free_name() call might
> be better?
> 

It would not be the first place to directly access kobj. members but sure
that will work as well.

> James
> 
> ---
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d1b2c9a..88ffaca 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1067,6 +1067,8 @@ done:
>  	cleanup_device_parent(dev);
>  	if (parent)
>  		put_device(parent);
> +	kfree(dev->kobj.name);
> +	dev->kobj.name = NULL;

Rrr thanks, the NULL. Therefore the lack of a signed-off ;-)

I take it that you agree with me that for some code it would be nice
if we add support for not mandating a device_put after a device_register
for the embedded types? For sake of already written code?

>  name_error:
>  	kfree(dev->p);
>  	dev->p = NULL;
> 
> 

Thanks
Boaz
--
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/