[PATCH v2] media: uvcvideo: reject bulk output payloads too small for the header

[PATCH v2] media: uvcvideo: reject bulk output payloads too small for the header

[Louis Clinckx]

A bulk video output stream prefixes each payload with a fixed 2-byte
header written by uvc_video_encode_header(). The per-URB length counter
in uvc_video_encode_bulk() is a signed int initialised to the URB size:

	int len = stream->urb_size;
	...
	len -= uvc_video_encode_header(...);	/* returns 2 */

An output device can describe a bulk endpoint with wMaxPacketSize 1 and
negotiate dwMaxPayloadTransferSize 1, leaving stream->urb_size == 1. len
then becomes -1, and uvc_video_encode_data() casts it to unsigned
(nbytes = min((unsigned int)len, ...)), so (unsigned)-1 defeats the
clamp and the following memcpy() copies the whole frame past the
one-byte URB buffer.

Rather than guard the encode path, reject the invalid configuration when
the stream is started: the negotiated maximum payload transfer size must
be able to hold the 2-byte payload header. Requiring size >= 2 also
guarantees urb_size = psize * npackets >= 2, so the encode path can no
longer underflow.

Signed-off-by: Louis Clinckx <clinckx.louis@gmail.com>
---
v1 -> v2: per review (Ricardo Ribalda, Laurent Pinchart), reject the bad
          configuration at stream start in uvc_init_video_bulk() instead
          of adding a guard to the encode path. Can move the check to
          probe/commit (uvc_fixup_video_ctrl) if preferred.

 drivers/media/usb/uvc/uvc_video.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1942,6 +1942,16 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
 	size = stream->ctrl.dwMaxPayloadTransferSize;
 	stream->bulk.max_payload_size = size;

+	/*
+	 * Output payloads start with a 2-byte header (see
+	 * uvc_video_encode_header()), so a device negotiating a maximum
+	 * payload transfer size smaller than that would make the encode
+	 * path underflow its signed length counter. Reject it here.
+	 */
+	if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
+	    stream->bulk.max_payload_size < 2)
+		return -EINVAL;
+
 	npackets = uvc_alloc_urb_buffers(stream, size, psize, gfp_flags);
 	if (npackets == 0)
 		return -ENOMEM;

Re: [PATCH v2] media: uvcvideo: reject bulk output payloads too small for the header

[Ricardo Ribalda]

HI Louis

On Tue, 13 Oct 2026 at 19:53, Louis Clinckx <clinckx.louis@gmail.com> wrote:
>
> A bulk video output stream prefixes each payload with a fixed 2-byte
> header written by uvc_video_encode_header(). The per-URB length counter
> in uvc_video_encode_bulk() is a signed int initialised to the URB size:
>
>         int len = stream->urb_size;
>         ...
>         len -= uvc_video_encode_header(...);    /* returns 2 */
>
> An output device can describe a bulk endpoint with wMaxPacketSize 1 and
> negotiate dwMaxPayloadTransferSize 1, leaving stream->urb_size == 1. len
> then becomes -1, and uvc_video_encode_data() casts it to unsigned
> (nbytes = min((unsigned int)len, ...)), so (unsigned)-1 defeats the
> clamp and the following memcpy() copies the whole frame past the
> one-byte URB buffer.
>
> Rather than guard the encode path, reject the invalid configuration when
> the stream is started: the negotiated maximum payload transfer size must
> be able to hold the 2-byte payload header. Requiring size >= 2 also
> guarantees urb_size = psize * npackets >= 2, so the encode path can no
> longer underflow.
>
> Signed-off-by: Louis Clinckx <clinckx.louis@gmail.com>
> ---
> v1 -> v2: per review (Ricardo Ribalda, Laurent Pinchart), reject the bad
>           configuration at stream start in uvc_init_video_bulk() instead
>           of adding a guard to the encode path. Can move the check to
>           probe/commit (uvc_fixup_video_ctrl) if preferred.
>
>  drivers/media/usb/uvc/uvc_video.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1942,6 +1942,16 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
>         size = stream->ctrl.dwMaxPayloadTransferSize;
>         stream->bulk.max_payload_size = size;
>
> +       /*
> +        * Output payloads start with a 2-byte header (see
> +        * uvc_video_encode_header()), so a device negotiating a maximum
> +        * payload transfer size smaller than that would make the encode
> +        * path underflow its signed length counter. Reject it here.
> +        */
> +       if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> +           stream->bulk.max_payload_size < 2)
> +               return -EINVAL;

nit: I'd replace the 2 with a #define and use it also in
uvc_video_encode_header() for the return



And I know that Laurent said otherwise, but probably due to my rust
intoxication, I still recommend applying the check that I recommended
in the previous version. It is pretty cheap. Otherwise If the size is
not important, why do we pass it to uvc_video_encode_header()?

In fact I think that if usb_endpoint_maxp()==1 and
stream->bulk.max_payload_size==2 with memory pressure your code could
fails

> +
>         npackets = uvc_alloc_urb_buffers(stream, size, psize, gfp_flags);
>         if (npackets == 0)
>                 return -ENOMEM;
>

ps: Sorry to repeat myself; this isn't related to accepting or not the
patch... Do you have a usb bulk output "camera" . I want one for my
collection :)
-- 
Ricardo Ribalda

Re: [PATCH v2] media: uvcvideo: reject bulk output payloads too small for the header

[Laurent Pinchart]

On Tue, Oct 13, 2026 at 08:40:07PM +0800, Ricardo Ribalda wrote:
> On Tue, 13 Oct 2026 at 19:53, Louis Clinckx wrote:
> >
> > A bulk video output stream prefixes each payload with a fixed 2-byte
> > header written by uvc_video_encode_header(). The per-URB length counter
> > in uvc_video_encode_bulk() is a signed int initialised to the URB size:
> >
> >         int len = stream->urb_size;
> >         ...
> >         len -= uvc_video_encode_header(...);    /* returns 2 */
> >
> > An output device can describe a bulk endpoint with wMaxPacketSize 1 and
> > negotiate dwMaxPayloadTransferSize 1, leaving stream->urb_size == 1. len
> > then becomes -1, and uvc_video_encode_data() casts it to unsigned
> > (nbytes = min((unsigned int)len, ...)), so (unsigned)-1 defeats the
> > clamp and the following memcpy() copies the whole frame past the
> > one-byte URB buffer.
> >
> > Rather than guard the encode path, reject the invalid configuration when
> > the stream is started: the negotiated maximum payload transfer size must
> > be able to hold the 2-byte payload header. Requiring size >= 2 also
> > guarantees urb_size = psize * npackets >= 2, so the encode path can no
> > longer underflow.
> >
> > Signed-off-by: Louis Clinckx <clinckx.louis@gmail.com>
> > ---
> > v1 -> v2: per review (Ricardo Ribalda, Laurent Pinchart), reject the bad
> >           configuration at stream start in uvc_init_video_bulk() instead
> >           of adding a guard to the encode path. Can move the check to
> >           probe/commit (uvc_fixup_video_ctrl) if preferred.
> >
> >  drivers/media/usb/uvc/uvc_video.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1942,6 +1942,16 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
> >         size = stream->ctrl.dwMaxPayloadTransferSize;
> >         stream->bulk.max_payload_size = size;
> >
> > +       /*
> > +        * Output payloads start with a 2-byte header (see
> > +        * uvc_video_encode_header()), so a device negotiating a maximum
> > +        * payload transfer size smaller than that would make the encode
> > +        * path underflow its signed length counter. Reject it here.
> > +        */
> > +       if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> > +           stream->bulk.max_payload_size < 2)
> > +               return -EINVAL;
> 
> nit: I'd replace the 2 with a #define and use it also in
> uvc_video_encode_header() for the return
> 
> And I know that Laurent said otherwise, but probably due to my rust
> intoxication, I still recommend applying the check that I recommended
> in the previous version.

Let's revisit that topic once the uvcvideo driver gets rewritten in rust
:-)

> It is pretty cheap. Otherwise If the size is
> not important, why do we pass it to uvc_video_encode_header()?

We can drop it.

> In fact I think that if usb_endpoint_maxp()==1 and
> stream->bulk.max_payload_size==2 with memory pressure your code could
> fails

The patch seems entirely wrong, checking max_payload_size only doesn't
make sense. And setting the limit to 2 also makes no sense, there would
be no space to encode any data.

> > +
> >         npackets = uvc_alloc_urb_buffers(stream, size, psize, gfp_flags);
> >         if (npackets == 0)
> >                 return -ENOMEM;
> >
> 
> ps: Sorry to repeat myself; this isn't related to accepting or not the
> patch... Do you have a usb bulk output "camera" . I want one for my
> collection :)

I doubt that. I've only seen one in the wild.

This patch started as a private LLM-generated security report. You can't
expect it to be tested on hardware, or even tested at all, and I have no
confidence that the author understands what they're doing.

-- 
Regards,

Laurent Pinchart