Re: [PATCH] media: uvcvideo: skip bulk output URBs smaller than the payload header

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

On Tue, Oct 13, 2026 at 05:53:55PM +0800, Ricardo Ribalda wrote:
> HI Louis
> 
> Thanks for your patch
> 
> On Tue, 13 Oct 2026 at 14:56, Louis Clinckx <clinckx.louis@gmail.com> wrote:
> >
> > For a bulk video output stream, uvc_video_encode_bulk() starts each
> > payload by writing a fixed 2-byte header with uvc_video_encode_header()
> > and subtracts it from a signed length counter initialised to the URB
> > size:
> 
> Out of curiosity, do you have a UVC output device? I would really like
> to test one out, but I could not find anything on the market.
> 
> >
> >         int len = stream->urb_size;
> >         ...
> >         len -= uvc_video_encode_header(...);    /* always returns 2 */
> >
> > An output device can describe a bulk endpoint with wMaxPacketSize 1 and
> > negotiate dwMaxPayloadTransferSize 1, which leaves stream->urb_size == 1.
> > len then becomes -1, and uvc_video_encode_data() casts it to unsigned:
> >
> >         nbytes = min((unsigned int)len, buf->bytesused - queue->buf_used);
> >
> > so (unsigned)-1 does not clamp and the following memcpy() copies the
> > whole frame past the one-byte URB buffer.
> >
> > A URB that cannot hold the 2-byte header cannot carry a payload, so skip
> > it instead of letting the length underflow.
> >
> > Signed-off-by: Louis Clinckx <clinckx.louis@gmail.com>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 7 +++++++
> >  1 file changed, 7 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
> > @@ -1701,7 +1701,14 @@ static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
> >         if (buf == NULL) {
> >                 urb->transfer_buffer_length = 0;
> >                 return;
> >         }
> >
> > +       /* The 2-byte payload header must fit; a smaller URB would make len
> > +        * underflow and uvc_video_encode_data() copy past the buffer. */
> > +       if (len < 2) {
> > +               urb->transfer_buffer_length = 0;
> > +               return;
> > +       }
> > +
> >         /* If the URB is the first of its payload, add the header. */
> >         if (stream->bulk.header_size == 0) {
>
> I'd also detect that the configuration is broken, maybe in
> uvc_init_video_bulk() ?

There's indeed no point in silently and unconditionally failing to
encode data when the issue can be detected when starting the stream (or
even better, at probe time). Thisis what the next version should do.

> and a bit nitpick. I think this is slightly more idiomatic than your check.
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index ec76595f3c4b..03940b62b250 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1373,6 +1373,8 @@ static void uvc_video_decode_end(struct uvc_streaming *stream,
>  static int uvc_video_encode_header(struct uvc_streaming *stream,
>                 struct uvc_buffer *buf, u8 *data, int len)
>  {
> +       if (len < 2)
> +               return -ENOSPC;

If we reject invalid configurations this won't be necessary any more.

>         data[0] = 2;    /* Header length */
>         data[1] = UVC_STREAM_EOH | UVC_STREAM_EOF
>                 | (stream->last_fid & UVC_STREAM_FID);
> @@ -1656,6 +1658,10 @@ static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
>         /* If the URB is the first of its payload, add the header. */
>         if (stream->bulk.header_size == 0) {
>                 ret = uvc_video_encode_header(stream, buf, mem, len);
> +               if (ret < 0) {
> +                       urb->transfer_buffer_length = 0;
> +                       return;
> +               }
>                 stream->bulk.header_size = ret;
>                 stream->bulk.payload_size += ret;
>                 mem += ret;

-- 
Regards,

Laurent Pinchart