[PATCH v5 0/6] media: meson: Fix memory leak in error path in vdec

[PATCH v5 0/6] media: meson: Fix memory leak in error path in vdec

[Anand Moon]

V5: Changes 
Following chamges try to fix the memory leak reported by Sashiko

New issues:
- [High] The newly added error path in `vdec_start_streaming()` leaks 
  `sess->priv` when `kthread_run()` fails.

Pre-existing issues:
- [Critical] Race condition between hardware power-on and `core->cur_sess`
   initialization leads to a NULL pointer dereference in the IRQ handler.
- [High] Returning buffers for both source and destination queues upon
    single-queue failure orphans active queue buffers.
- [High] Concurrent sessions can bypass the hardware exclusivity check, 
  leading to simultaneous hardware programming.
--

Reported-by: Sashiko <sashiko-bot@kernel.org>
https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/

V4: Changes:

Following chamges try to fix the memory leak reported by Sashiko

Pre-existing issues:
- [Critical] The `sess->esparser_queue_work` work item is not canceled
   before freeing the session context, leading to a potential Use-After-Free
   vulnerability.
- [High] The patch attempts to fix a memory leak reported by kmemleak,
    but misdiagnoses the root cause and leaves the primary memory leak
    (the V4L2 control handler) unresolved.
- [High] The driver does not verify if `kthread_run()` returns an `ERR_PTR`,
     leading to a kernel panic when `kthread_stop()` is called.

Reported-by: Sashiko <sashiko-bot@kernel.org>
https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t

Thanks
-Anand

Anand Moon (6):
  media: meson: vdec: Fix memory leak in error path of vdec_open
  media: meson: vdec: Protect session exclusivity check with lock
  media: meson: vdec: Set cur_sess before hardware vdec_poweron()
  media: meson: vdec: Handle kthread error and free codec private data
  media: meson: vdec: Isolate error path buffer flush to the active
    queue
  media: meson: vdec: Cancel esparser work in error and stop paths

 drivers/staging/media/meson/vdec/vdec.c | 54 ++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 10 deletions(-)


base-commit: e7ae89a0c97ce2b68b0983cd01eda67cf373517d
-- 
2.50.1


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

[PATCH v5 1/6] media: meson: vdec: Fix memory leak in error path of vdec_open

[Anand Moon]

The vdec_open() function previously jumped directly to err_m2m_release
when vdec_init_ctrls() failed, skipping release of the m2m context.
This caused a resource leak.

Fix it by introducing a proper err_m2m_ctx_release label that calls
v4l2_m2m_ctx_release(sess->m2m_ctx) before releasing the m2m device.
Also free the v4l2 control handler memory allocated by vdec_init_ctrls()
in vdec_close().

This was identified via kmemleak:
unreferenced object 0xffff0000205d6878 (size 8):
  comm "v4l_id", pid 5289, jiffies 4294938580
  hex dump (first 8 bytes):
    40 d2 49 18 00 00 ff ff                          @.I.....
  backtrace (crc d3204599):
    kmemleak_alloc+0xc8/0xf0
    __kvmalloc_node_noprof+0x60c/0x850
    v4l2_ctrl_handler_init_class+0x1b4/0x2e8 [videodev]
    vdec_open+0x1f4/0x788 [meson_vdec]
    v4l2_open+0x144/0x460 [videodev]
    chrdev_open+0x1ac/0x500
    do_dentry_open+0x3f0/0xfe8
    vfs_open+0x68/0x320
    do_open+0x2d8/0x9a8
    path_openat+0x1d0/0x4f0
    do_filp_open+0x190/0x380
    do_sys_openat2+0xf8/0x1b0
    __arm64_sys_openat+0x13c/0x1e8
    invoke_syscall+0xdc/0x268
    el0_svc_common.constprop.0+0x178/0x258
    do_el0_svc+0x4c/0x70

Cc: Nicolas Dufresne <nicolas@ndufresne.ca>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v5: update the error path for v4l2_ctrl_handler_free() as per the review
    ccmment to fix the use after free bug.
[4] https://patchwork.kernel.org/project/linux-amlogic/patch/20260521073449.10057-2-linux.amoon@gmail.com/

v4: update the commit message to add v4l2_ctrl_handler_free() in vdec_close()
to adderss the issue:
  This isn't a bug introduced by this patch, but does vdec_close() properly
  free the v4l2 control handler memory allocated by vdec_init_ctrls() here?

v3: https://lore.kernel.org/all/20260520044046.7553-1-linux.amoon@gmail.com/
  update the commit messagee.

v2: https://lore.kernel.org/all/20260321065408.209723-1-linux.amoon@gmail.com/
  updated the commit message, applied the suggestion from sashiko below.

  [3] https://sashiko.dev/#/patchset/20260321065408.209723-1-linux.amoon%40gmail.com

v1: https://lore.kernel.org/all/20260304100557.126488-1-linux.amoon@gmail.com/
  tried to address the issue reported by Nicolas improve the commit message.
---
 drivers/staging/media/meson/vdec/vdec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 4b77ec1af5a7..18a22b79e835 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -889,7 +889,7 @@ static int vdec_open(struct file *file)
 
 	ret = vdec_init_ctrls(sess);
 	if (ret)
-		goto err_m2m_release;
+		goto err_m2m_ctx_release;
 
 	sess->pixfmt_cap = formats[0].pixfmts_cap[0];
 	sess->fmt_out = &formats[0];
@@ -913,6 +913,8 @@ static int vdec_open(struct file *file)
 
 	return 0;
 
+err_m2m_ctx_release:
+	v4l2_m2m_ctx_release(sess->m2m_ctx);
 err_m2m_release:
 	v4l2_m2m_release(sess->m2m_dev);
 err_free_sess:
@@ -928,6 +930,7 @@ static int vdec_close(struct file *file)
 	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);
-- 
2.50.1


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

Re: [PATCH v5 1/6] media: meson: vdec: Fix memory leak in error path of vdec_open

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Allocating and destroying a V4L2 m2m_dev on every session open/close causes a Use-After-Free within the V4L2 M2M core.
- [High] The hardware interrupt handler vdec_isr() dereferences core->cur_sess without checking if it is NULL.
--

commit bde68706fa632effa876c30cef33b3528cd917da
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Fix memory leak in error path of vdec_open

This commit introduces an err_m2m_ctx_release label in vdec_open() to
prevent a resource leak when vdec_init_ctrls() fails. It also frees the
v4l2 control handler memory in vdec_close().

Closes: https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 4b77ec1af5a76..18a22b79e835e 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
[ ... ]
> @@ -913,6 +913,8 @@ static int vdec_open(struct file *file)
>  
>  	return 0;
>  
> +err_m2m_ctx_release:
> +	v4l2_m2m_ctx_release(sess->m2m_ctx);
>  err_m2m_release:
>  	v4l2_m2m_release(sess->m2m_dev);

[Severity: Critical]
This is a pre-existing issue, but does allocating and destroying the m2m_dev
on every session open and close cause a use-after-free?

Since the v4l2 m2m core schedules jobs via a work struct, if a job is
scheduled just before the file descriptor is closed:

vdec_close()
    v4l2_m2m_release(sess->m2m_dev);
        kfree(m2m_dev);

Could the system worker later execute v4l2_m2m_device_run_work() on the
freed m2m_dev, as pending background work isn't cancelled during release?

Should m2m_dev be allocated globally once in probe() instead?

[ ... ]

[Severity: High]
This is a pre-existing issue, but looking at vdec_isr(), sess is initialized
from core->cur_sess and immediately dereferenced:

vdec_isr()
    sess = core->cur_sess;
    sess->last_irq_jiffies = get_jiffies_64();

If a spurious interrupt fires while the device is idle or immediately after
streaming stops, could core->cur_sess be NULL here?

Should there be a check before dereferencing sess?

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

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

Re: [PATCH v5 1/6] media: meson: vdec: Fix memory leak in error path of vdec_open

[Anand Moon]

Hi Sashiko,

Thanks for your review comments

On Mon, 25 May 2026 at 15:57, <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Allocating and destroying a V4L2 m2m_dev on every session open/close causes a Use-After-Free within the V4L2 M2M core.
> - [High] The hardware interrupt handler vdec_isr() dereferences core->cur_sess without checking if it is NULL.
> --
>
> commit bde68706fa632effa876c30cef33b3528cd917da
> Author: Anand Moon <linux.amoon@gmail.com>
>
> media: meson: vdec: Fix memory leak in error path of vdec_open
>
> This commit introduces an err_m2m_ctx_release label in vdec_open() to
> prevent a resource leak when vdec_init_ctrls() fails. It also frees the
> v4l2 control handler memory in vdec_close().
>
> Closes: https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t
>
> > diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> > index 4b77ec1af5a76..18a22b79e835e 100644
> > --- a/drivers/staging/media/meson/vdec/vdec.c
> > +++ b/drivers/staging/media/meson/vdec/vdec.c
> [ ... ]
> > @@ -913,6 +913,8 @@ static int vdec_open(struct file *file)
> >
> >       return 0;
> >
> > +err_m2m_ctx_release:
> > +     v4l2_m2m_ctx_release(sess->m2m_ctx);
> >  err_m2m_release:
> >       v4l2_m2m_release(sess->m2m_dev);
>
> [Severity: Critical]
> This is a pre-existing issue, but does allocating and destroying the m2m_dev
> on every session open and close cause a use-after-free?
>
> Since the v4l2 m2m core schedules jobs via a work struct, if a job is
> scheduled just before the file descriptor is closed:
>
> vdec_close()
>     v4l2_m2m_release(sess->m2m_dev);
>         kfree(m2m_dev);
>
> Could the system worker later execute v4l2_m2m_device_run_work() on the
> freed m2m_dev, as pending background work isn't cancelled during release?
>
> Should m2m_dev be allocated globally once in probe() instead?
>
> [ ... ]
>
> [Severity: High]
> This is a pre-existing issue, but looking at vdec_isr(), sess is initialized
> from core->cur_sess and immediately dereferenced:
>
> vdec_isr()
>     sess = core->cur_sess;
>     sess->last_irq_jiffies = get_jiffies_64();
>
> If a spurious interrupt fires while the device is idle or immediately after
> streaming stops, could core->cur_sess be NULL here?
>
> Should there be a check before dereferencing sess?
Apologies, let me check. Every time I try to fix a bug, I seem to
create a new one.
>
Thanks
-Anand
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260525095216.12078-1-linux.amoon@gmail.com?part=1

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

[PATCH v5 2/6] media: meson: vdec: Protect session exclusivity check with lock

[Anand Moon]

Add the check for an active hardware session is performed without holding
the core->lock mutex. In multi-threaded environments, two concurrent
STREAMON ioctls on different file descriptors can simultaneously find
core->cur_sess to be NULL, bypass the check, and concurrently call
vdec_poweron(), corrupting hardware state.

Fix this by wrapping the session exclusivity check inside core->lock.

Cc: Nicolas Dufresne <nicolas@ndufresne.ca>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v5: New patch.
[High] Concurrent sessions can bypass the hardware exclusivity check,
leading to simultaneous hardware programming.
---
 drivers/staging/media/meson/vdec/vdec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 18a22b79e835..e72f54af026e 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -286,10 +286,13 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
 	struct vb2_v4l2_buffer *buf;
 	int ret;
 
+	mutex_lock(&core->lock);
 	if (core->cur_sess && core->cur_sess != sess) {
+		mutex_unlock(&core->lock);
 		ret = -EBUSY;
 		goto bufs_done;
 	}
+	mutex_unlock(&core->lock);
 
 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
 		sess->streamon_out = 1;
-- 
2.50.1


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

Re: [PATCH v5 2/6] media: meson: vdec: Protect session exclusivity check with lock

[sashiko-bot]

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

New issues:
- [High] The patch fails to resolve the concurrent STREAMON race condition, leaving a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability.

Pre-existing issues:
- [High] Unprotected modification of `core->cur_sess` in `vdec_stop_streaming` leads to a data race.
- [High] Missing NULL check for `core->cur_sess` in interrupt handlers risks a kernel panic from spurious interrupts.
--

commit 7f55f473f3e7d75722e346666034bf541752f2d3
Author: Anand Moon <linux.amoon@gmail.com>
Subject: media: meson: vdec: Protect session exclusivity check with lock

This commit attempts to fix a race condition during concurrent STREAMON calls.
It wraps the session exclusivity check in vdec_start_streaming() with a
mutex to prevent multiple threads from bypassing the active session check
and concurrently triggering hardware initialization.

Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 18a22b79e835e..e72f54af026e3 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -286,10 +286,13 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	struct vb2_v4l2_buffer *buf;
>  	int ret;
>  
> +	mutex_lock(&core->lock);
>  	if (core->cur_sess && core->cur_sess != sess) {
> +		mutex_unlock(&core->lock);
>  		ret = -EBUSY;
>  		goto bufs_done;
>  	}
> +	mutex_unlock(&core->lock);

[Severity: High]
Does this completely resolve the concurrent STREAMON race condition?

While the lock protects the check itself, it is released immediately
afterward. This leaves a window where two threads could sequentially acquire
the lock, observe core->cur_sess as NULL, and then concurrently proceed
with hardware initialization in vdec_poweron().

