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

[sashiko-bot@kernel.org]

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