Re: [PATCH v6 4/8] media: meson: vdec: Condition buffer flushing on queue type in start_streaming

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Double-free of codec DMA buffers in the error path of `vdec_start_streaming`.
- [Critical] During a dynamic resolution change, STREAMOFF bypasses hardware cleanup and leads to a critical Use-After-Free upon session closure.

Pre-existing issues:
- [High] The `sess->priv` context and codec DMA memory are permanently leaked if `vdec_poweron(sess)` fails.
--

commit 8f59207c5628689c8af221e3f815a3651ccc803f
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Condition buffer flushing on queue type in start_streaming

When vdec_start_streaming() fails, the error path clears buffers from both
the source and destination queues unconditionally. If one queue was already
streaming successfully from a prior invocation, flushing its buffers behind
its back leaves videobuf2 deadlocked waiting for completions.

Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 8a5bf1a96830c..698a95566ad27 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -386,15 +386,15 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	sess->status = STATUS_STOPPED;
>  	mutex_unlock(&core->lock);

[Severity: Critical]
Can this error path cause a double-free of the codec DMA buffers?

Looking at the error handling above this diff in vdec_start_streaming():

drivers/staging/media/meson/vdec/vdec.c:vdec_start_streaming() {
    [ ... ]
err_cleanup:
	vdec_free_canvas(sess);
	vdec_poweroff(sess);
	if (codec_ops && codec_ops->stop && sess->priv) {
		codec_ops->stop(sess);
    [ ... ]
}

When vdec_poweroff(sess) is called, it cascades to __vdec_1_stop(), which
calls codec_ops->stop(sess). The cleanup block then explicitly calls
codec_ops->stop(sess) a second time.

If the codec stop function (such as codec_h264_stop) frees the DMA memory
allocations without nullifying the pointers, could passing the exact same DMA
addresses to dma_free_coherent() a second time result in a double-free and
heap corruption?


[Severity: High]
This is a pre-existing issue, but does this bypass the cleanup of the session
context and permanently leak memory?

During vdec_poweron(sess), the driver allocates the sess->priv context and
codec DMA buffers. If a subsequent initialization step fails, vdec_poweron
propagates the error, and vdec_start_streaming() jumps directly to
vififo_free:

drivers/staging/media/meson/vdec/vdec.c:vdec_start_streaming() {
    [ ... ]
	ret = vdec_poweron(sess);
	if (ret)
		goto vififo_free;
    [ ... ]
err_cleanup:
    [ ... ]
		kfree(sess->priv);
    [ ... ]
vififo_free:
    [ ... ]
bufs_done:
    [ ... ]
	sess->status = STATUS_STOPPED;
}

This bypasses the err_cleanup: label and the kfree(sess->priv) cleanup.
Because the session status is then explicitly set to STATUS_STOPPED right
here at bufs_done, won't later calls to vdec_stop_streaming() evaluate
old_status == STATUS_STOPPED and skip the entire cleanup block, permanently
leaking the memory?


[Severity: Critical]
Does the newly introduced locking fix in vdec_stop_streaming() lead to a
Use-After-Free during a dynamic resolution change?

When the decoder detects a resolution change, it sets sess->status to
STATUS_NEEDS_RESUME. When userspace handles this by calling STREAMOFF on the
capture queue (the first call), vdec_stop_streaming() unconditionally sets
the status to STATUS_STOPPED at the start of the function:

drivers/staging/media/meson/vdec/vdec.c:vdec_stop_streaming() {
    [ ... ]
	mutex_lock(&core->lock);
	old_status = sess->status;
    [ ... ]
	sess->status = STATUS_STOPPED;
	mutex_unlock(&core->lock);

	if (old_status == STATUS_RUNNING ||
	    old_status == STATUS_INIT ||
	    (old_status == STATUS_NEEDS_RESUME &&
	     (!sess->streamon_out || !sess->streamon_cap))) {
    [ ... ]
}

Because sess->streamon_cap is only cleared at the end of the function, both
streams are still active on the first STREAMOFF, so the hardware cleanup
condition evaluates to false. 

When the second STREAMOFF is called on the output queue, old_status is now
STATUS_STOPPED, so cleanup is skipped again. Since cleanup is completely
bypassed, vififo memory and the recycle_thread are leaked.

If the user then calls STREAMON, won't it spawn a duplicate recycle_thread,
overwriting the old pointer? When the session is eventually closed, could
vdec_close() free the session structure while the orphaned thread continues
executing, resulting in a Use-After-Free?

> -	while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
> -		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> -	while ((buf = v4l2_m2m_dst_buf_remove(sess->m2m_ctx)))
> -		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> -
> -	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		sess->streamon_out = 0;
> -	else
> +		while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
> +			v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> +	} else {
>  		sess->streamon_cap = 0;
> +		while ((buf = v4l2_m2m_dst_buf_remove(sess->m2m_ctx)))
> +			v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> +	}
>  
>  	return ret;
>  }

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

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