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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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