linux-kernel - Re: [PATCH v5 2/4] media: Documentation: Add note about UVCH length field

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] [day] [month] [year] [list]

Message-ID: <CANiDSCvyBW2Zq6o2hbwQBOEPukryAgkqnfQhk7=zrdi7K5fF2g@mail.gmail.com>
Date: Mon, 26 May 2025 16:20:44 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hdegoede@...hat.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/4] media: Documentation: Add note about UVCH length field

On Mon, 26 May 2025 at 16:13, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Mon, May 26, 2025 at 04:04:03PM +0200, Ricardo Ribalda wrote:
> > On Mon, 26 May 2025 at 15:49, Laurent Pinchart wrote:
> > > On Fri, Apr 04, 2025 at 06:37:35AM +0000, Ricardo Ribalda wrote:
> > > > The documentation currently describes the UVC length field as the "length
> > > > of the rest of the block", which can be misleading. The driver limits the
> > > > data copied to a maximum of 12 bytes.
> > > >
> > > > This change adds a clarifying sentence to the documentation to make this
> > > > restriction explicit.
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > ---
> > > >  Documentation/userspace-api/media/v4l/metafmt-uvc.rst | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/userspace-api/media/v4l/metafmt-uvc.rst b/Documentation/userspace-api/media/v4l/metafmt-uvc.rst
> > > > index 784346d14bbdbf28348262084d5b0646d30bd1da..42599875331c0066cf529153caccb731148023b9 100644
> > > > --- a/Documentation/userspace-api/media/v4l/metafmt-uvc.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/metafmt-uvc.rst
> > > > @@ -44,7 +44,9 @@ Each individual block contains the following fields:
> > > >          them
> > > >      * - :cspan:`1` *The rest is an exact copy of the UVC payload header:*
> > > >      * - __u8 length;
> > > > -      - length of the rest of the block, including this field
> > > > +      - length of the rest of the block, including this field. Please note that
> > > > +        regardless of the this value, for V4L2_META_FMT_UVC the kernel will
> > >
> > > "the this value" looks like a typo.
> >
> > Thanks! Will fix in v2
> >
> > >
> > > > +        never copy more than 2-12 bytes.
> > >
> > > Are you saying here that length can be larger than 12, but only up to 12
> > > bytes will be copied (when both SCR and PTS are present) ? If that's the
> > > case, it would be better to fix the driver to clamp the length value to
> > > the number of bytes actually present in the buffer.
> >
> > As the documentation says, this is an exact copy of the UVC payload header.
> >
> > Assuming SCR and PTS, for devices that have metadata length will be
> > the real length provided by the hardware. but buf[] will only contain
> > 12 bytes.
> >
> > Replacing the value of length with the actual value will be a uAPI
> > breakage. I do not think that is a very good idea to change it,
> > considering that this number is used by parsers.
>
> Do you think there could be userspace code that relies on the value
> being larger than 12, even though the metadata after the standard UVC
> block isn't present in the buffer ? Are you aware of any particular
> implementation of such userspace code ?

Userspace code can use a value bigger than 12 to know if the actual
metadata has been enabled or not.
I have been using that to test my code.  I would not be surprised if
there are more userspace implementations like mine.

There is no reason to break uAPI, especially when it is documented to
behave like that.

This patch is just a clarification of the documentation. I would have
loved to have that clarification when I started working on metadata.

>
> > > >      * - __u8 flags;
> > > >        - Flags, indicating presence of other standard UVC fields
> > > >      * - __u8 buf[];
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda