Re: [PATCH 08/19] usb: gadget: uvc: fix oops on a premature STREAMON

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: gadget: uvc: racy and incomplete ep->desc check in uvcg_video_enable()
--

commit c0162971ecaec9497fd415372ef37c6e328eefc7
Author: Josef Bacik <josef@toxicpanda.com>

usb: gadget: uvc: fix oops on a premature STREAMON

This commit adds a NULL check on video->ep->desc in uvcg_video_enable() to
return -ENODEV if userspace issues VIDIOC_STREAMON before the host has
selected the streaming alternate setting.

> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 9ba09118bb74a..2a56536c34817 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -770,6 +770,10 @@ int uvcg_video_enable(struct uvc_video *video)
>  		return -ENODEV;
>  	}
>  
> +	/* Set while the host has selected the streaming alternate setting. */
> +	if (!video->ep->desc)
> +		return -ENODEV;

[Severity: High]
Is checking video->ep->desc without synchronization sufficient here to prevent
races and ensure the endpoint is ready for streaming?

uvc_v4l2_streamon() -> uvcg_video_enable() runs in process context holding
only video->mutex, whereas uvc_function_set_alt() and uvc_function_disable()
in drivers/usb/gadget/function/f_uvc.c run under cdev->lock without
video->mutex:

drivers/usb/gadget/function/f_uvc.c:uvc_function_disable() {
	...
	uvc->state = UVC_STATE_DISCONNECTED;

	usb_ep_disable(uvc->video.ep);
	...
}

drivers/usb/gadget/function/f_uvc.c:uvc_function_set_alt() {
	...
		usb_ep_disable(uvc->video.ep);

		ret = config_ep_by_speed(f->config->cdev->gadget,
				&(uvc->func), uvc->video.ep);
		if (ret)
			return ret;
		usb_ep_enable(uvc->video.ep);

		uvc->video.max_req_size = uvc->video.ep->maxpacket
			* max_t(unsigned int, uvc->video.ep->maxburst, 1)
			* (uvc->video.ep->mult);
	...
}

After uvcg_video_enable() checks video->ep->desc and calls
uvcg_queue_enable(), uvc_video_alloc_requests() -> uvc_video_prep_requests()
in drivers/usb/gadget/function/uvc_video.c reloads and dereferences
video->ep->desc again:

drivers/usb/gadget/function/uvc_video.c:uvc_video_prep_requests() {
	...
	max_req_size = video->max_req_size;

	if (!usb_endpoint_xfer_isoc(video->ep->desc)) {
		video->req_size = max_req_size;
		video->reqs_per_frame = video->uvc_num_requests =
			DIV_ROUND_UP(video->imagesize, max_req_size);

		return;
	}

	interval_duration = 1 << (video->ep->desc->bInterval - 1);
	...
}

Could this leave several issues open:

1. If uvc_function_disable() or uvc_function_set_alt() calls
   usb_ep_disable(uvc->video.ep) between the check in uvcg_video_enable() and
   uvc_video_prep_requests(), can a UDC driver that clears ep->desc on disable
   (such as dwc3) still trigger a NULL pointer dereference in
   uvc_video_prep_requests()?

2. In uvc_function_set_alt(), config_ep_by_speed() sets uvc->video.ep->desc
   before usb_ep_enable() and before uvc->video.max_req_size is calculated
   (and config_ep_by_speed_and_alt() can also fail after assigning _ep->desc).
   If a premature streamon runs in that window while video->max_req_size is
   still 0, can uvc_video_prep_requests() divide by zero on bulk endpoints or
   leave video->req_size as 0 (underflowing video->req_size -
   UVCG_REQUEST_HEADER_LEN in uvc_video_alloc_requests()) on isochronous
   endpoints?

3. Since usb_ep_disable() in drivers/usb/gadget/udc/core.c only clears
   ep->enabled and several UDC drivers (such as dwc2 and cdnsp) do not clear
   ep->desc in their disable callback, won't video->ep->desc remain non-NULL
   after streamoff or disconnect on those controllers, allowing a subsequent
   premature streamon to bypass this check and set uvc->state to
   UVC_STATE_STREAMING while the endpoint is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-llm-usb-v1-0-b9751ddb9c7c@linuxfoundation.org?part=8