Re: [PATCH] media: uvcvideo: Fix bounds for descriptor parsing

[Ricardo Ribalda]

On Mon, 28 Sept 2026 at 20:44, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Sep 28, 2026 at 04:03:15PM +0200, Ricardo Ribalda wrote:
> > On Mon, 28 Sept 2026 at 14:41, Laurent Pinchart wrote:
> > > On Mon, Sep 28, 2026 at 02:35:29PM +0200, Ricardo Ribalda wrote:
> > > > On Mon, 28 Sept 2026 at 14:24, Laurent Pinchart wrote:
> > > > > On Mon, Sep 28, 2026 at 02:10:26PM +0200, Ricardo Ribalda wrote:
> > > > > > On Mon, 28 Sept 2026 at 14:02, Laurent Pinchart wrote:
> > > > > > > On Fri, Sep 11, 2026 at 01:23:50PM +0000, Ricardo Ribalda wrote:
> > > > > > > > uvc_parse_control() passes descriptor by descriptor to
> > > > > > > > uvc_parse_standard_control() with the number of bytes remaining in the
> > > > > > > > buffer, not the number of bytes of that descriptor.
> > > > > > > >
> > > > > > > > Because of this, malformed descriptors could leak over the next
> > > > > > > > descriptor, leaving malformed data in our structures.
> > > > > > >
> > > > > > > What issue does this fix in practice ?
> > > > > >
> > > > > > Look into uvc_parse_standard_control().
> > > > > > Let's take the UVC_VC_HEADER
> > > > > >
> > > > > > Imagine we have a malformed control at the beginning of the descriptor that has:
> > > > > >
> > > > > > buffer[0] = 3
> > > > > > buffer[2] = UVC_VC_HEADER
> > > > > > buffer[3] => Beggining of next control
> > > > > >
> > > > > > With the current code, buffer[3...X] is parsed as UVC_VC_HEADER.
> > > > > >
> > > > > > What does it fix in practice? It avoids that data outside the controls
> > > > > > is parsed as part of the control.
> > > > >
> > > > > I understand that, but what does it fix in practice ? And have you
> > > > > encountered a device that exhibits such a problem ?
> > > >
> > > > I have not encountered such devices, but even if they do not exist
> > > > they are easy to emulate and use for escalation.
> > >
> > > That's what I'd like to understand, can this be used for any kind of
> > > escalation ?
> >
> > It is very difficult (impossible?) to prove that something is not
> > exploitable, especially as the code evolves.
>
> Regardless of whether or not all the bytes parsed by the
> uvc_parse_vendor_control() and uvc_parse_vendor_control() functions are
> part of the control descriptor or are split across multiple descriptors,
> they all come from the USB descriptors buffer and are all ultimately
> under device control.
>
> As far as I can tell, all this patch does it add a check on buffer[0]
> but has no impact on anything else. If a device can supply USB
> descriptors data with an invalid buffer[0] that currently causes any
> type of issue in the driver, the same device could supply the exact same
> USB descriptors with buffer[0] set to a value that will be accepted and
> still cause the same issue. Am I missing something ?
>
> > We could spend hours trying to craft a payload, plus extra time in
> > future reviews whenever uvc_parse_vendor_control() changes. It is much
> > safer to just enforce the correct bounds now.
>
> So is your concern only about introducing issues in the future without
> noticing ?

My main concerns are that the function's API looks wrong and yes, it
could introduce bugs in the future.

we basically have:
parse_field(void *data, size_t total_length)

instead of:
parse_field(void *data, size_t field_length)

btw: in parse_field we are ignoring the field_legth in most of the cases.

I am happy to drop the cc and fixes tags if you want to consider this
just a boring cleanup


>
> > As you said, the risk to break current hardware is low. If it happens
> > tis change is small enough to make it easy to revert.
> >
> > > > We add minimal code and solve a family of bugs. Do you think that it
> > > > is a better pattern that uvc_parse_vendor_control() and
> > > > uvc_parse_vendor_control() have visibility beyond the control that
> > > > they are parsing?
> > >
> > > I'm concerned that some devices may stop working. The risk is likely
> > > small though, but I'd like to understand what we get from this patch to
> > > see if it's worth the risk.
> > >
> > > > > > > > Change the code so we pass the actual length of the descriptor to the
> > > > > > > > parser.
> > > > > > > >
> > > > > > > > Note that this makes the existing check more strict and some devices
> > > > > > > > that are wrongly parsed today will not be probed now.
> > > > > > > >
> > > > > > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > > > > > Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
> > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > > > > > > ---
> > > > > > > >  drivers/media/usb/uvc/uvc_driver.c | 7 +++++--
> > > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > index e289cc71ba98..429f1ab19a2a 100644
> > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > @@ -1248,11 +1248,14 @@ static int uvc_parse_control(struct uvc_device *dev)
> > > > > > > >        */
> > > > > > > >
> > > > > > > >       while (buflen > 2) {
> > > > > > > > -             if (uvc_parse_vendor_control(dev, buffer, buflen) ||
> > > > > > > > +             if (buflen < buffer[0] || buffer[0] < 3)
> > > > > > > > +                     return -EINVAL;
> > > > > > > > +
> > > > > > > > +             if (uvc_parse_vendor_control(dev, buffer, buffer[0]) ||
> > > > > > > >                   buffer[1] != USB_DT_CS_INTERFACE)
> > > > > > > >                       goto next_descriptor;
> > > > > > > >
> > > > > > > > -             ret = uvc_parse_standard_control(dev, buffer, buflen);
> > > > > > > > +             ret = uvc_parse_standard_control(dev, buffer, buffer[0]);
> > > > > > > >               if (ret < 0)
> > > > > > > >                       return ret;
> > > > > > > >
> > > > > > > >
> > > > > > > > ---
> > > > > > > > base-commit: 27953c044974baf7e24dee3e9342fe0103dea80c
> > > > > > > > change-id: 20260911-uvc-ctrl-bound-9c1940f06fc7
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda