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

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

[Doruk Tan Ozturk]

The ESPARSER worker (esparser_queue_all_src(), scheduled via
sess->esparser_queue_work) dereferences sess->m2m_ctx and accesses the
ESPARSER/DOS registers. On the stop_streaming()/release() paths the
buffers and, on close, the m2m context are torn down while this worker
may still be pending or running, leading to a use-after-free of the
freed session state.

vdec_poweroff() previously only called vdec_ops->stop() and disabled the
clocks; it never synchronized against the worker. Two problems follow:

  - The decode (VDEC) interrupt is threaded
    (devm_request_threaded_irq(.., vdec_isr, vdec_threaded_isr, ..)) and
    its threaded handler re-arms the worker through amvdec_dst_buf_done()
    -> schedule_work(&sess->esparser_queue_work). A handler that is still
    in flight can therefore queue the worker again after teardown has
    begun.

  - The worker touches ESPARSER/DOS registers, so it must not run after
    the clocks have been disabled.

Quiesce everything in vdec_poweroff(), in order, before disabling the
clocks: vdec_ops->stop() masks the VDEC interrupt in hardware so no new
IRQ can be raised; synchronize_irq() on the VDEC line then drains any
threaded handler still in flight (the only context that re-arms the
worker); cancel_work_sync() finally cancels/waits for the worker. After
this nothing can re-arm the work, and the worker can no longer run with
clocks disabled or against a freed m2m context.

Only the VDEC interrupt is synchronized: the ESPARSER interrupt handler
(esparser_isr()) only acknowledges the start-code-found status and wakes
the internal wait queue used by esparser_write_data(); it never touches
the session or schedules the worker, so it cannot re-arm it.

stop_streaming()/release() run under the video device lock, not under
sess->lock (the mutex the worker takes), so cancel_work_sync() here
cannot deadlock against the worker.

The VDEC IRQ number is now stored in struct amvdec_core so it is
available to synchronize_irq() at teardown.

Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")
Cc: stable@vger.kernel.org
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
v2: also synchronize_irq() before cancelling the work, so a delayed
    threaded IRQ cannot re-arm it after cancel_work_sync() (raised by
    automated review). Only the VDEC IRQ is synchronized, as the
    ESPARSER IRQ handler does not touch the session or schedule the work.
v1: https://lore.kernel.org/linux-media/20260615140529.52653-1-doruk@0sec.ai/

 drivers/staging/media/meson/vdec/vdec.c | 21 +++++++++++++++++++++
 drivers/staging/media/meson/vdec/vdec.h |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 4b77ec1af5a7..5304987546fa 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -123,6 +123,25 @@ static void vdec_poweroff(struct amvdec_session *sess)
 		codec_ops->drain(sess);
 
 	vdec_ops->stop(sess);
+
+	/*
+	 * vdec_ops->stop() masks the VDEC interrupt at the hardware level, so
+	 * no new IRQ can be raised past this point. The threaded ISR re-arms
+	 * the ESPARSER worker via amvdec_dst_buf_done() (schedule_work()), so
+	 * drain any in-flight handler before cancelling the worker, otherwise
+	 * a late threaded IRQ could schedule it again after the cancel.
+	 *
+	 * The worker dereferences sess->m2m_ctx and touches the ESPARSER/DOS
+	 * registers, so it must be cancelled while m2m_ctx is still valid and
+	 * the clocks are still enabled, i.e. before the clk_disable below.
+	 *
+	 * This runs from the stop_streaming()/release() paths, which are
+	 * serialized by the video device lock, not by sess->lock (the lock the
+	 * worker takes), so cancel_work_sync() cannot deadlock here.
+	 */
+	synchronize_irq(sess->core->vdec_irq);
+	cancel_work_sync(&sess->esparser_queue_work);
+
 	clk_disable_unprepare(sess->core->dos_clk);
 	clk_disable_unprepare(sess->core->dos_parser_clk);
 }
@@ -1053,6 +1072,8 @@ static int vdec_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	core->vdec_irq = irq;
+
 	ret = esparser_init(pdev, core);
 	if (ret)
 		return ret;
diff --git a/drivers/staging/media/meson/vdec/vdec.h b/drivers/staging/media/meson/vdec/vdec.h
index 7a5d8e871d70..9a50116a2665 100644
--- a/drivers/staging/media/meson/vdec/vdec.h
+++ b/drivers/staging/media/meson/vdec/vdec.h
@@ -66,6 +66,8 @@ struct amvdec_session;
  * @v4l2_dev: v4l2 device
  * @cur_sess: current decoding session
  * @lock: video device lock
+ * @vdec_irq: IRQ line of the VDEC, used to synchronize the threaded ISR
+ *	      against teardown
  */
 struct amvdec_core {
 	void __iomem *dos_base;
@@ -91,6 +93,7 @@ struct amvdec_core {
 
 	struct amvdec_session *cur_sess;
 	struct mutex lock;
+	int vdec_irq;
 };
 
 /**
-- 
2.43.0


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

Re: [PATCH v2] 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:

New issues:
- [Critical] Placing `cancel_work_sync()` after `vdec_ops->stop()` powers down the VDEC hardware before waiting for the worker, leading to a synchronous external abort (kernel panic).
- [High] Calling `cancel_work_sync(&sess->esparser_queue_work)` inside `vdec_poweroff()` causes an AB-BA deadlock during `VIDIOC_STREAMOFF`.

Pre-existing issues:
- [Critical] `vdec_poweroff()` disables `dos_parser_clk` without disabling or synchronizing the ESPARSER interrupt, which can lead to a bus fault if the ISR runs.
- [Critical] Bypassed worker cancellation during VDEC teardown leads to Use-After-Free of the session structure because the worker can be re-armed after `vdec_poweroff` is called.
- [High] `vdec_decoder_cmd()` calls `esparser_queue_eos()` without holding `sess->lock` or waiting for the worker to finish, leading to concurrent hardware register writes.
--

commit 8d0a9c53c189aac76577ab8bfce08af609857dc4
Author: Doruk Tan Ozturk <doruk@0sec.ai>

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

This patch attempts to fix a use-after-free in the meson vdec driver by
synchronizing the vdec interrupt and canceling the esparser worker during
poweroff. It also records the vdec irq in the amvdec_core struct to
facilitate synchronization.

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index a039d925c0fe5..f549aedde288c 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -123,6 +123,25 @@ static void vdec_poweroff(struct amvdec_session *sess)
>  		codec_ops->drain(sess);
>  
>  	vdec_ops->stop(sess);

[Severity: Critical]
Is it safe to place cancel_work_sync() after vdec_ops->stop() here?

vdec_ops->stop() isolates the hardware power domain and disables its clocks.

If the worker is concurrently executing esparser_vififo_get_free_space(), it
will read hardware registers.

Could accessing these memories after they are powered down trigger a
synchronous external abort? Should the worker be canceled before tearing
down the hardware?

> +
> +	/*
> +	 * vdec_ops->stop() masks the VDEC interrupt at the hardware level, so
> +	 * no new IRQ can be raised past this point. The threaded ISR re-arms
> +	 * the ESPARSER worker via amvdec_dst_buf_done() (schedule_work()), so
> +	 * drain any in-flight handler before cancelling the worker, otherwise
> +	 * a late threaded IRQ could schedule it again after the cancel.
> +	 *
> +	 * The worker dereferences sess->m2m_ctx and touches the ESPARSER/DOS
> +	 * registers, so it must be cancelled while m2m_ctx is still valid and
> +	 * the clocks are still enabled, i.e. before the clk_disable below.
> +	 *
> +	 * This runs from the stop_streaming()/release() paths, which are
> +	 * serialized by the video device lock, not by sess->lock (the lock the
> +	 * worker takes), so cancel_work_sync() cannot deadlock here.
> +	 */
> +	synchronize_irq(sess->core->vdec_irq);
> +	cancel_work_sync(&sess->esparser_queue_work);

[Severity: High]
The commit message states stop_streaming() runs under the video device lock,
not sess->lock. Is that accurate for v4l2 queue ioctls?

When userspace issues VIDIOC_STREAMOFF, the v4l2 core acquires the queue
lock, which is mapped to sess->lock. It then calls vdec_stop_streaming(),
which calls vdec_poweroff() and blocks here on cancel_work_sync().

If the worker is running and tries to acquire sess->lock:

esparser.c:esparser_queue_all_src() {
    ...
    mutex_lock(&sess->lock);
    ...
}

won't this result in an AB-BA deadlock since the ioctl handler already
holds sess->lock?

[Severity: High]
This isn't a bug introduced by this patch, but how does vdec_decoder_cmd()
synchronize with the parser while this worker might be running?

When userspace issues VIDIOC_DECODER_CMD(V4L2_DEC_CMD_STOP), it calls
esparser_queue_eos() without holding sess->lock. The v4l2 core acquires the
video device lock for this ioctl, but not the queue lock.

Could this concurrently clobber parser registers or the search_done waitqueue
state if the worker is actively running esparser_write_data()?

[Severity: Critical]
This is a pre-existing issue, but could the worker still be re-armed and
cause a use-after-free during device close?

If VIDIOC_STREAMOFF is called on the capture queue, vdec_stop_streaming()
calls vdec_poweroff() and sets sess->status to STATUS_STOPPED.

However, if the output queue is still streaming, a subsequent VIDIOC_QBUF
bypasses status checks and re-arms the worker.

When the device is later closed, vdec_stop_streaming() skips vdec_poweroff()
entirely because the status is already STATUS_STOPPED.

Will vdec_close() then free the session structure via kfree(sess) while the
worker is still scheduled?

> +
>  	clk_disable_unprepare(sess->core->dos_clk);
>  	clk_disable_unprepare(sess->core->dos_parser_clk);

[Severity: Critical]
This is a pre-existing issue, but isn't the system vulnerable to a bus fault
if dos_parser_clk is disabled without synchronizing the parser interrupt?

The hardirq handler esparser_isr() unconditionally reads the
PARSER_INT_STATUS mmio register.

If the hardware asserts the interrupt right before or during this clock
disable, and the isr runs after the clock is off, could this trigger a
synchronous external abort?

>  }
[ ... ]

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

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

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