A later patch in the series wraps the eventual core->cur_sess = sess
assignment in the lock, but does that still leave an unprotected gap between
this check and the assignment?

[Severity: High]
This is a pre-existing issue, but does core->cur_sess need lock protection
in vdec_stop_streaming() as well?

Since V4L2 queue locks are used, STREAMON and STREAMOFF on different file
descriptors can execute concurrently. If vdec_stop_streaming() sets
core->cur_sess to NULL without acquiring core->lock, could this create a
data race on the global pointer?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
core->cur_sess to be NULL when an interrupt fires?

Since core->cur_sess is set to NULL when no decoding session is active,
could a spurious hardware interrupt arriving during an idle period cause
a NULL pointer dereference in vdec_isr() or vdec_threaded_isr() if they
blindly dereference core->cur_sess without checking?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525095216.12078-1-linux.amoon@gmail.com?part=2

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

[PATCH v5 3/6] media: meson: vdec: Set cur_sess before hardware vdec_poweron()

[Anand Moon]

vdec_poweron() initializes hardware and unmasks device interrupts. If an
interrupt fires before core->cur_sess is set, vdec_isr() dereferences a
NULL pointer when updating sess->last_irq_jiffies, leading to a kernel
panic.

Fix this by assigning core->cur_sess and updating sess->status under
core->lock before calling vdec_poweron(). This ensures the interrupt
handler always sees a valid session pointer. On the error path, clear
core->cur_sess and reset sess->status to STATUS_STOPPED to avoid stale
references.

Following change also strengthens the hardware exclusivity check by
holding core->lock during session assignment, preventing concurrent
sessions from racing through cur_sess == NULL and corrupting hardware
state.

Cc: Nicolas Dufresne <nicolas@ndufresne.ca>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v5: [Critical] Race condition between hardware power-on and `core->cur_sess`
 initialization leads to a NULL pointer dereference in the IRQ handler.
---
 drivers/staging/media/meson/vdec/vdec.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index e72f54af026e..52ace4de967c 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -334,6 +334,11 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
 	atomic_set(&sess->esparser_queued_bufs, 0);
 	v4l2_ctrl_s_ctrl(sess->ctrl_min_buf_capture, 1);
 
+	mutex_lock(&core->lock);
+	core->cur_sess = sess;
+	sess->status = STATUS_INIT;
+	mutex_unlock(&core->lock);
+
 	ret = vdec_poweron(sess);
 	if (ret)
 		goto vififo_free;
@@ -344,12 +349,14 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
 		sess->recycle_thread = kthread_run(vdec_recycle_thread, sess,
 						   "vdec_recycle");
 
-	sess->status = STATUS_INIT;
-	core->cur_sess = sess;
 	schedule_work(&sess->esparser_queue_work);
 	return 0;
 
 vififo_free:
+	mutex_lock(&core->lock);
+	core->cur_sess = NULL;
+	sess->status = STATUS_STOPPED;
+	mutex_unlock(&core->lock);
 	dma_free_coherent(sess->core->dev, sess->vififo_size,
 			  sess->vififo_vaddr, sess->vififo_paddr);
 bufs_done:
-- 
2.50.1


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

Re: [PATCH v5 3/6] media: meson: vdec: Set cur_sess before hardware vdec_poweron()

[sashiko-bot]

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

New issues:
- [Critical] Setting `core->cur_sess` before enabling hardware clocks creates a window where an interrupt can trigger hardware access while clocks are gated, causing a system crash.

Pre-existing issues:
- [High] The patch attempts to fix a hardware exclusivity race by protecting the assignment of `core->cur_sess` with a lock, but leaves a critical Time-Of-Check to Time-Of-Use (TOCTOU) gap.
- [High] The interrupt handlers lack a NULL pointer check for `core->cur_sess`, leaving the driver susceptible to panics from spurious or delayed interrupts.
- [High] The global pointers `core->cur_sess` and `sess->status` are cleared in `vdec_stop_streaming()` without acquiring `core->lock`, introducing a data race.
--

commit aa641955fed13a943e3d3bbb36f369ce0d7fb41c
Author: Anand Moon <linux.amoon@gmail.com>
Subject: media: meson: vdec: Set cur_sess before hardware vdec_poweron()

