| 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: <CANiDSCsC6SgYJ-jSswPaeAw8RkxjZN4n23X35XbntNGcUsuRJQ@mail.gmail.com>
Date: Tue, 21 Oct 2025 18:57:00 +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>,
Hans Verkuil <hverkuil+cisco@...nel.org>,
Thadeu Lima de Souza Cascardo <cascardo@...lia.com>, Angel4005 <ooara1337@...il.com>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: uvcvideo: Use heuristic to find stream entity
Hi Laurent
On Tue, 21 Oct 2025 at 18:44, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Tue, Oct 21, 2025 at 10:36:17AM +0000, Ricardo Ribalda wrote:
> > Some devices, like the Grandstream GUV3100 webcam, have an invalid UVC
> > descriptor where multiple entities share the same ID, this is invalid
> > and makes it impossible to make a proper entity tree without heuristics.
> >
> > We have recently introduced a change in the way that we handle invalid
> > entities that has caused a regression on broken devices.
> >
> > Implement a new heuristic to handle these devices properly.
> >
> > 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")
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> > I have managed to get my hands into a Grandstream GUV3100 and
> > implemented a new heuristics. (Thanks Yunke and Hidenori!).
> >
> > With this heuristics we can use this camera again (see the /dev/video0
> > in the topology).
> >
> > I have tested this change in a 6.6 kernel. Because the notebook that I
> > used for testing has some issues running master. But for the purpose of
> > this change this test should work.
> >
> > ~ # media-ctl --print-topology
> > Media controller API version 6.6.99
> >
> > Media device information
> > ------------------------
> > driver uvcvideo
> > model GRANDSTREAM GUV3100: GRANDSTREA
> > serial
> > bus info usb-0000:00:14.0-9
> > hw revision 0x409
> > driver version 6.6.99
> >
> > Device topology
> > - entity 1: GRANDSTREAM GUV3100: GRANDSTREA (1 pad, 1 link)
> > type Node subtype V4L flags 1
> > device node name /dev/video0
> > pad0: SINK
> > <- "Extension 3":1 [ENABLED,IMMUTABLE]
> >
> > - entity 4: GRANDSTREAM GUV3100: GRANDSTREA (0 pad, 0 link)
> > type Node subtype V4L flags 0
> > device node name /dev/video1
> >
> > - entity 8: Extension 3 (2 pads, 2 links, 0 routes)
> > type V4L2 subdev subtype Unknown flags 0
> > pad0: SINK
> > <- "Processing 2":1 [ENABLED,IMMUTABLE]
> > pad1: SOURCE
> > -> "GRANDSTREAM GUV3100: GRANDSTREA":0 [ENABLED,IMMUTABLE]
> >
> > - entity 11: Processing 2 (2 pads, 3 links, 0 routes)
> > type V4L2 subdev subtype Unknown flags 0
> > pad0: SINK
> > <- "Camera 1":0 [ENABLED,IMMUTABLE]
> > pad1: SOURCE
> > -> "Extension 3":0 [ENABLED,IMMUTABLE]
> > -> "Extension 4":0 [ENABLED,IMMUTABLE]
> >
> > - entity 14: Extension 4 (2 pads, 1 link, 0 routes)
> > type V4L2 subdev subtype Unknown flags 0
> > pad0: SINK
> > <- "Processing 2":1 [ENABLED,IMMUTABLE]
> > pad1: SOURCE
> >
> > - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
> > type V4L2 subdev subtype Sensor flags 0
> > pad0: SOURCE
> > -> "Processing 2":0 [ENABLED,IMMUTABLE]
> > ---
> > Changes in v2:
> > - Fix : invalid reference to the index variable of the iterator.
> > - Link to v1: https://lore.kernel.org/r/20251021-uvc-grandstream-v1-1-801e3d08b271@chromium.org
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..ee4f54d6834962414979a046afc59c5036455124 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -167,13 +167,26 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> >
> > static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> > {
> > - struct uvc_streaming *stream;
> > + struct uvc_streaming *stream, *last_stream;
> > + unsigned int count = 0;
> >
> > list_for_each_entry(stream, &dev->streams, list) {
> > + count += 1;
> > + last_stream = stream;
> > if (stream->header.bTerminalLink == id)
> > return stream;
> > }
> >
> > + /*
> > + * If the streaming entity is referenced by an invalid ID, notify the
> > + * user and use heuristics to guess the correct entity.
> > + */
> > + if (count == 1 && id == UVC_INVALID_ENTITY_ID) {
> > + dev_warn(&dev->intf->dev,
> > + "UVC non compliance: Invalid USB header. The streaming entity has an invalid ID, guessing the correct one.");
> > + return last_stream;
> > + }
>
> As far as I understand, the reason why we can't find the streaming
> interface here is because we have set the streaming output terminal ID
> to UVC_INVALID_ENTITY_ID, due to the extension unit being previously
> registered with the same ID. We're therefore adding a workaround on top
> of another workaround.
>
> Looking at the UVC descriptors for this camera, ID 4 is shared between
> an extension unit with GUID ffe52d21-8030-4e2c-82d9-f587d00540bd and the
> streaming output terminal. That GUID is apparently a Logitech GUID
> (according to https://github.com/soyersoyer/cameractrls/blob/c920e/cameractrls.py).
> I don't know if the XU actually works in that camera, but it could be
> useful to keep it functional.
>
> I think we could make the handling of non-unique IDs more clever. If an
> output streaming terminal and a unit share the same ID, we could
> allocate an unused ID for the output streaming terminal, and patch the
> bTerminalLink of the stream headers accordingly. The ID remapping should
> not cause other issues, as
>
> - streaming output terminals have no controls, so the ID isn't used to
> reference controls
>
> - the units graph is build from sink to source, with UVC descriptors
> containing the IDs of source units, so streaming output terminals are
> never referenced by ID from descriptors except for the bTerminalLink.
>
> This would produce a valid graph without duplicated IDs in this case.
I will try to work on a solution like you propose in this kernel
cycle, but I do not feel very confident about landing that big change
in "fixes" without being in linux-next for a couple of weeks. I'd
rather go for a simple solution that fixes the regression.
With this fix, the media controller topology looks identical as before
we introduce the bug, so we do not lose any functionality.
So if you agree I think the safest path is to land this fix in fixes
now. And try to get a better logic landed in next in this kernel
cycle.
What do you think?
Regards!
>
> > +
> > return NULL;
> > }
> >
> >
> > ---
> > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > change-id: 20251021-uvc-grandstream-05ecf0288f62
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
Powered by blists - more mailing lists