Re: [PATCH] usb: gadget: uvc: reserve frame interval headroom for the zero length backlog

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: gadget: uvc: Divide by zero in uvc_video_prep_requests()
- [High] usb: gadget: uvc: NULL pointer dereference if sg_alloc_table fails
--

commit 405aa67c114665f2574cee699b4f2eabe9b5de64
Author: Daehwan Jung <dh10.jung@samsung.com>

usb: gadget: uvc: reserve frame interval headroom for the zero length backlog

This commit modifies uvc_video_prep_requests() to reserve frame interval
headroom for zero length backlog requests. It achieves this by subtracting
headroom from nreq to ensure the processing budget can be met.

> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 9ba09118bb74a..90882c88ddc6a 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -521,6 +521,20 @@ uvc_video_prep_requests(struct uvc_video *video)
>  
>  	nreq = DIV_ROUND_UP(video->interval, interval_duration);
>  
> +	/*
> +	 * Not all of those intervals can carry payload: the zero length
> +	 * requests uvcg_video_hw_submit() queues while it waits for a buffer
> +	 * have to drain first, and uvc_buffer_prepare() splits a buffer into
> +	 * exactly reqs_per_frame requests regardless. Reserve them, unless
> +	 * that would claim more than half of the frame interval - a frame
> +	 * interval that short cannot absorb the backlog either way, and
> +	 * nreq - headroom would underflow.
> +	 */
> +	headroom = max_t(unsigned int, nreq / UVCG_REQ_HEADROOM_FRACT,
> +			 UVCG_REQ_HEADROOM_COUNT);
> +	if (nreq > 2 * headroom)
> +		nreq -= headroom;
> +
>  	header_size = nreq * UVCG_REQUEST_HEADER_LEN;
>  
>  	req_size = DIV_ROUND_UP(video->imagesize + header_size, nreq);

[Severity: High]
Does this code trigger a divide by zero if video->interval is 0?

Userspace can set a timeperframe numerator of 0 via VIDIOC_S_PARM, which
uvc_v4l2_s_parm() converts without bounds checking:

drivers/usb/gadget/function/uvc_v4l2.c:uvc_v4l2_s_parm() {
    ...
    video->interval = v4l2_fraction_to_interval(timeperframe.numerator,
                                                timeperframe.denominator);
    ...
}

If video->interval is 0, nreq will evaluate to 0 in uvc_video_prep_requests().
Since 0 > 2 * headroom evaluates to false, nreq remains 0. The subsequent
DIV_ROUND_UP() calculation will then attempt to divide by zero, causing a
kernel panic.

[Severity: High]
Can uvc_video_encode_isoc_sg() oops if sg_alloc_table() fails?

In uvc_video_alloc_requests(), the return value of sg_alloc_table() is
unchecked:

drivers/usb/gadget/function/uvc_video.c:uvc_video_alloc_requests() {
    ...
    sg_alloc_table(&ureq->sgt,
                   DIV_ROUND_UP(video->req_size - UVCG_REQUEST_HEADER_LEN,
                                PAGE_SIZE) + 2, GFP_KERNEL);
    ...
}

If the allocation fails, ureq->sgt.sgl remains NULL and nents is 0, but the
request is incorrectly considered valid and placed on the free list.

When queuing video buffers with video->queue.use_sg active, this request
will be used by uvc_video_encode_isoc_sg():

drivers/usb/gadget/function/uvc_video.c:uvc_video_encode_isoc_sg() {
    ...
    sg = ureq->sgt.sgl;
    sg_init_table(sg, ureq->sgt.nents);
    ...
}

This calls sg_init_table(NULL, 0), which eventually invokes
sg_mark_end(&sgl[nents - 1]). With nents = 0, this results in an
out-of-bounds dereference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012034018.3193146-1-dh10.jung@samsung.com?part=1