[Doruk Tan Ozturk]

Please drop v1 and v2 -- both are wrong, and the sashiko review was right
about the deadlock.

The underlying bug is real: vdec_close() does kfree(sess) (and
v4l2_m2m_ctx_release() frees sess->m2m_ctx) without cancelling
sess->esparser_queue_work, whose worker dereferences sess->lock and
sess->m2m_ctx -> UAF if it is pending/running at teardown.

But cancelling on the streamoff/poweroff path can't work:

1) Deadlock. The worker takes sess->lock. For an m2m fh the ioctl core
   takes m2m_ctx->q_lock (== sess->lock) for VIDIOC_STREAMOFF and holds it
   across the handler, so vdec_stop_streaming() -> vdec_poweroff() already
   runs under sess->lock; cancel_work_sync() there waits on a worker blocked
   on that same lock.

2) Use-after-power-down. v2 also cancelled after vdec_ops->stop(), which
   power-gates VDEC1 (__vdec_1_stop()), while the worker still reads a VDEC1
   register (vdec_1_vififo_level() -> VLD_MEM_VIFIFO_LEVEL).

The only deadlock-free point I see is vdec_close() (the ->release fop, not
under sess->lock), cancelling before v4l2_m2m_ctx_release() -- but that
still leaves the threaded VDEC ISR (amvdec_dst_buf_done() ->
schedule_work()) able to re-arm the worker, and there are adjacent teardown
issues (esparser_isr() vs the dos_parser_clk disable;
vdec_decoder_cmd()/esparser_queue_eos() without sess->lock).

I don't have Meson hardware to validate a corrected fix. Is a
vdec_close()-only cancel (plus quiescing the VDEC IRQ outside sess->lock)
the direction you'd want, or would you rather take it given the HW testing
and the surrounding teardown concerns?

Doruk

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