[PATCH] staging: media: meson: vdec: fix UAF, memory leak and NULL derefs

[PATCH] staging: media: meson: vdec: fix UAF, memory leak and NULL derefs

[Alfie Varghese]

This combined commit fixes several pre-existing issues identified in the
Amlogic video decoder driver:

1. Fix a critical Use-After-Free in vdec_close() by synchronously
   canceling the sess->esparser_queue_work before freeing the session.
2. Fix a memory leak in vdec_close() by freeing the session control
   handler via v4l2_ctrl_handler_free().
3. Prevent a potential NULL pointer dereference in vdec_probe() by adding
   a NULL check for of_match_node()'s return value.
4. Prevent a potential NULL pointer dereference in vdec_isr() and
   vdec_threaded_isr() by adding NULL checks for core->cur_sess.

Signed-off-by: Alfie Varghese <alfievarghese22@gmail.com>
---
 drivers/staging/media/meson/vdec/vdec.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index a039d925c0fe..b794ad53711c 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -926,11 +926,15 @@ static int vdec_close(struct file *file)
 {
 	struct amvdec_session *sess = file_to_amvdec_session(file);
 
+	cancel_work_sync(&sess->esparser_queue_work);
+
 	v4l2_m2m_ctx_release(sess->m2m_ctx);
 	v4l2_m2m_release(sess->m2m_dev);
 	v4l2_fh_del(&sess->fh, file);
 	v4l2_fh_exit(&sess->fh);
 
+	v4l2_ctrl_handler_free(&sess->ctrl_handler);
+
 	mutex_destroy(&sess->lock);
 	mutex_destroy(&sess->bufs_recycle_lock);
 
@@ -953,6 +957,9 @@ static irqreturn_t vdec_isr(int irq, void *data)
 	struct amvdec_core *core = data;
 	struct amvdec_session *sess = core->cur_sess;
 
+	if (!sess)
+		return IRQ_NONE;
+
 	sess->last_irq_jiffies = get_jiffies_64();
 
 	return sess->fmt_out->codec_ops->isr(sess);
@@ -963,6 +970,9 @@ static irqreturn_t vdec_threaded_isr(int irq, void *data)
 	struct amvdec_core *core = data;
 	struct amvdec_session *sess = core->cur_sess;
 
+	if (!sess)
+		return IRQ_NONE;
+
 	return sess->fmt_out->codec_ops->threaded_isr(sess);
 }
 
@@ -1020,6 +1030,8 @@ static int vdec_probe(struct platform_device *pdev)
 		return PTR_ERR(core->canvas);
 
 	of_id = of_match_node(vdec_dt_match, dev->of_node);
+	if (!of_id)
+		return -ENODEV;
 	core->platform = of_id->data;
 
 	if (core->platform->revision == VDEC_REVISION_G12A ||

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

Re: [PATCH] staging: media: meson: vdec: fix UAF, memory leak and NULL derefs

[sashiko-bot]

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

Pre-existing issues:
- [High] Incorrect placement of `cancel_work_sync()` before stopping the hardware leaves a Use-After-Free window unmitigated.
- [High] Lockless NULL checks in the interrupt handlers introduce a Time-of-Check to Time-of-Use (TOCTOU) race condition that fails to prevent a Use-After-Free.
- [High] Freeing the per-session `m2m_dev` without canceling its background work causes a Use-After-Free.
- [High] Unchecked return value from `kthread_run()` can lead to an invalid pointer dereference during `kthread_stop()`.
--

--- Patch [1]: [PATCH] staging: media: meson: vdec: fix UAF, memory leak and NULL derefs ---
commit 75b10e9da9f1903a18dbb28c84914aae2e545ae5
Author: Alfie Varghese <alfievarghese22@gmail.com>

staging: media: meson: vdec: fix UAF, memory leak and NULL derefs

This commit attempts to fix several issues in the Amlogic video decoder driver.
It adds synchronous work cancellation to session cleanup, frees the control
handler to address a memory leak, and adds NULL pointer checks in the probe
and interrupt handlers.

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index a039d925c0fe5..b794ad53711c0 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -926,11 +926,15 @@ static int vdec_close(struct file *file)
>  {
>  	struct amvdec_session *sess = file_to_amvdec_session(file);
>  
> +	cancel_work_sync(&sess->esparser_queue_work);

[Severity: High]
This isn't a regression introduced by this patch, but does placing
cancel_work_sync() before stopping the hardware leave a window for the
work to be rescheduled?

If v4l2_m2m_ctx_release() is called after cancel_work_sync(), the hardware
is still actively decoding. Could a hardware interrupt arrive and re-arm
the work via schedule_work() before the session is freed?

>  	v4l2_m2m_ctx_release(sess->m2m_ctx);
>  	v4l2_m2m_release(sess->m2m_dev);

[Severity: High]
This is a pre-existing issue, but might calling v4l2_m2m_release() directly
without canceling its background work cause a use-after-free?

When vdec_m2m_job_abort() finishes a job, it unconditionally schedules the
next job via schedule_work(&m2m_dev->job_work). If v4l2_m2m_release()
immediately frees the device using kfree(), could the pending work access
the freed structure when it executes?

>  	v4l2_fh_del(&sess->fh, file);

[ ... ]

> @@ -953,6 +957,9 @@ static irqreturn_t vdec_isr(int irq, void *data)
>  	struct amvdec_core *core = data;
>  	struct amvdec_session *sess = core->cur_sess;
>  
> +	if (!sess)
> +		return IRQ_NONE;
> +

[Severity: High]
This is a pre-existing issue, but does this lockless check fully prevent a
use-after-free during teardown?

Since vdec_stop_streaming() disables clocks but does not synchronize
interrupts before freeing resources, might a race occur?

For instance, if an ISR evaluates this check successfully but is delayed,
and vdec_stop_streaming() concurrently frees sess->priv and sets cur_sess
to NULL, will the ISR dereference the freed memory when it resumes and
calls sess->fmt_out->codec_ops->isr(sess)?

>  	sess->last_irq_jiffies = get_jiffies_64();
>  

[ ... ]

[Severity: High]
This is also a pre-existing issue in this driver, but in
vdec_start_streaming(), the return value of kthread_run() isn't checked:

vdec_start_streaming() {
    ...
    sess->recycle_thread = kthread_run(vdec_recycle_thread, sess,
                                       "vdec_recycle");
    sess->status = STATUS_INIT;
    ...
}

If thread creation fails and returns an ERR_PTR under memory pressure, is
it possible for vdec_stop_streaming() to unconditionally pass that
ERR_PTR to kthread_stop(), resulting in a panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713173859.643-1-alfievarghese22@gmail.com?part=1

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