[1/3] usb: gadget: storage: Fix reference count if fsg_alloc_inst() failed

[1/3] usb: gadget: storage: Fix reference count if fsg_alloc_inst() failed

[Jaejoong Kim]

The reference count is initialized in fsg_alloc_inst(). So if
fsg_alloc_inst() fails, we need to add kref_put() to free an
allocated memory.

Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
---
 drivers/usb/gadget/function/f_mass_storage.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index acecd13..83eeafe 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3392,6 +3392,7 @@ static struct usb_function_instance *fsg_alloc_inst(void)
 release_buffers:
 	fsg_common_free_buffers(opts->common);
 release_opts:
+	fsg_common_put(opts->common);
 	kfree(opts);
 	return ERR_PTR(rc);
 }

[1/3] usb: gadget: storage: Fix reference count if fsg_alloc_inst() failed

[Alan Stern]

On Thu, 14 Jun 2018, Jaejoong Kim wrote:

> The reference count is initialized in fsg_alloc_inst(). So if
> fsg_alloc_inst() fails, we need to add kref_put() to free an
> allocated memory.
> 
> Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
> ---
>  drivers/usb/gadget/function/f_mass_storage.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index acecd13..83eeafe 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3392,6 +3392,7 @@ static struct usb_function_instance *fsg_alloc_inst(void)
>  release_buffers:
>  	fsg_common_free_buffers(opts->common);
>  release_opts:
> +	fsg_common_put(opts->common);
>  	kfree(opts);
>  	return ERR_PTR(rc);
>  }

I'm not at all sure about this.  Michal is a lot more familiar with 
this part of the code than I am.

It looks like there's a memory leak if fsg_common_release() isn't 
called here.  But if it is called at this point, it might not behave 
properly.

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[1/3] usb: gadget: storage: Fix reference count if fsg_alloc_inst() failed

[Michał Nazarewicz]

2018-06-22 2:23 GMT+01:00 Jaejoong Kim <climbbb.kim@gmail.com>:
> Hi Michal,
>
> Could you check this patch?

Sorry about long delay, I haven’t settled in my new place yet (which
is also why I’m using Gmail).

> 2018년 6월 14일 (목) 오후 11:55, Alan Stern <stern@rowland.harvard.edu>님이 작성:
>>
>> On Thu, 14 Jun 2018, Jaejoong Kim wrote:
>>
>> > The reference count is initialized in fsg_alloc_inst(). So if
>> > fsg_alloc_inst() fails, we need to add kref_put() to free an
>> > allocated memory.
>> >
>> > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
>> > ---
>> >  drivers/usb/gadget/function/f_mass_storage.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c
>> > b/drivers/usb/gadget/function/f_mass_storage.c
>> > index acecd13..83eeafe 100644
>> > --- a/drivers/usb/gadget/function/f_mass_storage.c
>> > +++ b/drivers/usb/gadget/function/f_mass_storage.c
>> > @@ -3392,6 +3392,7 @@ static struct usb_function_instance
>> > *fsg_alloc_inst(void)
>> >  release_buffers:
>> >       fsg_common_free_buffers(opts->common);
>> >  release_opts:
>> > +     fsg_common_put(opts->common);
>> >       kfree(opts);
>> >       return ERR_PTR(rc);
>> >  }

This looks legitimate but in the current form could lead to dereference
of an uninitialized pointer. Furthermore, calling free_buffers is no longer
necessary.  To solve the dereference problem, add

        commo->buffhds = NULL;

to fsg_common_setup (or add memset(common, 0, sizeof *common)
to the path without kzalloc). With that, the the two error handling labels
can be collapsed to one:

error:
      fsg_common_put(opts->common);
      kfree(opts);
      return ERR_PTR(rc);

>>
>> I'm not at all sure about this.  Michal is a lot more familiar with
>> this part of the code than I am.
>>
>> It looks like there's a memory leak if fsg_common_release() isn't
>> called here.  But if it is called at this point, it might not behave
>> properly.
>>
>> Alan Stern
>>
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html