[PATCH 1/1] Free fw_priv in fw_create_instance

[PATCH 1/1] Free fw_priv in fw_create_instance

[Rajan Aggarwal]

fw_priv is not being freed in some of the error scenarios in
fw_create_instance.
This patch makes sure that this is kfreed properly in all error
situations in the fw_create_instance logic where it is required.

Signed-off-by: Rajan Aggarwal <rajan.aggarwal85@gmail.com>
---
 drivers/base/firmware_class.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 06ed6b4..3ddbc48 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -497,6 +497,7 @@ err_del_dev:
 	device_del(f_dev);
 err_put_dev:
 	put_device(f_dev);
+	kfree(fw_priv);
 err_out:
 	return ERR_PTR(error);
 }
-- 
1.7.4.1

Re: [PATCH 1/1] Free fw_priv in fw_create_instance

[Greg KH]

On Wed, Aug 31, 2011 at 12:36:32AM +0530, Rajan Aggarwal wrote:
> fw_priv is not being freed in some of the error scenarios in
> fw_create_instance.

Not true.

> This patch makes sure that this is kfreed properly in all error
> situations in the fw_create_instance logic where it is required.

Nope, this will cause a double-free to happen.

> Signed-off-by: Rajan Aggarwal <rajan.aggarwal85@gmail.com>
> ---
>  drivers/base/firmware_class.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 06ed6b4..3ddbc48 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -497,6 +497,7 @@ err_del_dev:
>  	device_del(f_dev);
>  err_put_dev:
>  	put_device(f_dev);
> +	kfree(fw_priv);

Did you test this patch out?  What happened when you tested it?

How did you find this "problem"?

Care to look a bit closer at the code?

I'd give you a hint, but it's more sporting this way :)

thanks,

greg k-h

Re: [PATCH 1/1] Free fw_priv in fw_create_instance

[Rajan Aggarwal]

Hi,


On Wed, Aug 31, 2011 at 12:49 AM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Aug 31, 2011 at 12:36:32AM +0530, Rajan Aggarwal wrote:
>> fw_priv is not being freed in some of the error scenarios in
>> fw_create_instance.
>
> Not true.
>
>> This patch makes sure that this is kfreed properly in all error
>> situations in the fw_create_instance logic where it is required.
>
> Nope, this will cause a double-free to happen.

>From what you say, the only suspicious line seems to be the following
assignment:
f_dev = &fw_priv->dev;
However, I cannot make out how this can be freed by device_del or
put_device as I
don't see the logic anywhere where f_dev is decremented to get the
address of fw_priv.

>
>> Signed-off-by: Rajan Aggarwal <rajan.aggarwal85@gmail.com>
>> ---
>>  drivers/base/firmware_class.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index 06ed6b4..3ddbc48 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -497,6 +497,7 @@ err_del_dev:
>>       device_del(f_dev);
>>  err_put_dev:
>>       put_device(f_dev);
>> +     kfree(fw_priv);
>
> Did you test this patch out?  What happened when you tested it?
>
> How did you find this "problem"?
>
> Care to look a bit closer at the code?
>
> I'd give you a hint, but it's more sporting this way :)
>

I was looking at some bluetooth code and went into the request_firmware API
and saw this.
I was browsing through this code and this looked like a problem.
I have just started using git and decided to post this email.

But I still don't see how the fw_priv can be freed. :)
There might be some place in kobject where this decrementation is
happening but I
must be missing something.

> thanks,
>
> greg k-h
>

Re: [PATCH 1/1] Free fw_priv in fw_create_instance

[Greg KH]

On Wed, Aug 31, 2011 at 01:16:24AM +0530, Rajan Aggarwal wrote:
> Hi,
> 
> 
> On Wed, Aug 31, 2011 at 12:49 AM, Greg KH <gregkh@suse.de> wrote:
> > On Wed, Aug 31, 2011 at 12:36:32AM +0530, Rajan Aggarwal wrote:
> >> fw_priv is not being freed in some of the error scenarios in
> >> fw_create_instance.
> >
> > Not true.
> >
> >> This patch makes sure that this is kfreed properly in all error
> >> situations in the fw_create_instance logic where it is required.
> >
> > Nope, this will cause a double-free to happen.
> 
> >From what you say, the only suspicious line seems to be the following
> assignment:
> f_dev = &fw_priv->dev;
> However, I cannot make out how this can be freed by device_del or
> put_device as I don't see the logic anywhere where f_dev is
> decremented to get the address of fw_priv.

Look at the release function for the f_dev that was set up when the
class pointer was assigned to it.  That function takes a pointer to a
"base" struct device, and back casts it to the fw_priv structure, and
then frees it.

That is how the driver core, and the kobject, and the kref model works.
When the last reference to the object is released, the release
function is called, freeing up the memory of the object.

Take a look at the Documentation/kobject.txt file for details as to how
this all works if you are still curious.

Hope this helps,

greg k-h