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: <20251022124937.GE727@pendragon.ideasonboard.com>
Date: Wed, 22 Oct 2025 15:49:37 +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 02:29:15PM +0200, Ricardo Ribalda wrote:
> On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart wrote:
> > 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);
> > > +

Another comment, I would have done this in the (single) caller, to keep
operating on real ids in this function. Or we could pass a struct
uvc_entity instead of an int id and rename the function to
uvc_stream_for_terminal(), which could better encapsulate the purpose.

> > >       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 it is OK with you I'd rather keep the dev_err(). If an ISP
> manufacturer tests their camera in Linux I want them to really notice
> that there is an error.

Yes I'm OK with that. It shouldn't happen. We want a dev_err_and_blame()
that prints a message to the kernel log and posts messages on social
networks to blame the hardware manufacturer.

> Besides that, I have implemented all your proposed changes.
> 
> I cannot test it until tomorrow in real hardware. But the changes are
> trivial, let me know if I shall send the v2 right now or wait til it
> is tested.

Up to you, I don't mind either way.

If we merge "[PATCH v2] media: uvcvideo: Use heuristic to find stream
entity" first, do you plan to revert it to get this patch merged ?

> > > +
> > > +     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