Re: [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size

[Ricardo Ribalda]

Hi Natasha, hi Noam


On Tue, 18 Aug 2026 at 10:00, Natasha Klaus
<natalie.klaus@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> For uncompressed formats uvc_parse_frame() recomputes
> dwMaxVideoFrameBufferSize from the frame dimensions and the bits per
> pixel. All three operands come from the frame and format descriptors and
> none of them is validated: wWidth and wHeight are read at
> uvc_driver.c:254 and uvc_driver.c:255, and bpp at uvc_driver.c:382.
>
> The computed size is therefore zero whenever any operand is zero, and
> also whenever the product is below 8 and truncates to zero on the shift,
> for instance bpp=1 with a 2x3 frame.
>
> A zero size is not harmless. It is copied into
> ctrl->dwMaxVideoFrameSize by uvc_fixup_video_ctrl() and reaches
> uvc_queue_setup() as the vb2 plane size, where it trips
> WARN_ON(!plane_sizes[i]) in vb2_core_reqbufs() at
> drivers/media/common/videobuf2/videobuf2-core.c:951 and fails
> VIDIOC_REQBUFS with -EINVAL. On a kernel built with panic_on_warn that
> WARN is fatal.
>
> Such a frame can also become the active one without any application
> asking for it: when no frame matches the device's default bFrameIndex,
> uvc_video_init() falls back to frames[0] at
> drivers/media/usb/uvc/uvc_video.c:2298, so a device that also has
> usable frames can come up unusable.
>
> Skip the frame descriptor instead of rejecting it. Rejecting the
> descriptor would discard the whole streaming interface, including every
> valid format on it. Skipping follows the convention introduced by
> commit 81f3affa19d6 ("media: uvcvideo: Don't expose unsupported formats
> to userspace"), which drops a format descriptor the driver cannot use
> rather than failing the parse, for the same reason: to keep an unusable
> descriptor from reaching userspace and triggering a WARN_ON. Extend the
> existing "return 0 means skip this descriptor" handling from the format
> loop to the frame loop so parsing continues with the next frame and the
> rest of the format survives.
>
> Frame based compressed formats are not affected. They legitimately
> carry a zero dwMaxVideoFrameBufferSize, set unconditionally at
> uvc_driver.c:265 because the frame based frame descriptor has no such
> field, and they never enter this branch because it is guarded by
> !UVC_FMT_FLAG_COMPRESSED.
>
> Signed-off-by: Natasha Klaus <natalie.klaus@xxxxxxxxxxxxxxxxxxxxxxx>
> ---
> Applies on top of Noam Ben Shimon's v2:
> https://lore.kernel.org/linux-media/20260812103251.18309-1-noambs2999@xxxxxxxxx/
> It sits directly after his overflow check and will not apply without it.

I think we need to have some consistency. We cannot have  one
condition returning -EINVAL and the other skipping it.

How does this plan sound to you:

1) Refactor a bit uvc_parse_frame (warning! not tested)
diff --git a/drivers/media/usb/uvc/uvc_driver.c
b/drivers/media/usb/uvc/uvc_driver.c
index e289cc71ba98..6cbeaf10d2e0 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -243,10 +243,10 @@ static int uvc_parse_frame(struct uvc_device *dev,
        n = n ? n : 3;

        if (buflen < 26 + 4 * n) {
-               uvc_dbg(dev, DESCR,
-                       "device %d videostreaming interface %d FRAME error\n",
-                       dev->udev->devnum, alts->desc.bInterfaceNumber);
-               return -EINVAL;
+               dev_warn(&streaming->intf->dev,
+                        "UVC non compliance: device %d videostreaming
interface %d FRAME error\n",
+                        dev->udev->devnum, alts->desc.bInterfaceNumber);
+               return -ENODATA;
        }

        frame->bFrameIndex = buffer[3];
@@ -312,6 +312,8 @@ static int uvc_parse_frame(struct uvc_device *dev,
                      frame->dwFrameInterval[0],
                      frame->dwFrameInterval[maxIntervalIndex]);

+       // Your overflow and zero checks go here and return -EINVAL
+
        /*
         * Some devices report frame intervals that are not functional. If the
         * corresponding quirk is set, restrict operation to the first interval
@@ -329,7 +331,7 @@ static int uvc_parse_frame(struct uvc_device *dev,

        *intervals += n;

-       return buffer[0];
+       return 0;
 }

 static int uvc_parse_format(struct uvc_device *dev,
@@ -492,11 +494,12 @@ static int uvc_parse_format(struct uvc_device *dev,
                        ret = uvc_parse_frame(dev, streaming, format, frame,
                                              intervals, ftype,
width_multiplier,
                                              buffer, buflen);
-                       if (ret < 0)
+                       if (!ret)
+                               format->nframes++;
+                       if (ret == -ENODATA)
                                return ret;
-                       format->nframes++;
-                       buflen -= ret;
-                       buffer += ret;
+                       buflen -= buffer[0];
+                       buffer += buffer[0];
                }
        }

2) apply a modified version of Noam patch with the fixed error message

3) Apply Natasha's patch

If Noam is okay with this, perhaps Natasha could prepare a patchset
with the 3 patches? (keeping Noams author on his patch)


WDYY?

Regards

>
> One consequence worth naming: if every frame of the default format is
> zero-sized, nframes ends up 0 and uvc_video_init() fails probe at
> uvc_video.c:2286. This cascade is not new here. 81f3affa19d6 already has
> it one level up, where skipping enough formats leaves nformats == 0 and
> trips the same guard at uvc_video.c:2226. Such a device has nothing to
> stream either way, but the outcome is no node rather than a node that
> fails at REQBUFS, so it is a judgement call I would rather leave to you.
>
> This does not cover compressed formats. For UVC 1.10 and later
> uvc_fixup_video_ctrl() does not overwrite dwMaxVideoFrameSize, so a zero
> in the device's probe response reaches vb2 unchecked and no parse-time
> check can see it.
>
> Not tested on hardware or a UVC gadget. Built and verified against the
> isolated expression only.
>
>  drivers/media/usb/uvc/uvc_driver.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 29e23f94751c..e5858cec7ee4 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -309,6 +309,20 @@ static int uvc_parse_frame(struct uvc_device *dev,
>                         return -EINVAL;
>                 }
>
> +               /*
> +                * A zero-sized frame is unusable: it reaches vb2 as a zero
> +                * plane size, and it is reported to userspace as a 0x0 frame
> +                * with a zero sizeimage. Skip the frame descriptor, the
> +                * caller moves on to the next one.
> +                */
> +               if (!bufsize) {
> +                       dev_warn(&streaming->intf->dev,
> +                                "UVC non compliance: FRAME %u has zero size (%ux%u, %u bpp), skipping it.\n",
> +                                frame->bFrameIndex, frame->wWidth,
> +                                frame->wHeight, format->bpp);
> +                       return 0;
> +               }
> +
>                 frame->dwMaxVideoFrameBufferSize = bufsize;
>         }
>
> @@ -506,6 +520,11 @@ static int uvc_parse_format(struct uvc_device *dev,
>                                               buffer, buflen);
>                         if (ret < 0)
>                                 return ret;
> +                       if (!ret) {
> +                               buflen -= buffer[0];
> +                               buffer += buffer[0];
> +                               continue;
> +                       }
>                         format->nframes++;
>                         buflen -= ret;
>                         buffer += ret;
>
> base-commit: bae860246e920a7d24256858b69133c9c5f1f6a1
> --
> 2.34.1
>


-- 
Ricardo Ribalda