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

[David Laight <david.laight.linux@gmail.com>]

On Fri,  7 Aug 2026 13:14:33 +0300
Noam Ben Shimon <noambs2999@gmail.com> 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 <noambs2999@gmail.com>
> ---
>  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