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

[Anand Moon]

Hi Doruk,

On Wed, 17 Jun 2026 at 13:11, Doruk Tan Ozturk <doruk@xxxxxxx> wrote:
>
> 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?
>
Actually, I've been working on this issue for a while and have made a few
changes. I really like your approach, so I'd like to integrate
it alongside my cleanup changes. It should solve this issue.

diff --git a/drivers/staging/media/meson/vdec/esparser.c
b/drivers/staging/media/meson/vdec/esparser.c
index 4632346f04a9..c5b909c6a2b7 100644
--- a/drivers/staging/media/meson/vdec/esparser.c
+++ b/drivers/staging/media/meson/vdec/esparser.c
@@ -375,6 +375,9 @@ void esparser_queue_all_src(struct work_struct *work)
        struct amvdec_session *sess =
                container_of(work, struct amvdec_session, esparser_queue_work);

+       if (READ_ONCE(sess->should_stop))
+               return;
+
        mutex_lock(&sess->lock);
        v4l2_m2m_for_each_src_buf_safe(sess->m2m_ctx, buf, n) {
                if (sess->should_stop)
diff --git a/drivers/staging/media/meson/vdec/vdec.c
b/drivers/staging/media/meson/vdec/vdec.c
index 51ea7beef811..de3a660d22b1 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -448,6 +448,9 @@ static void vdec_stop_streaming(struct vb2_queue *q)
        enum amvdec_status old_status;
        bool full_cleanup = false;

+       sess->should_stop = 1;
+       cancel_work_sync(&sess->esparser_queue_work);
+
        /*
         * Secure the hardware lock for the ENTIRE state evaluation
         * sequence to block concurrent start_streaming() callers.
@@ -1000,6 +1003,9 @@ static int vdec_close(struct file *file)
 {
        struct amvdec_session *sess = file_to_amvdec_session(file);

+       sess->should_stop = 1;
+       cancel_work_sync(&sess->esparser_queue_work);
+
        if (!IS_ERR_OR_NULL(sess->recycle_thread)) {
                kthread_stop(sess->recycle_thread);
                sess->recycle_thread = NULL;

> Doruk
>
Thanks
-Anand
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-amlogic