Re: [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation

[Ricardo Ribalda]

Hi Noam

On Fri, 7 Aug 2026 at 12:26, Noam Ben Shimon <noambs2999@xxxxxxxxx> wrote:
>
> In the function uvc_parse_frame(), it recomputes
> dwMaxVideoFrameBufferSize for uncompressed formats. This helps working
> around devices that report it wrong:
>
>         frame->dwMaxVideoFrameBufferSize = format->bpp * frame->wWidth
>                                          * frame->wHeight / 8;
>
> These three arguments originate from the device's own descriptors, and
> therefore can be decided by it. bpp is a u8 and wWidth and wHeight are
> u16. The expression is evaluated in int, and the maximum value is
> 255 * 65535 * 65535 (which is roughly 510 times INT_MAX).
> A device that declares large dimensions therefore overflows a signed
> int here.
> The kernel is built using -fno-strict-overflow, so this wraps rather
> than being miscompiled, but the wrapped value (which is often negative)
> is then divided by 8 and stored in a u32 used as a size.
>
> Two examples for this (using legal field values):
>
>   - 32 bpp, 16384x4096: the product is exactly 2^31 and wraps to
>     INT_MIN. After division and conversion to u32 the field has the
>     value 4026531840 rather than 268435456.
>
>   - 16 bpp, 16384x16384: the product is exactly 2^32 and wraps to 0.
>     The field holds 0, rather than the correct 536870912.
>
> I don't think memory corruption is a consequence of this. The value
> reaches uvc_queue_setup() as the vb2 buffer size, and every copy on the
> decode path is bounded by buf->length, which uvc_buffer_prepare() gets
> from vb2_plane_size() rather than this field. What a wrapped value does
> instead is make the driver describe the stream inconsistently.
> For an uncompressed format uvc_fixup_video_ctrl() copies it into
> ctrl->dwMaxVideoFrameSize unconditionally, and that becomes the
> sizeimage reported by VIDIOC_G_FMT. This is while width, height and
> bytesperline continue to describe the full frame.
> This also makes uvc_video_validate_buffer() mark error on all frames,
> because it is comparing bytesused against the same number.
>
> Compute the size in 64-bit, and if the result does not fit in the u32
> field then reject the frame descriptor. An uncompressed frame this
> large is probably not a real device and rejection is consistent with
> the other checks over malformed-descriptors in this function.
>
> Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Noam Ben Shimon <noambs2999@xxxxxxxxx>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index e289cc71ba98..d319368f9d21 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -296,9 +296,21 @@ static int uvc_parse_frame(struct uvc_device *dev,
>          * information. For uncompressed formats this can be fixed by computing
>          * the value from the frame size.
>          */
> -       if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
> -               frame->dwMaxVideoFrameBufferSize = format->bpp * frame->wWidth
> -                                                * frame->wHeight / 8;
> +       if (!(format->flags & UVC_FMT_FLAG_COMPRESSED)) {
> +               u64 bufsize;
> +
> +               bufsize = (u64)format->bpp * frame->wWidth * frame->wHeight / 8;
I'd rather avoid doing a u64 division (even knowing that the compiler
is often clever enough to avoid it)
What about:

bufsize = ((u64)format->bpp * frame->wWidth * frame->wHeight) >> 3;

> +               if (bufsize > U32_MAX) {
> +                       uvc_dbg(dev, DESCR,
> +                               "device %d videostreaming interface %d FRAME %u: computed buffer size overflows\n",
> +                               dev->udev->devnum,
> +                               alts->desc.bInterfaceNumber,
> +                               frame->bFrameIndex);
> +                       return -EINVAL;
> +               }
> +
> +               frame->dwMaxVideoFrameBufferSize = bufsize;
> +       }
>
>         /*
>          * Clamp the default frame interval to the boundaries. A zero
>
> base-commit: f9a2394a23482bfd330911e9c8295b71724feacd
> --
> 2.34.1
> I was working on a certain device that had a variation of the Linux kernel.
> During my work, I had searched for memory mismanagement and misallocation in media
> drivers. At some point I stumbled into the `uvc_driver.c` and found a flaw that is
> not a vulnerability, but still a flaw.  I figured that it was worth letting you
> know rather than shrug it off.
Thanks for the report. Most of the times we want to trust the hw...
but for usb cameras is totally worth it to be extra cautious.

With my change (and if you test it :P) you can add my:
Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>

>


-- 
Ricardo Ribalda