Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2736 bytes --]

Hi Avichal

On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>On 9/10/23 17:24, Michael Grzeschik wrote:
>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>> immediately deallocs all requests on its disable codepath. This is not
>> save since the dequeue function is async and does not ensure that the
>> requests are left unlinked in the controller driver.
>>
>> By adding the ep_free_request into the completion path of the requests
>> we ensure that the request will be properly deallocated.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> index 4b6e854e30c58c..52e3666b51f743 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>  	struct uvc_device *uvc = video->uvc;
>>  	unsigned long flags;
>>
>> +	if (uvc->state == UVC_STATE_CONNECTED) {
>> +		usb_ep_free_request(video->ep, ureq->req);
>nit: You can probably just call usb_ep_free_request with req instead of ureq->req.

Thanks, thats a good point.

>> +		ureq->req = NULL;
>> +		return;
>> +	}
>> +
>>  	switch (req->status) {
>>  	case 0:
>>  		break;
>
>Perhaps I am missing something here, but I am not sure how this alone
>fixes the use-after-free issue. uvcg_video_enable still deallocates
>_all_ usb_requests right after calling usb_ep_dequeue, so it is still
>possible that an unreturned request is deallocated, and now it is
>possible that the complete callback accesses a deallocated ureq :(

Since the issue I saw was usually coming from the list_del_entry_valid check in
the list_del_entry of the giveback function, the issue was probably just not
triggered anymore as the complete function did exit early.

So this fix alone is actually bogus without a second patch I had in the stack.
The second patch I am refering should change the actual overall issue:

https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u

This early list_del and this patch here should ensure that the
concurrent functions are not handling already freed memory.

Thanks,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]