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

[sashiko-bot@kernel.org]

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