This patch moves the assignment of core->cur_sess and sess->status before
vdec_poweron() in vdec_start_streaming() to prevent a NULL pointer dereference
in the ISR. It also updates the error path and protects the assignments with
core->lock.

Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index e72f54af026e3..52ace4de967cb 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -334,6 +334,11 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	atomic_set(&sess->esparser_queued_bufs, 0);
>  	v4l2_ctrl_s_ctrl(sess->ctrl_min_buf_capture, 1);
>  
> +	mutex_lock(&core->lock);
> +	core->cur_sess = sess;
> +	sess->status = STATUS_INIT;
> +	mutex_unlock(&core->lock);
> +
>  	ret = vdec_poweron(sess);

[Severity: Critical]
By setting core->cur_sess before calling vdec_poweron(), could this create a
window where a spurious or shared interrupt triggers a hardware access while
the hardware clocks are still gated?

If the interrupt fires here, the ISR might invoke the active codec's ISR and
perform MMIO writes. Accessing registers of a clock-gated device could result
in a synchronous external abort (bus error or SError) and crash the kernel.


[Severity: High]
This isn't a bug introduced by this patch, but there is a Time-Of-Check to
Time-Of-Use (TOCTOU) race condition regarding core->cur_sess.

Earlier in vdec_start_streaming(), core->lock is held to check if
core->cur_sess is available, but the lock is immediately released:

	mutex_lock(&core->lock);
	if (core->cur_sess && core->cur_sess != sess) {
		mutex_unlock(&core->lock);
		ret = -EBUSY;
		goto bufs_done;
	}
	mutex_unlock(&core->lock);

Multiple concurrent sessions could simultaneously observe
core->cur_sess == NULL, drop the lock, perform memory allocations, and then
sequentially re-acquire the lock here to overwrite core->cur_sess. Both
sessions would then execute vdec_poweron(), bypassing the exclusivity check.

Should the initial check and the assignment be performed atomically within the
same critical section?


[Severity: High]
This is a pre-existing issue, but does this fix address the root cause of the
NULL pointer dereference?

The commit message notes that vdec_isr() can dereference a NULL pointer if an
interrupt fires early. Because the IRQ is registered during probe, it remains
active for the device's lifetime. If a spurious interrupt fires when no
session is active, or if a delayed interrupt triggers immediately after
vdec_stop_streaming() clears core->cur_sess, could the kernel still panic?

Should vdec_isr() and vdec_threaded_isr() be updated to include a proper NULL
check for core->cur_sess?

>  	if (ret)
>  		goto vififo_free;
> @@ -344,12 +349,14 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  		sess->recycle_thread = kthread_run(vdec_recycle_thread, sess,
>  						   "vdec_recycle");
>  
> -	sess->status = STATUS_INIT;
> -	core->cur_sess = sess;
>  	schedule_work(&sess->esparser_queue_work);
>  	return 0;
>  
>  vififo_free:
> +	mutex_lock(&core->lock);
> +	core->cur_sess = NULL;
> +	sess->status = STATUS_STOPPED;
> +	mutex_unlock(&core->lock);

[Severity: High]
This is a pre-existing issue, but while core->lock is now used here in
vdec_start_streaming(), is it also required in vdec_stop_streaming()?

If vdec_stop_streaming() clears core->cur_sess and sess->status locklessly, it
could run concurrently with another session's vdec_start_streaming(). This
could break the synchronization model and allow for data races where the
starting session reads an inconsistent hardware state.

>  	dma_free_coherent(sess->core->dev, sess->vififo_size,
>  			  sess->vififo_vaddr, sess->vififo_paddr);
>  bufs_done:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525095216.12078-1-linux.amoon@gmail.com?part=3

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

[PATCH v5 4/6] media: meson: vdec: Handle kthread error and free codec private data

[Anand Moon]

vdec_start_streaming() launches a recycle thread when required by the
codec. If kthread_run() fails, the previous error path only powered off
the hardware, leaving sess->priv and codec state allocated. This caused
a permanent leak of the codec context and associated DMA buffers.

Fix this by adding an err_cleanup path: if thread creation fails, call
codec_ops->stop() to release the codec context and clear sess->priv,
then power off the hardware. Also reset core->cur_sess and sess->status
to avoid stale references.

This change closes the memory leak on kthread_run() failure and ensures
proper cleanup of codec resources.

Cc: Nicolas Dufresne <nicolas@ndufresne.ca>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v5: The vdec_poweron() function invoked earlier allocates dynamic memory for
the codec context and assigns it to sess->priv. When kthread_run() fails,
this new error path calls vdec_poweroff() which stops the hardware but
doesn't free sess->priv.
---
 drivers/staging/media/meson/vdec/vdec.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 52ace4de967c..b31bf08af88e 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -345,13 +345,25 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
 
 	sess->sequence_cap = 0;
 	sess->sequence_out = 0;
