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

[Natasha Klaus <natalie.klaus@runtimeverification.com>]

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

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 <natalie.klaus@runtimeverification.com>

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.

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