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

[Avichal Rakesh <arakesh@google.com>]

Hey Michael,

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.
> +		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 :(

Regards,
Avi