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

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 <[email protected]>
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/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic