[RFC PATCH] media: uvcvideo: compute frame buffer size in 64-bit arithmetic

[Natasha Klaus]

uvc_parse_frame() computes the default frame buffer size as

	frame->dwMaxVideoFrameBufferSize = format->bpp * frame->wWidth
					 * frame->wHeight / 8;

format->bpp is u8 and wWidth and wHeight are u16, so all three operands
promote to signed int and the product is evaluated in 32 bits. All three
come from the frame descriptor and none of them is range checked, so a
device can drive the product up to about 1.1e12 against a type that
holds 2.1e9.

The kernel builds with -fno-strict-overflow, so this is a defined
wraparound rather than undefined behaviour, but the stored value is
still wrong. A descriptor reporting bpp=207, wWidth=18501 and
wHeight=43738 has a true frame size of 20937965595 and the driver
stores 27. For bpp=233, wWidth=63435 and wHeight=58989 it stores 0, and
uvc_queue_setup() then hands vb2 a zero plane size.

Evaluate the product in 64-bit arithmetic by casting the first operand.

Signed-off-by: Natasha Klaus <natalie.klaus@xxxxxxxxxxxxxxxxxxxxxxx>
---
This is RFC because the patch is deliberately incomplete and I would
rather you pick the remedy than guess at it.

The cast fixes the arithmetic, but dwMaxVideoFrameBufferSize is a
32-bit field, so the assignment still truncates and sizes above 4 GiB
are not representable. The zero case survives too: bpp=32,
wWidth=32768, wHeight=32768 gives exactly 2^35, the 64-bit quotient is
exactly 2^32, and the stored value is 0 again.

So the open question is what should happen when a descriptor claims a
frame that does not fit. As I see it:

  1. this patch, accepting that oversized values truncate
  2. cap at U32_MAX
  3. reject the frame descriptor, on the grounds that a device claiming
     a 100 GB frame is lying
  4. range check wWidth, wHeight and bpp at parse time, which would
     also cover the two sibling computations

I am happy to send whichever you prefer as a proper patch.

Two other places take the same unchecked operands the same way:
uvc_v4l2_get_bytesperline() in uvc_v4l2.c, and the bandwidth estimate
in uvc_fixup_video_ctrl() in uvc_video.c, which is clamped only from
below. If you want those fixed I would send a series rather than fold
them in here.

How this came up: we are building a memory-safe Rust rewrite of the UVC
descriptor parser and proving properties about it, and the Rust and the
C disagreed on this field. Standalone reproducer, one command, builds
under gcc and clang with and without -fno-strict-overflow:

  https://github.com/runtimeverification/uvc-frame-size-overflow

Two caveats. This was measured on the isolated expression with the
kernel operand types, not on a running kernel and not on a UVC gadget,
so the vb2 behaviour above comes from reading the code rather than from
observing it. And I was unable to search the list archives from my
environment, so apologies if this has already been discussed.

 drivers/media/usb/uvc/uvc_driver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index e289cc71ba98..5dfbd487732d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -297,8 +297,8 @@ static int uvc_parse_frame(struct uvc_device *dev,
 	 * the value from the frame size.
 	 */
 	if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
-		frame->dwMaxVideoFrameBufferSize = format->bpp * frame->wWidth
-						 * frame->wHeight / 8;
+		frame->dwMaxVideoFrameBufferSize =
+			(u64)format->bpp * frame->wWidth * frame->wHeight / 8;
 
 	/*
 	 * Clamp the default frame interval to the boundaries. A zero

base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
-- 
2.34.1