[PATCH v2 2/3] media: uvcvideo: Fix integer overflow in frame buffer size calculation

From: Noam Ben Shimon <[email protected]>

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 skip the frame descriptor. An uncompressed frame this large
is probably not a real device, and skipping it leaves the rest of the
format and the streaming interface usable.

The rounding also changes from truncation to round-up. Truncation is
pre-existing rather than introduced here: the original expression used
integer division, so it has rounded a partial trailing byte away since
the driver was merged. Rounding up is the right direction for a buffer
size, and DIV_ROUND_UP() against BITS_PER_BYTE is what the rest of the
media tree uses for this computation, including uvc_parse_format()
itself for the FORCE_BPP quirk.

Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Cc: [email protected]
Signed-off-by: Noam Ben Shimon <[email protected]>
Reviewed-by: Ricardo Ribalda <[email protected]>
Signed-off-by: Natasha Klaus <[email protected]>
---
Changes from Noam's v2:
- Rebased onto patch 1/3; this patch no longer applies standalone.
- Diagnostic changed from uvc_dbg(dev, DESCR, ...) to dev_warn() on
  &streaming->intf->dev, reworded to begin "UVC non compliance: " and to
  say the frame is skipped. The explicit device and interface numbers are
  dropped from the message text because dev_warn() already identifies the
  interface. (The original could not be kept as-is in any case: patch 1/3
  removes the local alts variable it referenced.)
- bpp, wWidth and wHeight added to the message text (David Laight), matching
  the wording of the diagnostic in 3/3.
- The >> 3 replaced with DIV_ROUND_UP() against BITS_PER_BYTE (David Laight),
  so a partial trailing byte is no longer dropped. The overflow check is
  applied to the rounded-up value.
- The return value is still -EINVAL; what changed is its meaning, which
  patch 1/3 redefines as "skip this frame descriptor" rather than "fail the
  whole streaming interface".
- Last paragraph of the commit message reworded from "reject the frame
  descriptor ... rejection is consistent with the other checks over
  malformed-descriptors in this function" to describe skipping instead, and
  a paragraph added on the rounding change.
- Ricardo Ribalda's Reviewed-by dropped, as it was given on the unmodified
  v2.
- Submitter's Signed-off-by added.
- Fixes: and Cc: stable lines unchanged.
- DIV_ROUND_UP changed to DIV_ROUND_UP_ULL per Ricardo's review.

Rounding up also moves the U32_MAX boundary: two inputs within the field
limits (bpp=79 at 10077x43161 and bpp=237 at 3359x43161) landed exactly on
U32_MAX with the old truncation and are now rejected, since the rounded-up
size is not representable.

Build tested on x86_64 only. No hardware and no UVC gadget were used.

 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 b94fe5366e55..d7d71418f875 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -295,9 +295,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 = DIV_ROUND_UP_ULL((u64)format->bpp * frame->wWidth *
+					   frame->wHeight, BITS_PER_BYTE);
+		if (bufsize > U32_MAX) {
+			dev_warn(&streaming->intf->dev,
+				 "UVC non compliance: FRAME %u computed buffer size overflows (%ux%u, %u bpp), skipping it.\n",
+				 frame->bFrameIndex, frame->wWidth,
+				 frame->wHeight, format->bpp);
+			return -EINVAL;
+		}
+
+		frame->dwMaxVideoFrameBufferSize = bufsize;
+	}
 
 	/*
 	 * Clamp the default frame interval to the boundaries. A zero
-- 
2.34.1