Re: [PATCH v7 03/19] media: meson: vdec: Fix lifecycle leaks and race conditions in recycle_thread

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] vdec_close() frees session memory before releasing the V4L2 M2M context, triggering a NULL pointer dereference during queue teardown.
- [Critical] Unconditional execution of vdec_poweroff() and clearing of core->cur_sess in vdec_close() causes Data Abort panics and cross-session corruption.
- [Critical] Unconditional execution of vdec_poweroff() in vdec_close() leads to a double free of codec DMA allocations.
- [High] vdec_close() prematurely frees session context and DMA buffers without canceling background workqueues, causing Use-After-Free and DMA corruption.

Pre-existing issues:
- [High] Flawed logic in vdec_stop_streaming() orphans the recycle thread, leading to Use-After-Free during partial STREAMOFF.
- [Medium] The err_poweroff unwinding path in vdec_start_streaming() leaks sess->priv and associated codec assets.
--

--- Patch [3]: [PATCH v7 03/19] media: meson: vdec: Fix lifecycle leaks and race conditions in recycle_thread ---
commit 41dac138c8515aa623b248b4a110d8a7802b604b
Author: Anand Moon <linux.amoon@gmail.com>
Subject: media: meson: vdec: Fix lifecycle leaks and race conditions in recycle_thread

This patch validates the kthread_run return status during recycling thread
initialization to prevent memory leaks. It also ensures the thread is safely
stopped in both vdec_stop_streaming and vdec_close to prevent kernel panics
during teardown.

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
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
[ ... ]
@@ -353,13 +357,23 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	schedule_work(&sess->esparser_queue_work);
>  	return 0;
>  
>  err_poweroff:
>  	vdec_poweroff(sess);
>  	vdec_free_canvas(sess);
>  	vdec_reset_timestamps(sess);
>  	vdec_reset_bufs_recycle(sess);

[Severity: Medium]
This is a pre-existing issue, but does the err_poweroff unwinding path leak
sess->priv?

If kthread_run() fails, execution jumps to the err_poweroff label which
unwinds various hardware states but misses calling kfree(sess->priv).
When the application retries VIDIOC_STREAMON, the codec's start routine
unconditionally allocates a new structure over sess->priv, permanently
stranding the previous allocation.

>  err_free_vififo:
>  	if (sess->vififo_vaddr) {
>  		dma_free_coherent(sess->core->dev, sess->vififo_size,
[ ... ]
@@ -464,7 +478,10 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  	if (full_cleanup) {
>  		if ((q->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ||
>  		     !sess->streamon_out) && vdec_codec_needs_recycle(sess)) {

[Severity: High]
This is a pre-existing issue, but does this logic orphan the recycle thread
leading to a use-after-free during partial STREAMOFF?

If userspace invokes VIDIOC_STREAMOFF exclusively on the capture queue
while the output queue remains active, this condition evaluates to false,
bypassing the kthread_stop() call.

Because old_status == STATUS_RUNNING, full_cleanup evaluates to true,
proceeding to dismantle the session context and free sess->priv.
The orphaned recycle thread may concurrently awaken and attempt to access
this freed memory.

> -			kthread_stop(sess->recycle_thread);
> +			if (!IS_ERR_OR_NULL(sess->recycle_thread)) {
> +				kthread_stop(sess->recycle_thread);
> +				sess->recycle_thread = NULL;
> +			}
>  		}
[ ... ]
@@ -970,6 +987,31 @@ static int vdec_open(struct file *file)
>  static int vdec_close(struct file *file)
>  {
>  	struct amvdec_session *sess = file_to_amvdec_session(file);
> +	struct amvdec_core *core = sess->core;
> +
> +	if (!IS_ERR_OR_NULL(sess->recycle_thread)) {
> +		kthread_stop(sess->recycle_thread);
> +		sess->recycle_thread = NULL;
> +	}
> +
> +	mutex_lock(&core->lock);
> +
> +	vdec_poweroff(sess);
> +	vdec_free_canvas(sess);
> +	core->cur_sess = NULL;

[Severity: Critical]
Can this unconditional invocation of vdec_poweroff() and clearing of
core->cur_sess cause Data Abort panics and cross-session corruption?

If userspace opens two instances (FDs), starts streaming on instance A
(acquiring hardware ownership), and then closes instance B:

vdec_close() for instance B will unconditionally call vdec_poweroff(sess)
and clear core->cur_sess, without verifying if instance B actually owns the
hardware.

This forces clock disablements while instance A is actively decoding. When
the hardware for instance A fires an interrupt, vdec_isr() will dereference
core->cur_sess, which was just wrongfully set to NULL, causing a panic.

[Severity: Critical]
Will this unconditional vdec_poweroff() call also lead to a double free of
codec DMA allocations?

If userspace cleanly shuts down streaming via VIDIOC_STREAMOFF,
vdec_stop_streaming() executes vdec_poweroff(). This invokes the codec
stop callback (like codec_h264_stop) which frees memory buffers but does
not set their pointers to NULL.

When userspace subsequently calls close(), vdec_close() calls
vdec_poweroff() again. The codec stop routine will execute a second time,
passing the same non-NULL pointers to dma_free_coherent() resulting in a
catastrophic double-free.

> +
> +	if (sess->vififo_vaddr) {
> +		dma_free_coherent(core->dev, sess->vififo_size,
> +				  sess->vififo_vaddr, sess->vififo_paddr);
> +		sess->vififo_vaddr = NULL;
> +		sess->vififo_paddr = 0;
> +	}
> +	vdec_reset_timestamps(sess);
> +	vdec_reset_bufs_recycle(sess);
> +	kfree(sess->priv);

[Severity: High]
Should we call cancel_work_sync(&sess->esparser_queue_work) before freeing
the session context and DMA buffers here?

If vdec_close() is called while the background workqueue is actively
processing frames, the DMA buffer (sess->vififo_vaddr) and session
structure (sess->priv) are immediately freed.

The concurrently executing workqueue continues its operation inside
esparser_queue(), dereferencing the freed sess->priv pointer and
instructing the hardware to perform DMA transfers into the freshly freed
physical address, potentially causing severe memory corruption.

> +	sess->priv = NULL;
> +
> +	mutex_unlock(&core->lock);
>  
>  	v4l2_m2m_ctx_release(sess->m2m_ctx);

[Severity: Critical]
Does this sequence trigger a NULL pointer dereference during queue teardown?

If a user closes the file descriptor without calling VIDIOC_STREAMOFF,
vdec_close() will free sess->priv and set it to NULL.

Immediately after, it calls v4l2_m2m_ctx_release(), which triggers
vdec_stop_streaming(). Since sess->status is still STATUS_RUNNING, the
driver will execute codec_ops->drain(sess).

The codec's drain function unconditionally dereferences sess->priv, which
was just set to NULL, causing an immediate panic.

>  	v4l2_fh_del(&sess->fh, file);

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

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