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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Sep 28, 2026 at 09:49:05PM +0200, Ricardo Ribalda wrote:
> On Mon, 28 Sept 2026 at 21:45, Laurent Pinchart wrote:
> > On Mon, Sep 28, 2026 at 09:03:41PM +0200, Ricardo Ribalda wrote:
> > > On Mon, 28 Sept 2026 at 20:44, Laurent Pinchart 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.
> >
> > I agree there's a quite small risk of introducing future bugs. This is
> > what I wanted to know, if there was an actual issue today (as in
> > exploitable bugs), or if it was only a forward-looking change.
> >
> > > 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 it stands, I think the risk of introducing regressions is likely
> > smaller. bLength can't be completely off or the USB core would fail
> > parsing descriptors. It could be smaller than needed while still
> > pointing to the next descriptors, with the control parsing then using
> > the first few bytes of the next descriptor, but the chance that would
> > result in values that don't cause observable weird effects are slim.
> >
> > So I think we can merge this change. I'd drop the backport as we're not
> > fixing any existing issue. And maybe avoid "Fix" in the subject, to
> > avoid autosel being triggered ?
> 
> sgtm. Do you need a v2 or can you modify it locally when merging?

Can you tell me what you'd like as an updated subject line (and include
commit message updates if you think that's relevant) ? I'll then use
that and apply the patch.

> > > > > 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@vger.kernel.org
> > > > > > > > > > > Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
> > > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > > > > > ---
> > > > > > > > > > >  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