diff mbox series

usb: core: Fix possible memleak in usb_add_gadget

Message ID 311d64c6-f0e2-da42-5619-1efe46df0007@faberman.de
State New
Headers show
Series usb: core: Fix possible memleak in usb_add_gadget | expand

Commit Message

Florian Faber Sept. 4, 2021, 3:34 p.m. UTC
The memory for the udc structure allocated via kzalloc in line 1295 is 
not freed in the error handling code, leading to a memory leak in case 
of an error.

Singed-off-by: Florian Faber <faber@faberman.de>

---
  drivers/usb/gadget/udc/core.c | 2 ++
  1 file changed, 2 insertions(+)

Comments

Greg Kroah-Hartman Sept. 5, 2021, 2:56 p.m. UTC | #1
On Sat, Sep 04, 2021 at 05:34:29PM +0200, Florian Faber wrote:
> The memory for the udc structure allocated via kzalloc in line 1295 is not

> freed in the error handling code, leading to a memory leak in case of an

> error.

> 

> Singed-off-by: Florian Faber <faber@faberman.de>

> 

> ---

>  drivers/usb/gadget/udc/core.c | 2 ++

>  1 file changed, 2 insertions(+)

> 

> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c

> index 14fdf918ecfe..a1270a44855a 100644

> --- a/drivers/usb/gadget/udc/core.c

> +++ b/drivers/usb/gadget/udc/core.c

> @@ -1346,6 +1346,8 @@ int usb_add_gadget(struct usb_gadget *gadget)

> 

>   err_put_udc:

>  	put_device(&udc->dev);

> +	kfree(udc);

> +	gadget->udc = NULL;

> 

>   error:

>  	return ret;

> -- 

> 2.33.0

> 

> Flo

> -- 

> Machines can do the work, so people have time to think.


Did you test this?  I think you will find that you just caused a
use-after-free :(

Please read the documentation for device_initialize() for why this is
not the correct thing to do here.

thanks,

greg k-h
Florian Faber Sept. 5, 2021, 3:16 p.m. UTC | #2
Greg,

On 9/5/21 4:56 PM, Greg KH wrote:
> On Sat, Sep 04, 2021 at 05:34:29PM +0200, Florian Faber wrote:

>> The memory for the udc structure allocated via kzalloc in line 1295 is not

>> freed in the error handling code, leading to a memory leak in case of an

>> error.

>>

>> Singed-off-by: Florian Faber <faber@faberman.de>

>>

>> ---

>>   drivers/usb/gadget/udc/core.c | 2 ++

>>   1 file changed, 2 insertions(+)

>>

>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c

>> index 14fdf918ecfe..a1270a44855a 100644

>> --- a/drivers/usb/gadget/udc/core.c

>> +++ b/drivers/usb/gadget/udc/core.c

>> @@ -1346,6 +1346,8 @@ int usb_add_gadget(struct usb_gadget *gadget)

>>

>>    err_put_udc:

>>   	put_device(&udc->dev);

>> +	kfree(udc);

>> +	gadget->udc = NULL;

>>

>>    error:

>>   	return ret;

>> -- 

>> 2.33.0

>>

>> Flo

>> -- 

>> Machines can do the work, so people have time to think.

> 

> Did you test this?  I think you will find that you just caused a

> use-after-free :(


Correct, please forget about this patch.

This 'leak' was found by Klocwork and seemed plausible at first 
oversight. Sorry for wasting your time and not checking it further.

> Please read the documentation for device_initialize() for why this is

> not the correct thing to do here.


I know now :) It was a bit counter intuitive that two different methods 
are used for memory allocation and freeing.

Regarding the other patch: I found the real culprit in the meantime. The 
UDC driver (broadcom iproc udc, out of tree) did not call composite's 
disconnect when VBUS is lost. Out of the three gadgets I am using, only 
mass storage misbehaved that badly, which led me on the wrong track there.


Flo
-- 
Machines can do the work, so people have time to think.
Greg Kroah-Hartman Sept. 5, 2021, 4:27 p.m. UTC | #3
On Sun, Sep 05, 2021 at 05:16:36PM +0200, Florian Faber wrote:
> Greg,

> 

> On 9/5/21 4:56 PM, Greg KH wrote:

> > On Sat, Sep 04, 2021 at 05:34:29PM +0200, Florian Faber wrote:

> > > The memory for the udc structure allocated via kzalloc in line 1295 is not

> > > freed in the error handling code, leading to a memory leak in case of an

> > > error.

> > > 

> > > Singed-off-by: Florian Faber <faber@faberman.de>

> > > 

> > > ---

> > >   drivers/usb/gadget/udc/core.c | 2 ++

> > >   1 file changed, 2 insertions(+)

> > > 

> > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c

> > > index 14fdf918ecfe..a1270a44855a 100644

> > > --- a/drivers/usb/gadget/udc/core.c

> > > +++ b/drivers/usb/gadget/udc/core.c

> > > @@ -1346,6 +1346,8 @@ int usb_add_gadget(struct usb_gadget *gadget)

> > > 

> > >    err_put_udc:

> > >   	put_device(&udc->dev);

> > > +	kfree(udc);

> > > +	gadget->udc = NULL;

> > > 

> > >    error:

> > >   	return ret;

> > > -- 

> > > 2.33.0

> > > 

> > > Flo

> > > -- 

> > > Machines can do the work, so people have time to think.

> > 

> > Did you test this?  I think you will find that you just caused a

> > use-after-free :(

> 

> Correct, please forget about this patch.

> 

> This 'leak' was found by Klocwork and seemed plausible at first oversight.

> Sorry for wasting your time and not checking it further.


What is "Klockwork"?  How can it miss the reference counted logic that
all drivers use in the kernel?

> > Please read the documentation for device_initialize() for why this is

> > not the correct thing to do here.

> 

> I know now :) It was a bit counter intuitive that two different methods are

> used for memory allocation and freeing.


The joy of reference counted stuff, sorry.

thanks,

greg k-h
Florian Faber Sept. 5, 2021, 7:58 p.m. UTC | #4
On 9/5/21 6:27 PM, Greg KH wrote:
> On Sun, Sep 05, 2021 at 05:16:36PM +0200, Florian Faber wrote:

>> This 'leak' was found by Klocwork and seemed plausible at first oversight.

>> Sorry for wasting your time and not checking it further.

> 

> What is "Klockwork"? 


Some static code analysis tool. I did not know it either.

> How can it miss the reference counted logic that

> all drivers use in the kernel?


Obviously it is not running like clockwork :)

Anyway, got the problem fixed, sorry for the noise.


Flo
-- 
Machines can do the work, so people have time to think.
diff mbox series

Patch

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 14fdf918ecfe..a1270a44855a 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1346,6 +1346,8 @@  int usb_add_gadget(struct usb_gadget *gadget)

   err_put_udc:
  	put_device(&udc->dev);
+	kfree(udc);
+	gadget->udc = NULL;

   error:
  	return ret;