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

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



>
> 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