Re: [PATCH v2 2/2] usb: gadget: uvc: refactor video cleanup into uvcg_video_deinit()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: gadget: uvc: missing mutex in unbind causes UAF in uvcg_video_deinit()
--

commit 5b750200592f725e55689646ba5d3a79912818ba
Author: Xu Yang <xu.yang_2@nxp.com>

usb: gadget: uvc: refactor video cleanup into uvcg_video_deinit()

This commit consolidates the teardown logic for the UVC gadget's async
workqueue and kworker into a new uvcg_video_deinit() helper to prevent
resource leaks during bind failures and unbind. It also changes the unbind
sequence to tear down these resources after unregistering the V4L2 device.

> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index a4fb2790f4ffe..fa2f9d0e4ce0b 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
[ ... ]
> @@ -1048,6 +1044,7 @@ static void uvc_function_unbind(struct usb_configuration *c,
>  
>  	device_remove_file(&uvc->vdev.dev, &dev_attr_function_name);
>  	video_unregister_device(&uvc->vdev);
> +	uvcg_video_deinit(video);

[Severity: High]
Can this cause a use-after-free if an ioctl is currently executing?

When uvc_function_unbind() calls video_unregister_device(), it clears
the registered flag but does not wait for active ioctls to complete. Since
uvc_function_unbind() does not hold video->mutex, an in-flight
VIDIOC_STREAMON ioctl could concurrently be executing uvcg_video_enable().

Meanwhile, uvc_function_unbind() proceeds to call uvcg_video_deinit()
without any locks.

>  	v4l2_device_unregister(&uvc->v4l2_dev);
>  
>  	scoped_guard(mutex, &uvc->lock)
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 9ba09118bb74a..002afca9141ee 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
[ ... ]
> @@ -841,3 +841,18 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>  	return uvcg_queue_init(&video->queue, uvc->v4l2_dev.dev->parent,
>  			V4L2_BUF_TYPE_VIDEO_OUTPUT, &video->mutex);
>  }
> +
> +void uvcg_video_deinit(struct uvc_video *video)
> +{
> +	kthread_cancel_work_sync(&video->hw_submit);
> +
> +	if (!IS_ERR_OR_NULL(video->kworker)) {
> +		kthread_destroy_worker(video->kworker);
> +		video->kworker = NULL;
> +	}

[Severity: High]
Does destroying the kworker here locklessly open a race window?

If uvcg_video_enable() is executing concurrently under video->mutex, it calls
kthread_queue_work(video->kworker, &video->hw_submit).

If uvcg_video_deinit() frees video->kworker before the ioctl finishes,
will uvcg_video_enable() dereference freed memory and crash the kernel?

> +
> +	if (video->async_wq) {
> +		destroy_workqueue(video->async_wq);
> +		video->async_wq = NULL;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-usb-uvc-fixes-v2-0-844e04a4a4b7@nxp.com?part=2