-	if (vdec_codec_needs_recycle(sess))
+	if (vdec_codec_needs_recycle(sess)) {
 		sess->recycle_thread = kthread_run(vdec_recycle_thread, sess,
 						   "vdec_recycle");
+		if (IS_ERR(sess->recycle_thread)) {
+			ret = PTR_ERR(sess->recycle_thread);
+			sess->recycle_thread = NULL;
+			goto err_cleanup;
+		}
+	}
 
 	schedule_work(&sess->esparser_queue_work);
 	return 0;
 
+err_cleanup:
+	if (codec_ops && codec_ops->stop && sess->priv) {
+		codec_ops->stop(sess);
+		sess->priv = NULL;
+	}
+	vdec_poweroff(sess);
 vififo_free:
 	mutex_lock(&core->lock);
 	core->cur_sess = NULL;
-- 
2.50.1


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

Re: [PATCH v5 4/6] media: meson: vdec: Handle kthread error and free codec private data

[sashiko-bot]

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

New issues:
- [High] Calling `codec_ops->stop()` before `vdec_poweroff()` causes a hardware DMA use-after-free vulnerability.
- [Medium] The error path fails to free the dynamically allocated codec context (`sess->priv`), leading to a permanent memory leak.

Pre-existing issues:
- [High] The error path fails to free the hardware canvases allocated during `vdec_poweron()`.
--

commit 8dc81d30970c8a0de3ed5a1ebe498c0479036766
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Handle kthread error and free codec private data

This commit adds an error handling path in vdec_start_streaming() when
kthread_run() fails. It introduces an err_cleanup label to invoke the
codec's stop callback and power off the hardware, aiming to fix memory leaks.

Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 52ace4de967cb..b31bf08af88e6 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
[ ... ]
> @@ -345,13 +345,25 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
[ ... ]
>  	schedule_work(&sess->esparser_queue_work);
>  	return 0;
>  
> +err_cleanup:
> +	if (codec_ops && codec_ops->stop && sess->priv) {
> +		codec_ops->stop(sess);

[Severity: High]
Does calling codec_ops->stop() before vdec_poweroff() risk a hardware DMA use
after free?

The codec_ops->stop() callbacks unmap and free DMA memory used by the
firmware, but vdec_poweroff() hasn't been called yet to halt the hardware. The
hardware could still be powered on with its DMA pointers pointing to the freed
memory.

Could this lead to IOMMU faults or memory corruption if the active hardware
issues any memory transactions before it is powered off?

> +		sess->priv = NULL;

[Severity: Medium]
Does zeroing out sess->priv without freeing it cause a memory leak?

The codec_ops->stop() implementations typically only release internal DMA
buffers, but do not free the sess->priv structure itself. Normally, this
memory is freed via kfree() in vdec_stop_streaming().

Does setting it to NULL here permanently orphan the structure?

> +	}
> +	vdec_poweroff(sess);

[Severity: High]
This is a pre-existing issue, but does this error path also need to free the
hardware canvases?

During vdec_poweron(), codecs allocate hardware canvas IDs. The normal
shutdown path calls vdec_free_canvas() to release them.

Since the hardware has a limited global pool of canvas IDs, does omitting this
call risk rendering the video decoder subsystem unusable by leaking the canvas
IDs?

>  vififo_free:
>  	mutex_lock(&core->lock);
>  	core->cur_sess = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525095216.12078-1-linux.amoon@gmail.com?part=4

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

[PATCH v5 5/6] media: meson: vdec: Isolate error path buffer flush to the active queue

[Anand Moon]

When vdec_start_streaming() fails, the error path clears buffers from both
the source and destination queues unconditionally. If one queue was already
streaming successfully from a prior invocation, flushing its buffers behind
its back leaves videobuf2 deadlocked waiting for completions.

Fix this by only sweeping buffers from the specific queue type container
that failed to initialize.

Cc: Nicolas Dufresne <nicolas@ndufresne.ca>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v5:
This is a pre-existing issue, but will returning buffers for both queues upon
a single-queue failure orphan active queue buffers?

