Re: [PATCH] usb: core: Fix possible memleak in usb_add_gadget

[Florian Faber <faber@faberman.de>]

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.