linux-kernel - Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

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

Date:	Wed, 25 Nov 2015 13:02:12 +0000
From:	Felipe Ferreri Tonello <eu@...ipetonello.com>
To:	Robert Baldyga <r.baldyga@...sung.com>, linux-usb@...r.kernel.org
Cc:	linux-kernel@...r.kernel.org, Felipe Balbi <balbi@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Clemens Ladisch <clemens@...isch.de>
Subject: Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue
 out requests

Hi Robert,

On 16/11/15 11:43, Robert Baldyga wrote:
> On 11/16/2015 12:08 PM, Felipe Ferreri Tonello wrote:
>> Hi Robert,
>>
>> On 13/11/15 08:31, Robert Baldyga wrote:
>>> Hi Felipe,
>>>
>>> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>>>> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
>>>> the request. If that happens the complete function will never be called, thus
>>>> never freeing the request.
>>>>
>>>> Signed-off-by: Felipe F. Tonello <eu@...ipetonello.com>
>>>> ---
>>>>  drivers/usb/gadget/function/f_midi.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>>> index f36db2d..76ea53c 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>>>  		if (err) {
>>>>  			ERROR(midi, "%s queue req: %d\n",
>>>>  				    midi->out_ep->name, err);
>>>> +			free_ep_req(midi->out_ep, req);
>>>>  		}
>>>>  	}
>>>>  
>>>>
>>>
>>> There is one more thing I haven't noticed before. We can have situation
>>> when all requests were allocated successfully, but their allocation
>>> failed. What we get then is set_alt() returning 0, while no request is
>>> allocated, hence the function is, in fact, inactive.
>>
>> Right. So in this case should we return some error? We can restrict the
>> function to work iff allocates the 'qlen' number of allocations,
>> otherwise returns an error and frees all other requests (IN and OUT).
>>
> 
> Yes, IMO it's a proper solution. When user sets qlen to given value he
> expects that exact number of requests to be allocated and enqueued, and
> if we cannot do that we should consider this as an error.
> 

Ok. I will do that in a new patch on v6 since this patch is correct
anyway and Blabi already applied to his test branch.

Felipe

Download attachment "0x92698E6A.asc" of type "application/pgp-keys" (4831 bytes)