[PATCH] media: meson: vdec: fix use-after-free of decode work in stop/close path

[PATCH] media: meson: vdec: fix use-after-free of decode work in stop/close path

[Doruk Tan Ozturk]

vdec_close() calls v4l2_m2m_ctx_release() and then kfree(sess) without
ever cancelling sess->esparser_queue_work. The worker
esparser_queue_all_src() takes sess->lock and walks the source buffers
of sess->m2m_ctx, so if it is still pending or running when the session
is torn down it dereferences freed memory.

The work is (re-)armed from several places, including amvdec_buf_done(),
which runs from the decode-completion/IRQ path. That makes the obvious
fixes insufficient:

  - v4l2_m2m_ctx_release() frees m2m_ctx (and runs stop_streaming via
    vb2_queue_release()), but never cancels the work. Cancelling in
    vdec_close() after v4l2_m2m_ctx_release() would wait on a worker that
    may already be dereferencing the now-freed m2m_ctx.

  - Cancelling in vdec_close() before v4l2_m2m_ctx_release() keeps
    m2m_ctx valid, but the hardware is still live, so amvdec_buf_done()
    can re-arm the work right after the cancel, reintroducing the UAF.

Cancel the work in vdec_stop_streaming() instead, right after
vdec_poweroff() has quiesced the hardware (so its IRQ can no longer
re-arm the work) and while sess->m2m_ctx is still valid. Because
v4l2_m2m_ctx_release() always tears the queues down through
vb2_queue_release() -> __vb2_queue_cancel() -> stop_streaming, this
single cancel covers both the STREAMOFF and the close paths.

This does not deadlock: the queue lock (sess->lock, shared by both vb2
queues) is taken by the worker, but neither the STREAMOFF path
(video_ioctl2 serialises on vdev->lock == core->lock, and
v4l2_m2m_streamoff() calls the lockless vb2_streamoff()) nor the close
path (vb2_queue_release()) holds sess->lock when stop_streaming runs, so
cancel_work_sync() can safely wait for the worker.

Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")
Cc: stable@vger.kernel.org
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
 drivers/staging/media/meson/vdec/vdec.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 4b77ec1af5a7..42822064cf8d 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -419,6 +419,16 @@ static void vdec_stop_streaming(struct vb2_queue *q)
 		sess->status = STATUS_STOPPED;
 	}
 
+	/*
+	 * The esparser_queue_work worker dereferences sess->m2m_ctx and
+	 * sess->lock. The hardware (and its IRQ, which re-arms the work via
+	 * amvdec_buf_done()) has been quiesced by vdec_poweroff() above, so
+	 * no new work can be scheduled past this point. m2m_ctx is still
+	 * valid here. Wait for any in-flight worker to finish before the
+	 * buffers and (on the close path) m2m_ctx are torn down.
+	 */
+	cancel_work_sync(&sess->esparser_queue_work);
+
 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
 			v4l2_m2m_buf_done(buf, VB2_BUF_STATE_ERROR);
-- 
2.43.0


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

Re: [PATCH] media: meson: vdec: fix use-after-free of decode work in stop/close path

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing IRQ synchronization allows a delayed threaded IRQ to re-arm the worker after `cancel_work_sync()` completes, leading to multiple Use-After-Free bugs.
- [High] `vdec_poweroff()` disables hardware clocks while the worker thread is actively executing or sleeping, leading to fatal unclocked register accesses.
- [High] Unchecked return value of `kthread_run()` leads to a kernel panic in `vdec_stop_streaming()`.
- [High] The V4L2 control handler `sess->ctrl_handler` is never freed, leaking memory on every device close.
- [High] The `sess->priv` structure is leaked if `vdec_start_streaming()` fails.
--

commit 80c252d67b7235edb63d6a1fcdbbdfe975a45e0c
Author: Doruk Tan Ozturk <doruk@0sec.ai>
Subject: media: meson: vdec: fix use-after-free of decode work in stop/close path

This commit attempts to fix a use-after-free condition by cancelling
esparser_queue_work during vdec_stop_streaming(). It places the
cancellation after the hardware is quiesced to ensure the worker is not
re-armed by new interrupts.

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index a039d925c0fe5..74b44f7f7ad7c 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -419,6 +419,16 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  		sess->status = STATUS_STOPPED;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does kthread_stop() handle
errors safely here?

Earlier in vdec_stop_streaming(), the code calls:

    kthread_stop(sess->recycle_thread);

In vdec_start_streaming(), sess->recycle_thread is initialized with
kthread_run(), but the return value isn't checked for ERR_PTR.

If thread creation fails, kthread_stop() will attempt to dereference the
invalid pointer, resulting in a fatal page fault.

>  
> +	/*
> +	 * The esparser_queue_work worker dereferences sess->m2m_ctx and
> +	 * sess->lock. The hardware (and its IRQ, which re-arms the work via
> +	 * amvdec_buf_done()) has been quiesced by vdec_poweroff() above, so

[Severity: High]
This is a pre-existing issue, but does vdec_poweroff() safely quiesce the
hardware while the worker is still running?

vdec_stop_streaming() calls vdec_poweroff() before we cancel the work.
The worker thread esparser_queue_all_src() can block in esparser_write_data()
for up to 200ms.

Since vdec_poweroff() disables hardware clocks without holding sess->lock,
the worker could wake up and attempt to write to PARSER_FETCH_CMD while
clocks are disabled. Can this cause an asynchronous external abort and panic?

> +	 * no new work can be scheduled past this point. m2m_ctx is still
> +	 * valid here. Wait for any in-flight worker to finish before the
> +	 * buffers and (on the close path) m2m_ctx are torn down.
> +	 */
> +	cancel_work_sync(&sess->esparser_queue_work);

[Severity: Critical]
This is a pre-existing issue, but is cancel_work_sync() sufficient without
synchronizing IRQs first?

While vdec_poweroff() stops new hardware IRQs, it does not call
synchronize_irq().

If the threaded IRQ handler (vdec_threaded_isr) is already scheduled and
delayed, it can execute concurrently or after this cancellation.
The ISR would then call dst_buf_done(), which invokes:

    schedule_work(&sess->esparser_queue_work);

Could this re-arm the worker after cancel_work_sync() has completed,
leading to a second use-after-free when the worker later dereferences
the freed sess->m2m_ctx?

[ ... ]

Additionally, I noticed a couple of other pre-existing issues in this file:

[Severity: High]
This is a pre-existing issue, but does vdec_close() leak the V4L2 control
handler?

vdec_open() initializes sess->ctrl_handler via vdec_init_ctrls() and
v4l2_ctrl_handler_init(), allocating memory for the controls.

However, vdec_close() does not free it:

vdec_close() {
    ...
    v4l2_m2m_ctx_release(sess->m2m_ctx);
    v4l2_m2m_release(sess->m2m_dev);
    v4l2_fh_del(&sess->fh, file);
    v4l2_fh_exit(&sess->fh);
    ...
}

Since the V4L2 core does not automatically free control handlers during
v4l2_fh_exit(), is this memory leaked every time a device is closed?

[Severity: High]
This is a pre-existing issue, but does the error path in
vdec_start_streaming() leak sess->priv?

In vdec_start_streaming(), vdec_poweron() allocates sess->priv via
load_extended_firmware() (for example, allocating struct codec_h264).

If a subsequent initialization step fails, the error path calls the codec's
stop routine:

vdec_start_streaming() {
    ...
    ret = vdec_poweron(sess);
    if (ret)
        goto vififo_free;
    ...
}

The stop routine (e.g. codec_h264_stop) cleans up DMA memory but fails to
kfree() the codec structure itself.

Because the stream never starts, vdec_stop_streaming() is never invoked.
Does this leave sess->priv permanently leaked?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615140529.52653-1-doruk@0sec.ai?part=1

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