linux-kernel - Re: [PATCH] media: uvcvideo: Create a specific id namespace for output entities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251022120849.GD727@pendragon.ideasonboard.com>
Date: Wed, 22 Oct 2025 15:08:49 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Hans de Goede <hansg@...nel.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: uvcvideo: Create a specific id namespace for
 output entities

On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> Nothing can be connected from an output entity. Which means that no

s/output entity/output terminal. Same below.

Did you mean s/from an/to an/ ?

> other entity can reference an output entity as baSourceId.
> 

Some output terminals have controls, so we need to preserve their ID.
That's why my proposal only set the UVC_TERM_OUTPUT bit for the
*streaming* output terminals, not for all output terminals.

> Use this fact to move all the output entities to a different namespace
> id.
> 
> The output entities are usually named after the dev_name() of the usb
> device, so there should not be any uAPI change from this change.
> 
> Although with this change we can handle some id collisions
> automagically, change the logic of uvc_alloc_new_entity() to keep
> showing a warning when a camera has invalid descriptors. Hopefully this
> message will help vendors fix their invalid descriptors.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
> Hi, this patch fixes support for some devices with invalid USB
> descriptor.
> 
> It is orthogonal to:
> https://lore.kernel.org/linux-media/20251021184213.GC19043@pendragon.ideasonboard.com/T/#t
> 
> Some devices will be fixed by the other patch, other devices will be
> fixed by this. In my opinion is worth to land both patches.
> 
> Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
>  	return NULL;
>  }
>  
> +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)

This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
also & 0xff, as the UVC descriptors store IDs in 8-bit fields.

> +
>  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
>  {
>  	struct uvc_streaming *stream;
>  
> +	id = ENTITY_HARDWARE_ID(id);
> +
>  	list_for_each_entry(stream, &dev->streams, list) {
>  		if (stream->header.bTerminalLink == id)
>  			return stream;
> @@ -810,10 +814,12 @@ static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
>  	}
>  
>  	/* Per UVC 1.1+ spec 3.7.2, the ID is unique. */
> -	if (uvc_entity_by_id(dev, id)) {
> -		dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n", id);
> +	if (uvc_entity_by_id(dev, ENTITY_HARDWARE_ID(id)))
> +		dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> +			ENTITY_HARDWARE_ID(id));

It's not an error anymore if there's no collision of the full 16-bit ID,
right ? Should it be demoted to a dev_warn() ?

> +
> +	if (uvc_entity_by_id(dev, id))
>  		id = UVC_INVALID_ENTITY_ID;
> -	}
>  
>  	extra_size = roundup(extra_size, sizeof(*entity->pads));
>  	if (num_pads)
> @@ -969,6 +975,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  	struct usb_host_interface *alts = dev->intf->cur_altsetting;
>  	unsigned int i, n, p, len;
>  	const char *type_name;
> +	unsigned int id;
>  	u16 type;
>  
>  	switch (buffer[2]) {
> @@ -1107,8 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
>  			return 0;
>  		}
>  
> +		/*
> +		 * Nothing can be connected from an output terminal. To avoid
> +		 * entity-id's collisions in devices with invalid USB
> +		 * descriptors, move the output terminal id to its own
> +		 * namespace.
> +		 */
> +		id = buffer[3] | UVC_TERM_OUTPUT;
> +
>  		term = uvc_alloc_new_entity(dev, type | UVC_TERM_OUTPUT,
> -					    buffer[3], 1, 0);
> +					    id, 1, 0);
>  		if (IS_ERR(term))
>  			return PTR_ERR(term);
>  
> 
> ---
> base-commit: ea299a2164262ff787c9d33f46049acccd120672
> change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b

-- 
Regards,

Laurent Pinchart