| 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
| ||
|
Message-ID: <CANiDSCuex8w8GvMuKMyZw5sBCeW0wLteRJy97LG5Z_TDbWZ71w@mail.gmail.com>
Date: Thu, 23 Oct 2025 13:47:39 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
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 Thu, 23 Oct 2025 at 13:25, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Wed, Oct 22, 2025 at 03:14:09PM +0200, Ricardo Ribalda wrote:
> > On Wed, 22 Oct 2025 at 15:12, Laurent Pinchart wrote:
> > > On Wed, Oct 22, 2025 at 03:08:58PM +0200, Ricardo Ribalda wrote:
> > > > On Wed, 22 Oct 2025 at 14:49, Laurent Pinchart wrote:
> > > > > 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.
> > > >
> > > > Like the second option better.
> > >
> > > I think I do too.
> > >
> > > > > > > > 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 ?
> > > >
> > > > I think they solve two different issues:
> > > >
> > > > - Output terminal id collides with another entity id.
> > > > - Incorrect bTerminalLink
> > >
> > > Do we know of any device affected by that issue ?
> >
> > I bet you there is one :)
>
> I'd rather be cautious and address that if the issue arises. Enabling
> non-compliant behaviour has the drawback of making issues less visible
> to vendors, so I would prefer not working around problems unless we know
> they exist.
My main goal right now is to solve the regression.
If you think that this approach is best, please add your review-by and
the following tags to the patch. And let's start landing into upstream
asap.
We can discuss later if we want an extra patch or if we want to wait
for another report.
Reported-by: Angel4005 <ooara1337@...il.com>
Closes: https://lore.kernel.org/linux-media/CAOzBiVuS7ygUjjhCbyWg-KiNx+HFTYnqH5+GJhd6cYsNLT=DaA@mail.gmail.com/
Fixes: 0e2ee70291e6 ("media: uvcvideo: Mark invalid entities with id
UVC_INVALID_ENTITY_ID")
Regards!
>
> > > > We can have the two patches in. If there is any conflict because we
> > > > land one and then the other I can send a v3 fixing the conflict. Or a
> > > > maintainer can do that, they should be trivial conflicts.
> > > >
> > > > > > > > +
> > > > > > > > + 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
--
Ricardo Ribalda
Powered by blists - more mailing lists