Re: [PATCH v7 07/19] media: meson: vdec: Refactor esparser work queue and fix teardown race

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The worker thread enters a 100% CPU infinite loop when the queue is empty, full, or hits an EOS buffer, and prematurely exits after a single frame on success due to an inverted loop condition and misuse of `scoped_guard`.

Pre-existing issues:
- [Critical] Maliciously crafted VP9 superframe headers can cause an integer overflow in `total_datasize`, bypassing bounds checks and triggering a massive out-of-bounds `memmove()`.
--

--- Patch [7]: [PATCH v7 07/19] media: meson: vdec: Refactor esparser work queue and fix teardown race ---
commit f4cf4afadbba1cd9ed8438a311d676dabd768d33
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Refactor esparser work queue and fix teardown race

Rework the esparser work queue logic to prevent concurrent queue mutations
and fix a race condition during streaming teardown.

> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> index 37749ede308c6..e5d3d817b9b2b 100644
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c

[ ... ]

> @@ -348,12 +344,10 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>  	if (sess->fmt_out->pixfmt == V4L2_PIX_FMT_VP9) {
>  		payload_size = vp9_update_header(core, vb);
>  
> -		/* If unable to alter buffer to add headers */
>  		if (payload_size == 0) {
> +			dev_err(core->dev, "esparser: VP9 header update failed\n");
>  			amvdec_remove_ts(sess, vb->timestamp);
> -			v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> -
> -			return 0;
> +			return -EBADMSG;
>  		}
>  	}

[Severity: Critical]
This is a pre-existing issue, but can maliciously crafted VP9 superframe
headers cause an integer overflow in vp9_update_header(), leading to a
massive out-of-bounds memory copy?

Looking at vp9_update_header() in the same file:

    for (cur_mag = 0; cur_mag < mag; cur_mag++) {
        ...
        frame_size[cur_frame] |=
            (dp[mag_ptr] << (cur_mag * 8));
        mag_ptr++;
    }
    ...
    total_datasize += frame_size[cur_frame];

If untrusted user data shifted into the signed 32-bit frame_size becomes
negative, multiple negative values can sum to a small positive
total_datasize, which bypasses the size checks:

    new_frame_size = total_datasize + num_frames * VP9_HEADER_SIZE;
    if (new_frame_size >= vb2_plane_size(buf, 0)) {
        return -ENOMEM;
    }

Then, later in the same function:

    int framesize = frame_size[cur_frame];
    ...
    memmove(fdata + VP9_HEADER_SIZE, old_framedata, framesize);

The negative framesize is implicitly converted to a size_t, which causes
an out-of-bounds memmove(). Could this lead to memory corruption when
handling untrusted V4L2 video stream data?

[ ... ]

>  void esparser_queue_all_src(struct work_struct *work)
>  {
> -	struct v4l2_m2m_buffer *buf, *n;
>  	struct amvdec_session *sess =
>  		container_of(work, struct amvdec_session, esparser_queue_work);
> +	struct device *dev = sess->core->dev_dec;
> +	int ret;
>  
> -	mutex_lock(&sess->lock);
> -	v4l2_m2m_for_each_src_buf_safe(sess->m2m_ctx, buf, n) {
> -		if (sess->should_stop)
> -			break;
> +	while (1) {
> +		struct vb2_v4l2_buffer *vbuf = NULL;
> +		bool processed_frame = false;
> +
> +		scoped_guard(mutex, &sess->lock) {
> +			/* Safe atomic tracking check: exit loop if session is shutting down */
> +			if (sess->should_stop)
> +				return;
> +
> +			/* Queue completely empty: exit work loop cleanly */
> +			vbuf = v4l2_m2m_next_src_buf(sess->m2m_ctx);
> +			if (!vbuf)
> +				break;
> +
> +			/* Stop processing if we hit the end-of-stream drain buffer */
> +			if (vbuf->flags & V4L2_BUF_FLAG_LAST)
> +				break;
> +
> +			/* Check hardware FIFO limits safely inside the locks */
> +			ret = esparser_queue(sess, vbuf);
> +			if (ret == -EAGAIN)
> +				break;
> +
> +			/* Pop the buffer from the source queue since it is now processed */
> +			vbuf = v4l2_m2m_src_buf_remove(sess->m2m_ctx);
> +			if (!vbuf) {
> +				dev_dbg(dev, "Buffer missing during queue removal\n");
> +				break;
> +			}
> +
> +			/* Complete the buffer transaction based on parser results */
> +			if (ret < 0)
> +				v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> +			else
> +				v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE);
>  
> -		if (esparser_queue(sess, &buf->vb) < 0)
> +			/* Set tracking flag indicating transaction completion */
> +			processed_frame = true;
> +		}
> +
> +		if (processed_frame)
>  			break;

[Severity: High]
Does this code inadvertently create an infinite loop?

Because scoped_guard() is implemented internally as a for-loop, the
break statements inside the scoped_guard() block will only exit the lock
scope, not the outer while (1) loop.

If the queue is empty (!vbuf), we break out of the scoped_guard(),
processed_frame remains false, and the while (1) loop continues infinitely,
consuming 100% CPU.

Conversely, if a frame is successfully processed, processed_frame becomes
true, which causes the outer while (1) loop to exit after just one frame
instead of continuing to drain the queue.

Could the logic here be inverted, or should the inner loop use goto
instead of break to exit the outer loop?

> +
> +		/* Give other threads and IRQ routines a window to execute while unlocked */
> +		cond_resched();
>  	}
> -	mutex_unlock(&sess->lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713120840.17427-1-linux.amoon@gmail.com?part=7

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic