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

Hi David,

Thanks for the review!
Agreed on "frame buffer size" rather than "buffer size".

On Tue, 18 Aug 2026 at 11:28, David Laight <[email protected]> wrote:
>
> On Fri,  7 Aug 2026 13:14:33 +0300
> Noam Ben Shimon <[email protected]> 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")
> > Signed-off-by: Noam Ben Shimon <[email protected]>
> > ---
> >  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 bet there is a requirement that width*bpp is a multiple of 8 (or even 32)?
> You definitely don't want the divide rounding down!
>
> > +             if (bufsize > U32_MAX) {
>
> Should that be >= ?
>
> > +                     uvc_dbg(dev, DESCR,
> > +                             "device %d videostreaming interface %d FRAME %u: computed buffer size overflows\n",
>
> s/buffer/frame buffer/ ?
>
> > +                             dev->udev->devnum,
> > +                             alts->desc.bInterfaceNumber,
> > +                             frame->bFrameIndex);
>
> I'd include the bpp, width and height values in the trace.
> If the error happens the first thing you need the the three values.
>
>         David
>
> > +                     return -EINVAL;
> > +             }
> > +
> > +             frame->dwMaxVideoFrameBufferSize = bufsize;
> > +     }
> >
> >       /*
> >        * Clamp the default frame interval to the boundaries. A zero
> >
> > base-commit: f9a2394a23482bfd330911e9c8295b71724feacd
>