[PATCH 0/2] usb: gadget: uvc: fix resource leak on video->async_wq and video->kworker

[PATCH 0/2] usb: gadget: uvc: fix resource leak on video->async_wq and video->kworker

[Xu Yang]

This patchset fixes the resource leak issue on uvc_function_bind() error
handling path and uvc_function_unbind(). For the former, video->async_wq
and video->kworker may never be destroyed. For the latter, video->kworker
needs to be destroyed.

Besides, patch#1 also fixes incorrect goto labels that could clean up
resources not yet initialized.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
Xu Yang (2):
      usb: gadget: uvc: replace mutex lock/unlock with scoped_guard in uvc_function_bind()
      usb: gadget: uvc: refactor video cleanup into uvcg_video_deinit()

 drivers/usb/gadget/function/f_uvc.c     | 87 ++++++++++++++-------------------
 drivers/usb/gadget/function/uvc_video.c | 15 ++++++
 drivers/usb/gadget/function/uvc_video.h |  1 +
 3 files changed, 54 insertions(+), 49 deletions(-)
---
base-commit: e1e6e541c5c9cf548e9fdc35fc26808c82074440
change-id: 20260819-usb-uvc-fixes-3176195c148a

Best regards,
--  
Xu Yang <xu.yang_2@nxp.com>

[PATCH 1/2] usb: gadget: uvc: replace mutex lock/unlock with scoped_guard in uvc_function_bind()

[Xu Yang]

From: Xu Yang <xu.yang_2@nxp.com>

Commit 68aa70648b62 ("usb: gadget: uvc: hold opts->lock across XU walks
in uvc_function_bind") introduced an error_unlock label to release
opts->lock on failure paths. The label is misplaced between the return
statement and v4l2_error, causing it to fall through into v4l2_error
and call v4l2_device_unregister() on a device that was never registered.

Replace the manual mutex_lock/unlock pair and the error_unlock label
with scoped_guard(mutex), removing the need for explicit lock cleanup on
error paths.

Fixes: 68aa70648b62 ("usb: gadget: uvc: hold opts->lock across XU walks in uvc_function_bind")
Assisted-by: Claude:claude-sonnet-4.6
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/gadget/function/f_uvc.c | 80 +++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index d1bf3ea75197..a4fb2790f4ff 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -768,23 +768,17 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 	uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
 	uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
 
-	/*
-	 * Hold opts->lock across both the XU string-descriptor fixup below and
-	 * the descriptor-copy block further down.  Without this, configfs
-	 * uvcg_extension_drop() (which takes opts->lock) can race with the
-	 * list_for_each_entry() walks here and inside uvc_copy_descriptors(),
-	 * leading to a UAF on a freed struct uvcg_extension.  See
-	 * drivers/usb/gadget/function/uvc_configfs.c::uvcg_extension_drop().
-	 */
-	mutex_lock(&opts->lock);
-
 	/*
 	 * XUs can have an arbitrary string descriptor describing them. If they
-	 * have one pick up the ID.
+	 * have one pick up the ID. Hold opts->lock here to avoid race with configfs
+	 * uvcg_extension_make() and uvcg_extension_drop().
 	 */
-	list_for_each_entry(xu, &opts->extension_units, list)
-		if (xu->string_descriptor_index)
-			xu->desc.iExtension = cdev->usb_strings[xu->string_descriptor_index].id;
+	scoped_guard(mutex, &opts->lock) {
+		list_for_each_entry(xu, &opts->extension_units, list)
+			if (xu->string_descriptor_index)
+				xu->desc.iExtension =
+					cdev->usb_strings[xu->string_descriptor_index].id;
+	}
 
 	/*
 	 * We attach the hard-coded defaults incase the user does not provide
@@ -795,7 +789,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 				 ARRAY_SIZE(uvc_en_us_strings));
 	if (IS_ERR(us)) {
 		ret = PTR_ERR(us);
-		goto error_unlock;
+		goto error;
 	}
 
 	uvc_iad.iFunction = opts->iad_index ? cdev->usb_strings[opts->iad_index].id :
@@ -809,50 +803,50 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 
 	/* Allocate interface IDs. */
 	if ((ret = usb_interface_id(c, f)) < 0)
-		goto error_unlock;
+		goto error;
 	uvc_iad.bFirstInterface = ret;
 	uvc_control_intf.bInterfaceNumber = ret;
 	uvc->control_intf = ret;
 	opts->control_interface = ret;
 
 	if ((ret = usb_interface_id(c, f)) < 0)
-		goto error_unlock;
+		goto error;
 	uvc_streaming_intf_alt0.bInterfaceNumber = ret;
 	uvc_streaming_intf_alt1.bInterfaceNumber = ret;
 	uvc->streaming_intf = ret;
 	opts->streaming_interface = ret;
 
 	/* Copy descriptors */
-	f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
-	if (IS_ERR(f->fs_descriptors)) {
-		ret = PTR_ERR(f->fs_descriptors);
-		f->fs_descriptors = NULL;
-		goto error_unlock;
-	}
+	scoped_guard(mutex, &opts->lock) {
+		f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
+		if (IS_ERR(f->fs_descriptors)) {
+			ret = PTR_ERR(f->fs_descriptors);
+			f->fs_descriptors = NULL;
+			goto error;
+		}
 
-	f->hs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_HIGH);
-	if (IS_ERR(f->hs_descriptors)) {
-		ret = PTR_ERR(f->hs_descriptors);
-		f->hs_descriptors = NULL;
-		goto error_unlock;
-	}
+		f->hs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_HIGH);
+		if (IS_ERR(f->hs_descriptors)) {
+			ret = PTR_ERR(f->hs_descriptors);
+			f->hs_descriptors = NULL;
+			goto error;
+		}
 
-	f->ss_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER);
-	if (IS_ERR(f->ss_descriptors)) {
-		ret = PTR_ERR(f->ss_descriptors);
-		f->ss_descriptors = NULL;
-		goto error_unlock;
-	}
+		f->ss_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER);
+		if (IS_ERR(f->ss_descriptors)) {
+			ret = PTR_ERR(f->ss_descriptors);
+			f->ss_descriptors = NULL;
+			goto error;
+		}
 
-	f->ssp_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER_PLUS);
-	if (IS_ERR(f->ssp_descriptors)) {
-		ret = PTR_ERR(f->ssp_descriptors);
-		f->ssp_descriptors = NULL;
-		goto error_unlock;
+		f->ssp_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER_PLUS);
+		if (IS_ERR(f->ssp_descriptors)) {
+			ret = PTR_ERR(f->ssp_descriptors);
+			f->ssp_descriptors = NULL;
+			goto error;
+		}
 	}
 
-	mutex_unlock(&opts->lock);
-
 	/* Preallocate control endpoint request. */
 	uvc->control_req = usb_ep_alloc_request(cdev->gadget->ep0, GFP_KERNEL);
 	uvc->control_buf = kmalloc(UVC_MAX_REQUEST_SIZE, GFP_KERNEL);
@@ -884,8 +878,6 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 
 	return 0;
 
-error_unlock:
-	mutex_unlock(&opts->lock);
 v4l2_error:
 	v4l2_device_unregister(&uvc->v4l2_dev);
 error:

-- 
2.34.1

Re: [PATCH 1/2] usb: gadget: uvc: replace mutex lock/unlock with scoped_guard in uvc_function_bind()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Medium] The function mixes `goto`-based error handling with scope-based cleanup (`scoped_guard`), violating subsystem rules.

Pre-existing issues:
- [High] `uvc_function_bind()` fails to catch OOM conditions from `uvc_copy_descriptors()` because it checks for `IS_ERR()` instead of `NULL`.
- [High] Integer overflow in `xu->desc.bLength` calculation leads to under-allocation and an OOB write in `uvc_copy_descriptors()`.
- [High] If `v4l2_device_register()` fails, `uvc_function_bind()` returns a positive success code despite freeing its resources, leading to a Use-After-Free.
- [High] Concurrent modification of statically allocated global string array `uvc_en_us_strings` leads to a dangling pointer and Use-After-Free.
--

--- Patch [1]: [PATCH 1/2] usb: gadget: uvc: replace mutex lock/unlock with scoped_guard in uvc_function_bind() ---
commit ab6a04018ad2f28e486890d53690fb679bf7a633
Author: Xu Yang <xu.yang_2@nxp.com>

usb: gadget: uvc: replace mutex lock/unlock with scoped_guard in uvc_function_bind()

This patch replaces the manual mutex_lock and error_unlock label with
scoped_guard(mutex) in uvc_function_bind(). This removes the need for
explicit lock cleanup on error paths.

> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -768,23 +768,17 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)

[ ... ]

>  	/*
>  	 * We attach the hard-coded defaults incase the user does not provide
> @@ -795,7 +789,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>  				 ARRAY_SIZE(uvc_en_us_strings));
>  	if (IS_ERR(us)) {
>  		ret = PTR_ERR(us);
> -		goto error_unlock;
> +		goto error;

[Severity: Medium]
Does replacing this manual lock with scoped_guard while keeping the goto
labels violate the subsystem cleanup rules?

The cleanup guidelines state that usage of goto and cleanup helpers should
never be mixed in the same function.

Should all resources in uvc_function_bind() be converted to
scope-based cleanup, or should this function continue using manual
locking instead?

>  	}
>  
>  	uvc_iad.iFunction = opts->iad_index ? cdev->usb_strings[opts->iad_index].id :

[ ... ]

> @@ -809,50 +803,50 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  	/* Allocate interface IDs. */
>  	if ((ret = usb_interface_id(c, f)) < 0)
> -		goto error_unlock;
> +		goto error;
>  	uvc_iad.bFirstInterface = ret;
>  	uvc_control_intf.bInterfaceNumber = ret;
>  	uvc->control_intf = ret;
>  	opts->control_interface = ret;

[Severity: High]
This is a pre-existing issue, but can modifying the global statically
allocated uvc_en_us_strings array during initialization lead to a dangling
pointer?

Earlier in this function, the array is modified:

drivers/usb/gadget/function/f_uvc.c:uvc_function_bind() {
    ...
    uvc_en_us_strings[UVC_STRING_CONTROL_IDX].s = opts->function_name;
    us = usb_gstrings_attach(cdev, uvc_function_strings,
                             ARRAY_SIZE(uvc_en_us_strings));
    ...
}

If two gadgets bind concurrently, they could race to write their
per-instance opts->function_name pointer into the global array at
uvc_en_us_strings[UVC_STRING_CONTROL_IDX].s.

usb_gstrings_attach() then copies this pointer. If Gadget B receives a
pointer to Gadget A's string, and Gadget A is unbound and freed, would
the host dereference a freed pointer when requesting the string
descriptor from Gadget B?