If the CAPTURE queue was successfully started in a previous call, returning
its buffers puts them back into the vb2 queued list while the driver
discards its references. Because the CAPTURE queue remains active,
userspace calling DQBUF will hang indefinitely waiting for frames that
the driver will never process.
---
 drivers/staging/media/meson/vdec/vdec.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index b31bf08af88e..925537bd4d0b 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -372,15 +372,15 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
 	dma_free_coherent(sess->core->dev, sess->vififo_size,
 			  sess->vififo_vaddr, sess->vififo_paddr);
 bufs_done:
-	while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
-		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
-	while ((buf = v4l2_m2m_dst_buf_remove(sess->m2m_ctx)))
-		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
-
-	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		sess->streamon_out = 0;
-	else
+		while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
+			v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
+	} else {
 		sess->streamon_cap = 0;
+		while ((buf = v4l2_m2m_dst_buf_remove(sess->m2m_ctx)))
+			v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
+	}
 
 	return ret;
 }
-- 
2.50.1


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

Re: [PATCH v5 5/6] media: meson: vdec: Isolate error path buffer flush to the active queue

[sashiko-bot]

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

Pre-existing issues:
- [High] The error path in `vdec_start_streaming()` bypasses the teardown block, causing permanent memory leaks of hardware canvas IDs and codec private data.
--

commit c72dab3c816fea617e47aef85e4ce526fc910b8a
Author: Anand Moon <linux.amoon@gmail.com>
Subject: media: meson: vdec: Isolate error path buffer flush to the active queue

When vdec_start_streaming() fails, this commit limits clearing buffers to
the specific queue type that failed to initialize. This prevents videobuf2
deadlocks when the other queue is already streaming successfully.

Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index b31bf08af88e6..925537bd4d0b3 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -372,15 +372,15 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	dma_free_coherent(sess->core->dev, sess->vififo_size,
>  			  sess->vififo_vaddr, sess->vififo_paddr);
>  bufs_done:

[Severity: High]
This isn't a bug introduced by this patch, but does the error path leading
up to this label cause permanent memory leaks of hardware canvas IDs and
codec private data?

When initialization fails late in vdec_start_streaming(), for example if
kthread_run() fails, the error path handles cleanup:

err_cleanup:
    if (codec_ops && codec_ops->stop && sess->priv) {
        codec_ops->stop(sess);
        sess->priv = NULL;
    }
    vdec_poweroff(sess);
vififo_free:
    mutex_lock(&core->lock);
    core->cur_sess = NULL;
    sess->status = STATUS_STOPPED;
    mutex_unlock(&core->lock);

By setting sess->status = STATUS_STOPPED, vdec_stop_streaming() is
prevented from executing its teardown block on device close. As a result,
vdec_free_canvas() is skipped, permanently leaking the limited hardware
canvas IDs allocated during codec_ops->start().

Additionally, setting sess->priv = NULL without freeing it permanently
leaks the codec context memory, because codec_ops->stop() implementations
only free internal DMA buffers and rely on vdec_stop_streaming() to free
sess->priv.

> -	while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
> -		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> -	while ((buf = v4l2_m2m_dst_buf_remove(sess->m2m_ctx)))
> -		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> -
> -	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		sess->streamon_out = 0;
> -	else
> +		while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
> +			v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> +	} else {
>  		sess->streamon_cap = 0;
> +		while ((buf = v4l2_m2m_dst_buf_remove(sess->m2m_ctx)))
> +			v4l2_m2m_buf_done(buf, VB2_BUF_STATE_QUEUED);
> +	}
>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525095216.12078-1-linux.amoon@gmail.com?part=5

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

[PATCH v5 6/6] media: meson: vdec: Cancel esparser work in error and stop paths

[Anand Moon]

The esparser workqueue may remain pending when streaming is stopped or
the device is closed, leading to use-after-free if it runs after session
teardown. vdec_start_streaming(), vdec_stop_streaming(), and vdec_close()
did not cancel this work, leaving a race between session cleanup and
work execution.

Fix this by calling cancel_work_sync(&sess->esparser_queue_work) in all
cleanup paths. Unlocking and relocking sess->lock around the cancel
ensures the work handler cannot run concurrently with teardown. This
prevents dangling work items from accessing freed session memory and
eliminates a potential kernel crash.

Cc: Nicolas Dufresne <nicolas@ndufresne.ca>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v5: Tried to fix the order of cancel_work_sync() which could lead to
   a use-after-free. update the commit message.
---
 drivers/staging/media/meson/vdec/vdec.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 925537bd4d0b..296b387f3667 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -372,6 +372,10 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
 	dma_free_coherent(sess->core->dev, sess->vififo_size,
 			  sess->vififo_vaddr, sess->vififo_paddr);
 bufs_done:
