* [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation
@ 2026-08-07 10:14 Noam Ben Shimon
2026-08-10 14:12 ` Ricardo Ribalda
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Noam Ben Shimon @ 2026-08-07 10:14 UTC (permalink / raw)
To: laurent.pinchart, hansg, mchehab
Cc: linux-media, linux-kernel, Noam Ben Shimon
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;
+ 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;
+ }
/*
* Clamp the default frame interval to the boundaries. A zero
base-commit: f9a2394a23482bfd330911e9c8295b71724feacd
--
2.34.1
I was working on a certain device that had a variation of the Linux kernel.
During my work, I had searched for memory mismanagement and misallocation in media
drivers. At some point I stumbled into the `uvc_driver.c` and found a flaw that is
not a vulnerability, but still a flaw. I figured that it was worth letting you
know rather than shrug it off.
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation 2026-08-07 10:14 [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation Noam Ben Shimon @ 2026-08-10 14:12 ` Ricardo Ribalda 2026-08-12 10:32 ` [PATCH v2] " Noam Ben Shimon 2026-08-18 8:28 ` [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation David Laight 2 siblings, 0 replies; 14+ messages in thread From: Ricardo Ribalda @ 2026-08-10 14:12 UTC (permalink / raw) To: Noam Ben Shimon Cc: laurent.pinchart, hansg, mchehab, linux-media, linux-kernel Hi Noam On Fri, 7 Aug 2026 at 12:26, 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") Cc: stable@vger.kernel.org > 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 rather avoid doing a u64 division (even knowing that the compiler is often clever enough to avoid it) What about: 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; > + } > > /* > * Clamp the default frame interval to the boundaries. A zero > > base-commit: f9a2394a23482bfd330911e9c8295b71724feacd > -- > 2.34.1 > I was working on a certain device that had a variation of the Linux kernel. > During my work, I had searched for memory mismanagement and misallocation in media > drivers. At some point I stumbled into the `uvc_driver.c` and found a flaw that is > not a vulnerability, but still a flaw. I figured that it was worth letting you > know rather than shrug it off. Thanks for the report. Most of the times we want to trust the hw... but for usb cameras is totally worth it to be extra cautious. With my change (and if you test it :P) you can add my: Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] media: uvcvideo: Fix integer overflow in frame buffer size calculation 2026-08-07 10:14 [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation Noam Ben Shimon 2026-08-10 14:12 ` Ricardo Ribalda @ 2026-08-12 10:32 ` Noam Ben Shimon 2026-08-18 6:45 ` Natasha Klaus 2026-08-18 7:59 ` [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size Natasha Klaus 2026-08-18 8:28 ` [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation David Laight 2 siblings, 2 replies; 14+ messages in thread From: Noam Ben Shimon @ 2026-08-12 10:32 UTC (permalink / raw) To: laurent.pinchart, hansg, mchehab Cc: ribalda, linux-media, linux-kernel, Noam Ben Shimon, stable 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") Cc: stable@vger.kernel.org Signed-off-by: Noam Ben Shimon <noambs2999@gmail.com> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> --- Changes in v2: - Used a shift instead of division (Ricardo Ribalda) (Thanks!) - Add Cc: stable@vger.kernel.org (Ricardo Ribalda) Compile-tested using W=1 and no warnings. Tested with a UVC gadget over dummy_hcd with WSL. A frame descriptor declaring 32 bpp at 40000x40000 (computed size = 6400000000, which is above U32_MAX) is then rejected as expected, and the streaming interface is not registered: uvcvideo 1-1:1.0: Found format YUYV little-endian (0x56595559) uvcvideo 1-1:1.0: device 2 videostreaming interface 1 FRAME 1: computed buffer size overflows uvcvideo 1-1:1.0: No streaming interface found for terminal 32771. I was working on a certain device that had a variation of the Linux kernel. During my work, I had searched for memory mismanagement and misallocation in media drivers. At some point I stumbled into the uvc_driver.c and found a flaw that is not a vulnerability, but still a flaw. I figured that it was worth letting you know rather than shrug it off. 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..29e23f94751c 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) >> 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; + } /* * Clamp the default frame interval to the boundaries. A zero base-commit: f9a2394a23482bfd330911e9c8295b71724feacd -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] media: uvcvideo: Fix integer overflow in frame buffer size calculation 2026-08-12 10:32 ` [PATCH v2] " Noam Ben Shimon @ 2026-08-18 6:45 ` Natasha Klaus 2026-08-18 6:54 ` Ricardo Ribalda 2026-08-18 7:59 ` [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size Natasha Klaus 1 sibling, 1 reply; 14+ messages in thread From: Natasha Klaus @ 2026-08-18 6:45 UTC (permalink / raw) To: noambs2999 Cc: ribalda, laurent.pinchart, hansg, mchehab, linux-media, linux-kernel, Natasha Klaus 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] media: uvcvideo: Fix integer overflow in frame buffer size calculation 2026-08-18 6:45 ` Natasha Klaus @ 2026-08-18 6:54 ` Ricardo Ribalda 2026-08-18 6:57 ` Ricardo Ribalda 0 siblings, 1 reply; 14+ messages in thread From: Ricardo Ribalda @ 2026-08-18 6:54 UTC (permalink / raw) To: Natasha Klaus Cc: noambs2999, laurent.pinchart, hansg, mchehab, linux-media, linux-kernel Hi Natasha Thanks for the thorough review On Tue, 18 Aug 2026 at 08:45, Natasha Klaus <natalie.klaus@runtimeverification.com> 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 <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. 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] media: uvcvideo: Fix integer overflow in frame buffer size calculation 2026-08-18 6:54 ` Ricardo Ribalda @ 2026-08-18 6:57 ` Ricardo Ribalda 0 siblings, 0 replies; 14+ messages in thread From: Ricardo Ribalda @ 2026-08-18 6:57 UTC (permalink / raw) To: Natasha Klaus Cc: noambs2999, laurent.pinchart, hansg, mchehab, linux-media, linux-kernel On Tue, 18 Aug 2026 at 08:54, Ricardo Ribalda <ribalda@chromium.org> wrote: > > Hi Natasha > > Thanks for the thorough review > > > On Tue, 18 Aug 2026 at 08:45, Natasha Klaus > <natalie.klaus@runtimeverification.com> 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 I meant dev_warn() not dev_warn_once() The user cannot easily trigger the error message (besides re-probing the device) > > > > > > > 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. > > 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 -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size 2026-08-12 10:32 ` [PATCH v2] " Noam Ben Shimon 2026-08-18 6:45 ` Natasha Klaus @ 2026-08-18 7:59 ` Natasha Klaus 2026-08-18 8:31 ` Ricardo Ribalda 1 sibling, 1 reply; 14+ messages in thread From: Natasha Klaus @ 2026-08-18 7:59 UTC (permalink / raw) To: laurent.pinchart, hansg, mchehab Cc: ribalda, noambs2999, linux-media, linux-kernel, Natasha Klaus For uncompressed formats uvc_parse_frame() recomputes dwMaxVideoFrameBufferSize from the frame dimensions and the bits per pixel. All three operands come from the frame and format descriptors and none of them is validated: wWidth and wHeight are read at uvc_driver.c:254 and uvc_driver.c:255, and bpp at uvc_driver.c:382. The computed size is therefore zero whenever any operand is zero, and also whenever the product is below 8 and truncates to zero on the shift, for instance bpp=1 with a 2x3 frame. A zero size is not harmless. It is copied into ctrl->dwMaxVideoFrameSize by uvc_fixup_video_ctrl() and reaches uvc_queue_setup() as the vb2 plane size, where it trips WARN_ON(!plane_sizes[i]) in vb2_core_reqbufs() at drivers/media/common/videobuf2/videobuf2-core.c:951 and fails VIDIOC_REQBUFS with -EINVAL. On a kernel built with panic_on_warn that WARN is fatal. Such a frame can also become the active one without any application asking for it: when no frame matches the device's default bFrameIndex, uvc_video_init() falls back to frames[0] at drivers/media/usb/uvc/uvc_video.c:2298, so a device that also has usable frames can come up unusable. Skip the frame descriptor instead of rejecting it. Rejecting the descriptor would discard the whole streaming interface, including every valid format on it. Skipping follows the convention introduced by commit 81f3affa19d6 ("media: uvcvideo: Don't expose unsupported formats to userspace"), which drops a format descriptor the driver cannot use rather than failing the parse, for the same reason: to keep an unusable descriptor from reaching userspace and triggering a WARN_ON. Extend the existing "return 0 means skip this descriptor" handling from the format loop to the frame loop so parsing continues with the next frame and the rest of the format survives. Frame based compressed formats are not affected. They legitimately carry a zero dwMaxVideoFrameBufferSize, set unconditionally at uvc_driver.c:265 because the frame based frame descriptor has no such field, and they never enter this branch because it is guarded by !UVC_FMT_FLAG_COMPRESSED. Signed-off-by: Natasha Klaus <natalie.klaus@runtimeverification.com> --- Applies on top of Noam Ben Shimon's v2: https://lore.kernel.org/linux-media/20260812103251.18309-1-noambs2999@gmail.com/ It sits directly after his overflow check and will not apply without it. One consequence worth naming: if every frame of the default format is zero-sized, nframes ends up 0 and uvc_video_init() fails probe at uvc_video.c:2286. This cascade is not new here. 81f3affa19d6 already has it one level up, where skipping enough formats leaves nformats == 0 and trips the same guard at uvc_video.c:2226. Such a device has nothing to stream either way, but the outcome is no node rather than a node that fails at REQBUFS, so it is a judgement call I would rather leave to you. This does not cover compressed formats. For UVC 1.10 and later uvc_fixup_video_ctrl() does not overwrite dwMaxVideoFrameSize, so a zero in the device's probe response reaches vb2 unchecked and no parse-time check can see it. Not tested on hardware or a UVC gadget. Built and verified against the isolated expression only. drivers/media/usb/uvc/uvc_driver.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 29e23f94751c..e5858cec7ee4 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -309,6 +309,20 @@ static int uvc_parse_frame(struct uvc_device *dev, return -EINVAL; } + /* + * A zero-sized frame is unusable: it reaches vb2 as a zero + * plane size, and it is reported to userspace as a 0x0 frame + * with a zero sizeimage. Skip the frame descriptor, the + * caller moves on to the next one. + */ + if (!bufsize) { + dev_warn(&streaming->intf->dev, + "UVC non compliance: FRAME %u has zero size (%ux%u, %u bpp), skipping it.\n", + frame->bFrameIndex, frame->wWidth, + frame->wHeight, format->bpp); + return 0; + } + frame->dwMaxVideoFrameBufferSize = bufsize; } @@ -506,6 +520,11 @@ static int uvc_parse_format(struct uvc_device *dev, buffer, buflen); if (ret < 0) return ret; + if (!ret) { + buflen -= buffer[0]; + buffer += buffer[0]; + continue; + } format->nframes++; buflen -= ret; buffer += ret; base-commit: bae860246e920a7d24256858b69133c9c5f1f6a1 -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size 2026-08-18 7:59 ` [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size Natasha Klaus @ 2026-08-18 8:31 ` Ricardo Ribalda 2026-08-18 9:40 ` Natasha Klaus 2026-08-18 10:32 ` Natasha Klaus 0 siblings, 2 replies; 14+ messages in thread From: Ricardo Ribalda @ 2026-08-18 8:31 UTC (permalink / raw) To: Natasha Klaus, Noam Ben Shimon Cc: laurent.pinchart, hansg, mchehab, linux-media, linux-kernel Hi Natasha, hi Noam On Tue, 18 Aug 2026 at 10:00, Natasha Klaus <natalie.klaus@runtimeverification.com> wrote: > > For uncompressed formats uvc_parse_frame() recomputes > dwMaxVideoFrameBufferSize from the frame dimensions and the bits per > pixel. All three operands come from the frame and format descriptors and > none of them is validated: wWidth and wHeight are read at > uvc_driver.c:254 and uvc_driver.c:255, and bpp at uvc_driver.c:382. > > The computed size is therefore zero whenever any operand is zero, and > also whenever the product is below 8 and truncates to zero on the shift, > for instance bpp=1 with a 2x3 frame. > > A zero size is not harmless. It is copied into > ctrl->dwMaxVideoFrameSize by uvc_fixup_video_ctrl() and reaches > uvc_queue_setup() as the vb2 plane size, where it trips > WARN_ON(!plane_sizes[i]) in vb2_core_reqbufs() at > drivers/media/common/videobuf2/videobuf2-core.c:951 and fails > VIDIOC_REQBUFS with -EINVAL. On a kernel built with panic_on_warn that > WARN is fatal. > > Such a frame can also become the active one without any application > asking for it: when no frame matches the device's default bFrameIndex, > uvc_video_init() falls back to frames[0] at > drivers/media/usb/uvc/uvc_video.c:2298, so a device that also has > usable frames can come up unusable. > > Skip the frame descriptor instead of rejecting it. Rejecting the > descriptor would discard the whole streaming interface, including every > valid format on it. Skipping follows the convention introduced by > commit 81f3affa19d6 ("media: uvcvideo: Don't expose unsupported formats > to userspace"), which drops a format descriptor the driver cannot use > rather than failing the parse, for the same reason: to keep an unusable > descriptor from reaching userspace and triggering a WARN_ON. Extend the > existing "return 0 means skip this descriptor" handling from the format > loop to the frame loop so parsing continues with the next frame and the > rest of the format survives. > > Frame based compressed formats are not affected. They legitimately > carry a zero dwMaxVideoFrameBufferSize, set unconditionally at > uvc_driver.c:265 because the frame based frame descriptor has no such > field, and they never enter this branch because it is guarded by > !UVC_FMT_FLAG_COMPRESSED. > > Signed-off-by: Natasha Klaus <natalie.klaus@runtimeverification.com> > --- > Applies on top of Noam Ben Shimon's v2: > https://lore.kernel.org/linux-media/20260812103251.18309-1-noambs2999@gmail.com/ > It sits directly after his overflow check and will not apply without it. I think we need to have some consistency. We cannot have one condition returning -EINVAL and the other skipping it. How does this plan sound to you: 1) Refactor a bit uvc_parse_frame (warning! not tested) diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index e289cc71ba98..6cbeaf10d2e0 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -243,10 +243,10 @@ static int uvc_parse_frame(struct uvc_device *dev, n = n ? n : 3; if (buflen < 26 + 4 * n) { - uvc_dbg(dev, DESCR, - "device %d videostreaming interface %d FRAME error\n", - dev->udev->devnum, alts->desc.bInterfaceNumber); - return -EINVAL; + dev_warn(&streaming->intf->dev, + "UVC non compliance: device %d videostreaming interface %d FRAME error\n", + dev->udev->devnum, alts->desc.bInterfaceNumber); + return -ENODATA; } frame->bFrameIndex = buffer[3]; @@ -312,6 +312,8 @@ static int uvc_parse_frame(struct uvc_device *dev, frame->dwFrameInterval[0], frame->dwFrameInterval[maxIntervalIndex]); + // Your overflow and zero checks go here and return -EINVAL + /* * Some devices report frame intervals that are not functional. If the * corresponding quirk is set, restrict operation to the first interval @@ -329,7 +331,7 @@ static int uvc_parse_frame(struct uvc_device *dev, *intervals += n; - return buffer[0]; + return 0; } static int uvc_parse_format(struct uvc_device *dev, @@ -492,11 +494,12 @@ static int uvc_parse_format(struct uvc_device *dev, ret = uvc_parse_frame(dev, streaming, format, frame, intervals, ftype, width_multiplier, buffer, buflen); - if (ret < 0) + if (!ret) + format->nframes++; + if (ret == -ENODATA) return ret; - format->nframes++; - buflen -= ret; - buffer += ret; + buflen -= buffer[0]; + buffer += buffer[0]; } } 2) apply a modified version of Noam patch with the fixed error message 3) Apply Natasha's patch If Noam is okay with this, perhaps Natasha could prepare a patchset with the 3 patches? (keeping Noams author on his patch) WDYY? Regards > > One consequence worth naming: if every frame of the default format is > zero-sized, nframes ends up 0 and uvc_video_init() fails probe at > uvc_video.c:2286. This cascade is not new here. 81f3affa19d6 already has > it one level up, where skipping enough formats leaves nformats == 0 and > trips the same guard at uvc_video.c:2226. Such a device has nothing to > stream either way, but the outcome is no node rather than a node that > fails at REQBUFS, so it is a judgement call I would rather leave to you. > > This does not cover compressed formats. For UVC 1.10 and later > uvc_fixup_video_ctrl() does not overwrite dwMaxVideoFrameSize, so a zero > in the device's probe response reaches vb2 unchecked and no parse-time > check can see it. > > Not tested on hardware or a UVC gadget. Built and verified against the > isolated expression only. > > drivers/media/usb/uvc/uvc_driver.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 29e23f94751c..e5858cec7ee4 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -309,6 +309,20 @@ static int uvc_parse_frame(struct uvc_device *dev, > return -EINVAL; > } > > + /* > + * A zero-sized frame is unusable: it reaches vb2 as a zero > + * plane size, and it is reported to userspace as a 0x0 frame > + * with a zero sizeimage. Skip the frame descriptor, the > + * caller moves on to the next one. > + */ > + if (!bufsize) { > + dev_warn(&streaming->intf->dev, > + "UVC non compliance: FRAME %u has zero size (%ux%u, %u bpp), skipping it.\n", > + frame->bFrameIndex, frame->wWidth, > + frame->wHeight, format->bpp); > + return 0; > + } > + > frame->dwMaxVideoFrameBufferSize = bufsize; > } > > @@ -506,6 +520,11 @@ static int uvc_parse_format(struct uvc_device *dev, > buffer, buflen); > if (ret < 0) > return ret; > + if (!ret) { > + buflen -= buffer[0]; > + buffer += buffer[0]; > + continue; > + } > format->nframes++; > buflen -= ret; > buffer += ret; > > base-commit: bae860246e920a7d24256858b69133c9c5f1f6a1 > -- > 2.34.1 > -- Ricardo Ribalda ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size 2026-08-18 8:31 ` Ricardo Ribalda @ 2026-08-18 9:40 ` Natasha Klaus 2026-08-18 9:53 ` Natasha Klaus 2026-08-18 10:18 ` Ricardo Ribalda 2026-08-18 10:32 ` Natasha Klaus 1 sibling, 2 replies; 14+ messages in thread From: Natasha Klaus @ 2026-08-18 9:40 UTC (permalink / raw) To: ribalda, noambs2999 Cc: laurent.pinchart, hansg, mchehab, linux-media, linux-kernel On Tue, Aug 18, 2026, Ricardo Ribalda wrote: > I think we need to have some consistency. We cannot have one > condition returning -EINVAL and the other skipping it. Agreed, and your shape is cleaner than mine. > If Noam is okay with this, perhaps Natasha could prepare a patchset > with the 3 patches? (keeping Noams author on his patch) Happy to. Noam, are you okay with me carrying your patch in a series? One question on placement before I write it. Your comment puts the checks after the frame interval parsing, which is outside the !UVC_FMT_FLAG_COMPRESSED branch. Frame based formats legitimately carry a zero dwMaxVideoFrameBufferSize, set unconditionally at uvc_driver.c:265 because the frame based frame descriptor has no such field, so a zero check there would skip every frame of a conformant frame based device. Did you mean the checks stay inside the branch with only the return value changing to -EINVAL, or outside with an explicit frame based exemption? I will test the refactor before sending. The Media CI failure on my patch is the missing dependency on Noam's v2. The series fixes that. Natasha ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size 2026-08-18 9:40 ` Natasha Klaus @ 2026-08-18 9:53 ` Natasha Klaus 2026-08-18 10:18 ` Ricardo Ribalda 1 sibling, 0 replies; 14+ messages in thread From: Natasha Klaus @ 2026-08-18 9:53 UTC (permalink / raw) To: ribalda, noambs2999 Cc: laurent.pinchart, hansg, mchehab, linux-media, linux-kernel Three more things on the refactor, from reading it rather than running it. Placement: bufsize is declared at uvc_driver.c:300 and the block closes at 327, while interval parsing runs 329-354, so at the proposed position the variable is out of scope. And the exemption a check outside the branch would need is format->flags & UVC_FMT_FLAG_COMPRESSED, which is the branch condition at line 299 itself. The compressed case is worse than frame based alone. For UVC 1.10 and later uvc_fixup_video_ctrl() at uvc_video.c:214-218 never consumes the descriptor value, the size comes from the probe response. An MJPEG descriptor reporting zero is inert on those devices today, so a check outside the branch would skip those frames and break cameras that stream fine. -ENODATA already appears in this driver with the opposite polarity, at uvc_video.c:1284 and :1310, where it means drop this payload and carry on. Using it for the fatal case reads backwards against that. The mechanical parts of your refactor hold: buffer[0] cannot be zero because the USB core truncates the config at the first bLength < 2 descriptor (config.c:706, :785), the interval array is pre-counted at uvc_driver.c:718-729 with *intervals += n after every skip return, and all frames[] access is positional. Not tested on hardware. I will build and test before sending the series. Natasha ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size 2026-08-18 9:40 ` Natasha Klaus 2026-08-18 9:53 ` Natasha Klaus @ 2026-08-18 10:18 ` Ricardo Ribalda 1 sibling, 0 replies; 14+ messages in thread From: Ricardo Ribalda @ 2026-08-18 10:18 UTC (permalink / raw) To: Natasha Klaus Cc: noambs2999, laurent.pinchart, hansg, mchehab, linux-media, linux-kernel Hi Natasha On Tue, 18 Aug 2026 at 11:40, Natasha Klaus <natalie.klaus@runtimeverification.com> wrote: > > On Tue, Aug 18, 2026, Ricardo Ribalda wrote: > > I think we need to have some consistency. We cannot have one > > condition returning -EINVAL and the other skipping it. > > Agreed, and your shape is cleaner than mine. > > > If Noam is okay with this, perhaps Natasha could prepare a patchset > > with the 3 patches? (keeping Noams author on his patch) > > Happy to. Noam, are you okay with me carrying your patch in a series? > > One question on placement before I write it. Your comment puts the checks after > the frame interval parsing, which is outside the !UVC_FMT_FLAG_COMPRESSED > branch. Frame based formats legitimately carry a zero > dwMaxVideoFrameBufferSize, set unconditionally at uvc_driver.c:265 because the > frame based frame descriptor has no such field, so a zero check there would skip > every frame of a conformant frame based device. Did you mean the checks stay > inside the branch with only the return value changing to -EINVAL, or outside > with an explicit frame based exemption? I meant keep the checks on the position of your patch and Noam patch. Sorry for the misunderstanding. > > I will test the refactor before sending. > > The Media CI failure on my patch is the missing dependency on Noam's v2. The > series fixes that. > > Natasha -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size 2026-08-18 8:31 ` Ricardo Ribalda 2026-08-18 9:40 ` Natasha Klaus @ 2026-08-18 10:32 ` Natasha Klaus 2026-08-18 10:40 ` Ricardo Ribalda 1 sibling, 1 reply; 14+ messages in thread From: Natasha Klaus @ 2026-08-18 10:32 UTC (permalink / raw) To: ribalda, noambs2999 Cc: laurent.pinchart, hansg, mchehab, linux-media, linux-kernel Understood on the placement, thanks. One finding while building your refactor: it does not compile as written. Dropping the uvc_dbg() removes the last use of alts in uvc_parse_frame(), and the kernel treats that as an error: drivers/media/usb/uvc/uvc_driver.c:233:36: error: unused variable 'alts' [-Werror=unused-variable] I removed the now-dead declaration, which adds a hunk you did not specify. Say if you would rather keep alts and identify the device explicitly in the dev_warn() instead. I also wrote the dev_warn() text, since you specified the level but not the wording: dev_warn(&streaming->intf->dev, "UVC non compliance: FRAME descriptor is %d bytes, expected at least %u.\n", buflen, 26 + 4 * n); Happy to change it. To carry your refactor as 1/3 with you as author I need your Signed-off-by. Could you send it, or tell me if you would rather I take authorship with a Suggested-by: line pointing at your message. Natasha ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size 2026-08-18 10:32 ` Natasha Klaus @ 2026-08-18 10:40 ` Ricardo Ribalda 0 siblings, 0 replies; 14+ messages in thread From: Ricardo Ribalda @ 2026-08-18 10:40 UTC (permalink / raw) To: Natasha Klaus Cc: noambs2999, laurent.pinchart, hansg, mchehab, linux-media, linux-kernel Hi Natasha On Tue, 18 Aug 2026 at 12:32, Natasha Klaus <natalie.klaus@runtimeverification.com> wrote: > > Understood on the placement, thanks. > > One finding while building your refactor: it does not compile as written. > Dropping the uvc_dbg() removes the last use of alts in uvc_parse_frame(), and > the kernel treats that as an error: > > drivers/media/usb/uvc/uvc_driver.c:233:36: error: unused variable 'alts' > [-Werror=unused-variable] > > I removed the now-dead declaration, which adds a hunk you did not specify. Say > if you would rather keep alts and identify the device explicitly in the > dev_warn() instead. > > I also wrote the dev_warn() text, since you specified the level but not the > wording: > > dev_warn(&streaming->intf->dev, > "UVC non compliance: FRAME descriptor is %d bytes, expected at least %u.\n", > buflen, 26 + 4 * n); I believe that I wrote :) : + dev_warn(&streaming->intf->dev, + "UVC non compliance: device %d videostreaming interface %d FRAME error\n", + dev->udev->devnum, alts->desc.bInterfaceNumber); But anyway... I think dev_warn with intf->dev is more than enough. I prefer your message. Thanks for that > > Happy to change it. > > To carry your refactor as 1/3 with you as author I need your Signed-off-by. > Could you send it, or tell me if you would rather I take authorship with a > Suggested-by: line pointing at your message. Suggested-by is more than enough. Thanks! > > Natasha -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation 2026-08-07 10:14 [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation Noam Ben Shimon 2026-08-10 14:12 ` Ricardo Ribalda 2026-08-12 10:32 ` [PATCH v2] " Noam Ben Shimon @ 2026-08-18 8:28 ` David Laight 2 siblings, 0 replies; 14+ messages in thread From: David Laight @ 2026-08-18 8:28 UTC (permalink / raw) To: Noam Ben Shimon Cc: laurent.pinchart, hansg, mchehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-08-18 10:40 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-07 10:14 [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation Noam Ben Shimon 2026-08-10 14:12 ` Ricardo Ribalda 2026-08-12 10:32 ` [PATCH v2] " Noam Ben Shimon 2026-08-18 6:45 ` Natasha Klaus 2026-08-18 6:54 ` Ricardo Ribalda 2026-08-18 6:57 ` Ricardo Ribalda 2026-08-18 7:59 ` [PATCH] media: uvcvideo: Skip frame descriptors with a zero computed size Natasha Klaus 2026-08-18 8:31 ` Ricardo Ribalda 2026-08-18 9:40 ` Natasha Klaus 2026-08-18 9:53 ` Natasha Klaus 2026-08-18 10:18 ` Ricardo Ribalda 2026-08-18 10:32 ` Natasha Klaus 2026-08-18 10:40 ` Ricardo Ribalda 2026-08-18 8:28 ` [PATCH] media: uvcvideo: Fix integer overflow in frame buffer size calculation David Laight
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.