>  
>  	if ((ret = usb_interface_id(c, f)) < 0)
> -		goto error_unlock;
> +		goto error;
>  	uvc_streaming_intf_alt0.bInterfaceNumber = ret;
>  	uvc_streaming_intf_alt1.bInterfaceNumber = ret;
>  	uvc->streaming_intf = ret;
>  	opts->streaming_interface = ret;
>  
>  	/* Copy descriptors */
> -	f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> -	if (IS_ERR(f->fs_descriptors)) {
> -		ret = PTR_ERR(f->fs_descriptors);
> -		f->fs_descriptors = NULL;
> -		goto error_unlock;
> -	}
> +	scoped_guard(mutex, &opts->lock) {
> +		f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> +		if (IS_ERR(f->fs_descriptors)) {

[Severity: High]
This is a pre-existing issue, but does checking IS_ERR() here fail
to catch out-of-memory conditions?

In uvc_copy_descriptors(), if kmalloc() fails it returns NULL:

drivers/usb/gadget/function/f_uvc.c:uvc_copy_descriptors() {
    ...
    mem = kmalloc((n_desc + 1) * sizeof(*src) + bytes, GFP_KERNEL);
    if (mem == NULL)
        return NULL;
    ...
}

Since IS_ERR(NULL) evaluates to false, the OOM condition is missed,
and the gadget setup proceeds with a NULL descriptor array. Should
this check IS_ERR_OR_NULL() instead?

[Severity: High]
This is a pre-existing issue, but could an integer overflow in the
extension unit size lead to an out-of-bounds write inside
uvc_copy_descriptors()?

If an attacker configures an extension unit with large bNrInPins and
bControlSize via configfs, the u8 bLength calculation wraps around:

drivers/usb/gadget/function/uvc_configfs.c:uvcg_extension_b_control_size_store() {
    xu->desc.bLength = UVC_DT_EXTENSION_UNIT_SIZE(xu->desc.bNrInPins, xu->desc.bControlSize);
}

Then uvc_copy_descriptors() uses this wrapped bLength to size the kmalloc
buffer, but UVC_COPY_XU_DESCRIPTOR() copies the full unpacked size.

Could this result in a heap buffer overflow?

> +			ret = PTR_ERR(f->fs_descriptors);
> +			f->fs_descriptors = NULL;
> +			goto error;
> +		}

[ ... ]

> @@ -884,8 +878,6 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  	return 0;
>  
> -error_unlock:
> -	mutex_unlock(&opts->lock);
>  v4l2_error:
>  	v4l2_device_unregister(&uvc->v4l2_dev);
>  error:

[Severity: High]
This is a pre-existing issue, but if v4l2_device_register() fails earlier
in uvc_function_bind(), does it return a positive success code despite
jumping to this error path?

drivers/usb/gadget/function/f_uvc.c:uvc_function_bind() {
    ...
    if (v4l2_device_register(&cdev->gadget->dev, &uvc->v4l2_dev)) {
        uvcg_err(f, "failed to register V4L2 device\n");
        goto error;
    }
    ...
}

At this point, ret holds a positive interface ID from an earlier
usb_interface_id() call. 

Since ret is not updated to a negative error code, the framework
treats the positive return as a success. When the host later interacts
with the gadget, will it access the freed uvc->control_req and
uvc->control_buf buffers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821-usb-uvc-fixes-v1-0-80e6e279523d@nxp.com?part=1

Re: [PATCH 1/2] usb: gadget: uvc: replace mutex lock/unlock with scoped_guard in uvc_function_bind()

[Frank Li]

On Fri, Aug 21, 2026 at 04:22:36PM +0800, Xu Yang wrote:
> From: Xu Yang <xu.yang_2@nxp.com>
>
> Commit 68aa70648b62 ("usb: gadget: uvc: hold opts->lock across XU walks
> in uvc_function_bind") introduced an error_unlock label to release
> opts->lock on failure paths. The label is misplaced between the return
> statement and v4l2_error, causing it to fall through into v4l2_error
> and call v4l2_device_unregister() on a device that was never registered.
>
> Replace the manual mutex_lock/unlock pair and the error_unlock label
> with scoped_guard(mutex), removing the need for explicit lock cleanup on
> error paths.
>
> Fixes: 68aa70648b62 ("usb: gadget: uvc: hold opts->lock across XU walks in uvc_function_bind")
> Assisted-by: Claude:claude-sonnet-4.6
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/usb/gadget/function/f_uvc.c | 80 +++++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index d1bf3ea75197..a4fb2790f4ff 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -768,23 +768,17 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>  	uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
>  	uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
>
> -	/*
> -	 * Hold opts->lock across both the XU string-descriptor fixup below and
> -	 * the descriptor-copy block further down.  Without this, configfs
> -	 * uvcg_extension_drop() (which takes opts->lock) can race with the
> -	 * list_for_each_entry() walks here and inside uvc_copy_descriptors(),
> -	 * leading to a UAF on a freed struct uvcg_extension.  See
> -	 * drivers/usb/gadget/function/uvc_configfs.c::uvcg_extension_drop().
> -	 */
> -	mutex_lock(&opts->lock);
> -
>  	/*
>  	 * XUs can have an arbitrary string descriptor describing them. If they
> -	 * have one pick up the ID.
> +	 * have one pick up the ID. Hold opts->lock here to avoid race with configfs
> +	 * uvcg_extension_make() and uvcg_extension_drop().
>  	 */
> -	list_for_each_entry(xu, &opts->extension_units, list)
> -		if (xu->string_descriptor_index)
> -			xu->desc.iExtension = cdev->usb_strings[xu->string_descriptor_index].id;
> +	scoped_guard(mutex, &opts->lock) {
> +		list_for_each_entry(xu, &opts->extension_units, list)
> +			if (xu->string_descriptor_index)
> +				xu->desc.iExtension =
> +					cdev->usb_strings[xu->string_descriptor_index].id;
> +	}
>
>  	/*
>  	 * We attach the hard-coded defaults incase the user does not provide
> @@ -795,7 +789,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>  				 ARRAY_SIZE(uvc_en_us_strings));
>  	if (IS_ERR(us)) {
>  		ret = PTR_ERR(us);
> -		goto error_unlock;
> +		goto error;
>  	}
>
>  	uvc_iad.iFunction = opts->iad_index ? cdev->usb_strings[opts->iad_index].id :
> @@ -809,50 +803,50 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>
>  	/* Allocate interface IDs. */
>  	if ((ret = usb_interface_id(c, f)) < 0)
> -		goto error_unlock;
> +		goto error;
>  	uvc_iad.bFirstInterface = ret;
>  	uvc_control_intf.bInterfaceNumber = ret;
>  	uvc->control_intf = ret;
>  	opts->control_interface = ret;
>
>  	if ((ret = usb_interface_id(c, f)) < 0)
> -		goto error_unlock;
> +		goto error;
>  	uvc_streaming_intf_alt0.bInterfaceNumber = ret;
>  	uvc_streaming_intf_alt1.bInterfaceNumber = ret;
>  	uvc->streaming_intf = ret;
>  	opts->streaming_interface = ret;
>
>  	/* Copy descriptors */
> -	f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> -	if (IS_ERR(f->fs_descriptors)) {
> -		ret = PTR_ERR(f->fs_descriptors);
> -		f->fs_descriptors = NULL;
> -		goto error_unlock;
> -	}
> +	scoped_guard(mutex, &opts->lock) {
> +		f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> +		if (IS_ERR(f->fs_descriptors)) {
> +			ret = PTR_ERR(f->fs_descriptors);
> +			f->fs_descriptors = NULL;
> +			goto error;
> +		}
>
> -	f->hs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_HIGH);
> -	if (IS_ERR(f->hs_descriptors)) {
> -		ret = PTR_ERR(f->hs_descriptors);
> -		f->hs_descriptors = NULL;
> -		goto error_unlock;
> -	}
> +		f->hs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_HIGH);
> +		if (IS_ERR(f->hs_descriptors)) {
> +			ret = PTR_ERR(f->hs_descriptors);
> +			f->hs_descriptors = NULL;
> +			goto error;
> +		}
>
> -	f->ss_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER);
> -	if (IS_ERR(f->ss_descriptors)) {
> -		ret = PTR_ERR(f->ss_descriptors);
> -		f->ss_descriptors = NULL;
> -		goto error_unlock;
> -	}
> +		f->ss_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER);
> +		if (IS_ERR(f->ss_descriptors)) {
> +			ret = PTR_ERR(f->ss_descriptors);
> +			f->ss_descriptors = NULL;
> +			goto error;
> +		}
>
> -	f->ssp_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER_PLUS);
> -	if (IS_ERR(f->ssp_descriptors)) {
> -		ret = PTR_ERR(f->ssp_descriptors);
> -		f->ssp_descriptors = NULL;
> -		goto error_unlock;
> +		f->ssp_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER_PLUS);
> +		if (IS_ERR(f->ssp_descriptors)) {
> +			ret = PTR_ERR(f->ssp_descriptors);
> +			f->ssp_descriptors = NULL;
> +			goto error;
> +		}
>  	}
>
> -	mutex_unlock(&opts->lock);
> -
>  	/* Preallocate control endpoint request. */
>  	uvc->control_req = usb_ep_alloc_request(cdev->gadget->ep0, GFP_KERNEL);
>  	uvc->control_buf = kmalloc(UVC_MAX_REQUEST_SIZE, GFP_KERNEL);
> @@ -884,8 +878,6 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>
>  	return 0;
>
> -error_unlock:
> -	mutex_unlock(&opts->lock);
>  v4l2_error:
>  	v4l2_device_unregister(&uvc->v4l2_dev);
>  error:
>
> --
> 2.34.1
>
>

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

[Xu Yang]

From: Xu Yang <xu.yang_2@nxp.com>

kthread_destroy_worker() was never called during unbind, leaving the
UVCG kthread running after the gadget function is unbound. Also, if
uvcg_video_init() or uvc_register_video() fails during bind, async_wq
and kworker were not cleaned up, causing resource leaks.

Both issues require the same teardown sequence: cancel the pending work,
destroy the kworker, and destroy the workqueue. Consolidate this logic
into a new uvcg_video_deinit() helper and call it from both
uvc_function_unbind() and the v4l2_error path in uvc_function_bind().

In uvc_function_unbind(), uvcg_video_deinit() is placed after
video_unregister_device() to fix the ordering. Without this ordering,
tearing down the workers before unregistering the V4L2 device could lead
to use-after-free on video device resources still accessed by those
workers.

Fixes: f0bbfbd16b3b ("usb: gadget: uvc: rework to enqueue in pump worker from encoded queue")
Assisted-by: Claude:claude-sonnet-4.6
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/gadget/function/f_uvc.c     |  7 ++-----
 drivers/usb/gadget/function/uvc_video.c | 15 +++++++++++++++
 drivers/usb/gadget/function/uvc_video.h |  1 +
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index a4fb2790f4ff..fa2f9d0e4ce0 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -879,6 +879,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 	return 0;
 
 v4l2_error:
+	uvcg_video_deinit(&uvc->video);
 	v4l2_device_unregister(&uvc->v4l2_dev);
 error:
 	if (uvc->control_req) {
@@ -1028,11 +1029,6 @@ static void uvc_function_unbind(struct usb_configuration *c,
 		connected = uvc->func_connected;
 	}
 
-	kthread_cancel_work_sync(&video->hw_submit);
-
-	if (video->async_wq)
-		destroy_workqueue(video->async_wq);
-
 	/*
 	 * If we know we're connected via v4l2, then there should be a cleanup
 	 * of the device from userspace either via UVC_EVENT_DISCONNECT or
@@ -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);
 	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 9ba09118bb74..002afca9141e 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;
+	}
+
+	if (video->async_wq) {
+		destroy_workqueue(video->async_wq);
+		video->async_wq = NULL;
+	}
+}
diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
index 8ef6259741f1..6c5481f107f9 100644
--- a/drivers/usb/gadget/function/uvc_video.h
+++ b/drivers/usb/gadget/function/uvc_video.h
@@ -18,5 +18,6 @@ int uvcg_video_enable(struct uvc_video *video);
 int uvcg_video_disable(struct uvc_video *video);
 
 int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc);
+void uvcg_video_deinit(struct uvc_video *video);
 
 #endif /* __UVC_VIDEO_H__ */

-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `uvcg_video_deinit()` in `uvc_function_unbind()` before waiting for V4L2 file release completion leaves it exposed to concurrent in-flight ioctls, causing use-after-free or NULL pointer dereferences.
--

--- Patch [2]: [PATCH 2/2] usb: gadget: uvc: refactor video cleanup into uvcg_video_deinit() ---
commit 4d7b46b3698b9db70da6cefc8230b5f7e4eb66ce
Author: Xu Yang <xu.yang_2@nxp.com>

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

This commit refactors the cleanup logic for UVC video into a new 
uvcg_video_deinit() helper to fix resource leaks in error paths and missing 
kworker destruction during unbind. It also reorders the teardown in 
uvc_function_unbind() to happen after video_unregister_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]
This is a pre-existing issue, but could this ordering lead to a NULL
pointer dereference if there are in-flight ioctls?

If a userspace app is calling an ioctl like VIDIOC_QBUF concurrently while
the gadget is unbound, video_unregister_device() clears the registered
flag but doesn't wait for in-flight ioctls to finish.

Then uvcg_video_deinit() destroys video->async_wq and sets it to NULL:

drivers/usb/gadget/function/uvc_video.c:uvcg_video_deinit() {
	...
	if (video->async_wq) {
		destroy_workqueue(video->async_wq);
		video->async_wq = NULL;
	}
}

An in-flight uvc_v4l2_qbuf() could then resume and try to use the workqueue
by dereferencing the NULL pointer:

drivers/usb/gadget/function/uvc_v4l2.c:uvc_v4l2_qbuf() {
	...
	if (uvc->state == UVC_STATE_STREAMING)
		queue_work(video->async_wq, &video->pump);
}

Should uvcg_video_deinit() be deferred until after 
wait_for_completion(&vdev_release_done) when the video device is fully 
released and no more ioctls can be executing?

>  	v4l2_device_unregister(&uvc->v4l2_dev);
>  
>  	scoped_guard(mutex, &uvc->lock)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821-usb-uvc-fixes-v1-0-80e6e279523d@nxp.com?part=2

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

[Frank Li]

On Fri, Aug 21, 2026 at 04:22:37PM +0800, Xu Yang wrote:
> From: Xu Yang <xu.yang_2@nxp.com>
>
> kthread_destroy_worker() was never called during unbind, leaving the
> UVCG kthread running after the gadget function is unbound. Also, if
> uvcg_video_init() or uvc_register_video() fails during bind, async_wq
> and kworker were not cleaned up, causing resource leaks.
>
> Both issues require the same teardown sequence: cancel the pending work,
> destroy the kworker, and destroy the workqueue. Consolidate this logic
> into a new uvcg_video_deinit() helper and call it from both
> uvc_function_unbind() and the v4l2_error path in uvc_function_bind().
>
> In uvc_function_unbind(), uvcg_video_deinit() is placed after
> video_unregister_device() to fix the ordering. Without this ordering,
> tearing down the workers before unregistering the V4L2 device could lead
> to use-after-free on video device resources still accessed by those
> workers.
>
> Fixes: f0bbfbd16b3b ("usb: gadget: uvc: rework to enqueue in pump worker from encoded queue")
> Assisted-by: Claude:claude-sonnet-4.6
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/usb/gadget/function/f_uvc.c     |  7 ++-----
>  drivers/usb/gadget/function/uvc_video.c | 15 +++++++++++++++
>  drivers/usb/gadget/function/uvc_video.h |  1 +
>  3 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index a4fb2790f4ff..fa2f9d0e4ce0 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -879,6 +879,7 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>  	return 0;
>
>  v4l2_error:
> +	uvcg_video_deinit(&uvc->video);
>  	v4l2_device_unregister(&uvc->v4l2_dev);
>  error:
>  	if (uvc->control_req) {
> @@ -1028,11 +1029,6 @@ static void uvc_function_unbind(struct usb_configuration *c,
>  		connected = uvc->func_connected;
>  	}
>
> -	kthread_cancel_work_sync(&video->hw_submit);
> -
> -	if (video->async_wq)
> -		destroy_workqueue(video->async_wq);
> -
>  	/*
>  	 * If we know we're connected via v4l2, then there should be a cleanup
>  	 * of the device from userspace either via UVC_EVENT_DISCONNECT or
> @@ -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);
>  	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 9ba09118bb74..002afca9141e 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;
> +	}
> +
> +	if (video->async_wq) {
> +		destroy_workqueue(video->async_wq);
> +		video->async_wq = NULL;
> +	}
> +}
> diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
> index 8ef6259741f1..6c5481f107f9 100644
> --- a/drivers/usb/gadget/function/uvc_video.h
> +++ b/drivers/usb/gadget/function/uvc_video.h
> @@ -18,5 +18,6 @@ int uvcg_video_enable(struct uvc_video *video);
>  int uvcg_video_disable(struct uvc_video *video);
>
>  int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc);
> +void uvcg_video_deinit(struct uvc_video *video);
>
>  #endif /* __UVC_VIDEO_H__ */
>
> --
> 2.34.1
>
>