+	mutex_unlock(&sess->lock);
+	cancel_work_sync(&sess->esparser_queue_work);
+	mutex_lock(&sess->lock);
+
 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		sess->streamon_out = 0;
 		while ((buf = v4l2_m2m_src_buf_remove(sess->m2m_ctx)))
@@ -430,6 +434,9 @@ static void vdec_stop_streaming(struct vb2_queue *q)
 			kthread_stop(sess->recycle_thread);
 
 		vdec_poweroff(sess);
+		mutex_unlock(&sess->lock);
+		cancel_work_sync(&sess->esparser_queue_work);
+		mutex_lock(&sess->lock);
 		vdec_free_canvas(sess);
 		dma_free_coherent(sess->core->dev, sess->vififo_size,
 				  sess->vififo_vaddr, sess->vififo_paddr);
@@ -948,6 +955,8 @@ 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);
-- 
2.50.1


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

Re: [PATCH v5 6/6] media: meson: vdec: Cancel esparser work in error and stop paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Dropping the V4L2 queue lock inside `vdec_stop_streaming()` breaks videobuf2 synchronization, allowing concurrent ioctls to cause use-after-free and double-frees.
- [High] Unbalanced mutex unlock/lock in `vdec_stop_streaming()` when called during file release causes kernel panic and lock state corruption.
- [Low] The `cancel_work_sync()` call added to `vdec_close()` is racy and ineffective because it is called before hardware interrupts are disabled.
--

commit 6c14d2bfd15b9a61e01b7119c0cddce74a6adee0
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Cancel esparser work in error and stop paths

This commit adds cancel_work_sync() calls to the stop and close paths to
prevent the esparser workqueue from running after session teardown. It
drops and reacquires the session lock around the cancellation in the
stop_streaming callback.

Closes: https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 925537bd4d0b3..296b387f3667b 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c

[ ... ]

> @@ -430,6 +434,9 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  			kthread_stop(sess->recycle_thread);
>  
>  		vdec_poweroff(sess);
> +		mutex_unlock(&sess->lock);
> +		cancel_work_sync(&sess->esparser_queue_work);
> +		mutex_lock(&sess->lock);

[Severity: Critical]
Does dropping the queue lock here break videobuf2 synchronization?

When the v4l2 core processes a streamoff ioctl, it enters
vb2_core_streamoff() holding this queue lock and eventually calls
q->ops->stop_streaming().

If we unlock it here, could a concurrent streamoff ioctl on the same
queue acquire the lock and re-enter vb2_core_streamoff()?

Since q->start_streaming_called is only cleared after the
stop_streaming callback returns, the second thread might also invoke
vdec_stop_streaming() concurrently. Because sess->status hasn't been
updated to STATUS_STOPPED yet, both threads could execute this teardown
block, potentially leading to double-frees of sess->vififo_vaddr and
sess->priv, or concurrent kthread_stop() calls.

[Severity: High]
Is it guaranteed that sess->lock is held when vdec_stop_streaming() is
called?

When a file descriptor is closed without explicitly stopping the stream
first, the call chain looks like this:

vdec_close()
  v4l2_m2m_ctx_release()
    vb2_queue_release()
      __vb2_queue_cancel()
        q->ops->stop_streaming()

In this path, the v4l2 core does not hold the queue lock. Calling
mutex_unlock() on an unheld mutex will cause lock state corruption.

Furthermore, this code will then acquire the mutex and return, leaving it
locked when vdec_close() subsequently calls mutex_destroy() on it.

[ ... ]

> @@ -948,6 +955,8 @@ static int vdec_close(struct file *file)
>  {
>  	struct amvdec_session *sess = file_to_amvdec_session(file);
>  
> +	cancel_work_sync(&sess->esparser_queue_work);
> +

[Severity: Low]
Can the workqueue be re-armed immediately after this cancellation?

At this point in vdec_close(), the hardware hasn't been powered off yet.
If a hardware interrupt fires right after cancel_work_sync() returns,
could the interrupt service routine call schedule_work() and re-queue it?

While this might not lead to a use-after-free because it gets cancelled
again in vdec_stop_streaming() before any memory is freed, it seems to
leave the workqueue active longer than intended.

>  	v4l2_m2m_ctx_release(sess->m2m_ctx);
>  	v4l2_m2m_release(sess->m2m_dev);
>  	v4l2_fh_del(&sess->fh, file);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525095216.12078-1-linux.amoon@gmail.com?part=6

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