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

On Tue, 18 Aug 2026 at 08:54, Ricardo Ribalda <[email protected]> wrote:
>
> Hi Natasha
>
> Thanks for the thorough review
>
>
> On Tue, 18 Aug 2026 at 08:45, Natasha Klaus
> <[email protected]> wrote:
> >
> > On Wed, Aug 12, 2026 at 01:32:51PM +0300, Noam Ben Shimon wrote:
> > > +     if (!(format->flags & UVC_FMT_FLAG_COMPRESSED)) {
> > > +             u64 bufsize;
> > > +
> > > +             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;
> > > +     }
> >
> > Ricardo asked me to look at this. I had been looking at the same expression
> > independently, so I checked your numbers and the surrounding behaviour
> > rather than only the diff.
> >
> > Both examples in the commit message reproduce exactly. 32 bpp at
> > 16384x4096 gives 4026531840 against a true 268435456, and 16 bpp at
> > 16384x16384 gives 0 against 536870912.
> >
> > One case I would add to the commit message, because it is the strongest
> > argument for rejecting rather than only widening. bpp=32 at 32768x32768
> > is exactly 2^35, so the 64-bit quotient is exactly 2^32. A bare (u64)
> > cast without your check would store 0 there, which is the same failure
> > the patch removes. Your check catches it.
> >
> > I also checked when the check can fire at all. For bpp <= 8 the threshold
> > is unreachable given the u16 field limits, and it does not need to be
> > reachable: the largest possible result at bpp=8 is 4294836225, which
> > still fits in u32. So the check fires exactly where it is needed and
> > nowhere else. That seemed worth confirming rather than assuming.
> >
> > My one question is about the error path rather than the arithmetic.
> >
> > uvc_parse_frame() has a single caller, and -EINVAL propagates further
> > than I first expected:
> >
> >   uvc_driver.c:495   return ret, so the whole format is abandoned
> >   uvc_driver.c:745   goto error, so remaining formats are never parsed
> >   uvc_driver.c:788   usb_driver_release_interface() and uvc_stream_delete(),
> >                      so the streaming interface never reaches dev->streams
> >   uvc_driver.c:1004  the uvc_parse_streaming() return value is discarded,
> >                      so probe continues and succeeds
> >   uvc_driver.c:2135  "No streaming interface found for terminal %u"
> >
> > So one malformed frame descriptor costs the entire streaming interface,
> > not just that frame, and probe still succeeds. On a single-interface
> > webcam that means the device binds with no /dev/videoN, and the only
> > explanation is the uvc_dbg line above, which sits behind a debug bit
> > that is off by default.
> >
> > I do not think this is a practical regression risk, since no plausible
> > device reaches 2^35, and the surrounding function is otherwise built
> > around repairing bad descriptors rather than rejecting them. What
> > bothers me is the silence: a user who does trip it sees a device that
> > binds and produces nothing, with no logged reason. Two ways to address
> > that:
> >
> >   - dev_warn() instead of uvc_dbg(), so the reason is visible without a
> >     debug build
> >   - skip only that frame descriptor and continue, rather than failing
> >     the format
>
> Good point. if you send a v3. please use:
>  dev_warn_once(&dev->intf->dev, "UVC non compliance: blah blah foo bar"...)
>
> We are trying to standarize the "UVC non compliance" string

I meant dev_warn() not dev_warn_once()

The user cannot easily trigger the error message (besides re-probing the device)


>
>
>
> >
> > Either would satisfy me. If you and the maintainers would rather keep
> > -EINVAL with uvc_dbg as it stands, I have no objection to that either,
> > and you are welcome to add
> >
> > Reviewed-by: Natasha Klaus <[email protected]>
> >
> > to v2 as it is.
> >
> > Separately, and explicitly not an objection to this patch: a zero
> > dwMaxVideoFrameBufferSize stays reachable from the other end. Any zero
> > operand, or any product below 8, gives 0 after the shift, and bpp=0 does
> > reach the computation on uncompressed formats. Nothing between the
> > descriptor bytes at uvc_driver.c:254, :255 and :382 and this line
> > validates any of the three. A zero then goes through uvc_video.c:214
> > into stream->ctrl, sizes vb2 at uvc_queue.c:90, and trips
> > WARN_ON(!plane_sizes[i]) in vb2_core_reqbufs() at
> > videobuf2-core.c:951. That is pre-existing and unchanged by your patch,
> > so it is not yours to fix here. I am happy to send a follow-up if the
> > maintainers want it as a separate change.
>
> Happy to review it if you send it :)
>
> >
> > One caveat on my side: all of the above comes from reading the tree at
> > v7.2, not from running it. I did not test on hardware or a UVC gadget,
> > and I did not build your patch.
> >
> > Natasha
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda