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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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