[PATCH v4 0/3] media: meson: Fix memory leak in error path in

[PATCH v4 0/3] media: meson: Fix memory leak in error path in

[Anand Moon]

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>
Closes: https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t

Thanks
-Anand

Anand Moon (3):
  media: meson: vdec: Fix memory leak in error path of vdec_open
  media: meson: vdec: Add error handling for recycle thread creation
  media: meson: vdec: Cancel esparser work in error and stop paths

 drivers/staging/media/meson/vdec/vdec.c | 27 +++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)


base-commit: 8bc67e4db64aa72732c474b44ea8622062c903f0
-- 
2.50.1

[PATCH v4 0/3] media: meson: Fix memory leak in error path in

[Anand Moon]

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>
Closes: https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t

Thanks
-Anand

Anand Moon (3):
  media: meson: vdec: Fix memory leak in error path of vdec_open
  media: meson: vdec: Add error handling for recycle thread creation
  media: meson: vdec: Cancel esparser work in error and stop paths

 drivers/staging/media/meson/vdec/vdec.c | 27 +++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)


base-commit: 8bc67e4db64aa72732c474b44ea8622062c903f0
-- 
2.50.1


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

[PATCH v4 0/3] media: meson: Fix memory leak in error path in

[Anand Moon]

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>
Closes: https://lore.kernel.org/all/20260520045905.6ACBA1F000E9@smtp.kernel.org/#t

Thanks
-Anand

Anand Moon (3):
  media: meson: vdec: Fix memory leak in error path of vdec_open
  media: meson: vdec: Add error handling for recycle thread creation
  media: meson: vdec: Cancel esparser work in error and stop paths

 drivers/staging/media/meson/vdec/vdec.c | 27 +++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)


base-commit: 8bc67e4db64aa72732c474b44ea8622062c903f0
-- 
2.50.1

[PATCH v4 1/3] 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>
---
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..9244fb09eb36 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:
@@ -926,6 +928,7 @@ static int vdec_close(struct file *file)
 
 	v4l2_m2m_ctx_release(sess->m2m_ctx);
 	v4l2_m2m_release(sess->m2m_dev);
+	v4l2_ctrl_handler_free(&sess->ctrl_handler);
 	v4l2_fh_del(&sess->fh, file);
 	v4l2_fh_exit(&sess->fh);
 
-- 
2.50.1

[PATCH v4 1/3] 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>
---
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..9244fb09eb36 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:
@@ -926,6 +928,7 @@ static int vdec_close(struct file *file)
 
 	v4l2_m2m_ctx_release(sess->m2m_ctx);
 	v4l2_m2m_release(sess->m2m_dev);
+	v4l2_ctrl_handler_free(&sess->ctrl_handler);
 	v4l2_fh_del(&sess->fh, file);
 	v4l2_fh_exit(&sess->fh);
 
-- 
2.50.1


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

[PATCH v4 1/3] 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>
---
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..9244fb09eb36 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:
@@ -926,6 +928,7 @@ static int vdec_close(struct file *file)
 
 	v4l2_m2m_ctx_release(sess->m2m_ctx);
 	v4l2_m2m_release(sess->m2m_dev);
+	v4l2_ctrl_handler_free(&sess->ctrl_handler);
 	v4l2_fh_del(&sess->fh, file);
 	v4l2_fh_exit(&sess->fh);
 
-- 
2.50.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect teardown order in `vdec_close()` causes use-after-destroy of the V4L2 control handler's mutex.
--

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

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

This commit introduces a proper err_m2m_ctx_release label in the error path
of vdec_open() to ensure the m2m context is released before releasing the m2m
device. It also attempts to free the V4L2 control handler memory in
vdec_close() to fix a resource leak.

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
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c

[ ... ]

> @@ -926,6 +928,7 @@ static int vdec_close(struct file *file)
>  
>  	v4l2_m2m_ctx_release(sess->m2m_ctx);
>  	v4l2_m2m_release(sess->m2m_dev);
> +	v4l2_ctrl_handler_free(&sess->ctrl_handler);

[Severity: High]
Could this specific ordering cause a use-after-destroy of the V4L2 control
handler's mutex?

If an application has subscribed to a V4L2 control event, v4l2_fh_exit()
unconditionally calls v4l2_event_unsubscribe_all(). This unsubscribe logic
eventually invokes v4l2_ctrl_del_event(), which attempts to lock the
control handler's mutex.

Since v4l2_ctrl_handler_free() destroys the mutex:

v4l2_ctrl_handler_free()
    mutex_destroy(&hdl->_lock);

Calling it before v4l2_fh_exit() could lead to operating on a destroyed mutex
and potentially cause a kernel panic if mutex debugging is enabled.

Should v4l2_ctrl_handler_free() be called after v4l2_fh_del() and
v4l2_fh_exit() instead?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521073449.10057-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 v4 1/3] media: meson: vdec: Fix memory leak in error path of vdec_open

[Anand Moon]

Hi Sashiko,

Thanks for your  review comment,
On Thu, 21 May 2026 at 13:39, <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Incorrect teardown order in `vdec_close()` causes use-after-destroy of the V4L2 control handler's mutex.
> --
>
> commit 82e916771a779b02500f97d58e344bb93ca8e9ca
> Author: Anand Moon <linux.amoon@gmail.com>
>
> media: meson: vdec: Fix memory leak in error path of vdec_open
>
> This commit introduces a proper err_m2m_ctx_release label in the error path
> of vdec_open() to ensure the m2m context is released before releasing the m2m
> device. It also attempts to free the V4L2 control handler memory in
> vdec_close() to fix a resource leak.
>
> 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
> > --- a/drivers/staging/media/meson/vdec/vdec.c
> > +++ b/drivers/staging/media/meson/vdec/vdec.c
>
> [ ... ]
>
> > @@ -926,6 +928,7 @@ static int vdec_close(struct file *file)
> >
> >       v4l2_m2m_ctx_release(sess->m2m_ctx);
> >       v4l2_m2m_release(sess->m2m_dev);
> > +     v4l2_ctrl_handler_free(&sess->ctrl_handler);
>
> [Severity: High]
> Could this specific ordering cause a use-after-destroy of the V4L2 control
> handler's mutex?
>
> If an application has subscribed to a V4L2 control event, v4l2_fh_exit()
> unconditionally calls v4l2_event_unsubscribe_all(). This unsubscribe logic
> eventually invokes v4l2_ctrl_del_event(), which attempts to lock the
> control handler's mutex.
>
> Since v4l2_ctrl_handler_free() destroys the mutex:
>
> v4l2_ctrl_handler_free()
>     mutex_destroy(&hdl->_lock);
>
> Calling it before v4l2_fh_exit() could lead to operating on a destroyed mutex
> and potentially cause a kernel panic if mutex debugging is enabled.
>
> Should v4l2_ctrl_handler_free() be called after v4l2_fh_del() and
> v4l2_fh_exit() instead?
>
> >       v4l2_fh_del(&sess->fh, file);
> >       v4l2_fh_exit(&sess->fh);
> >
I will try to study and update in the next version.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260521073449.10057-1-linux.amoon@gmail.com?part=1

Thanks
-Anand

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

[PATCH v4 2/3] media: meson: vdec: Add error handling for recycle thread creation

[Anand Moon]

Add proper error handling for kthread_run() in vdec_start_streaming().
If thread creation fails and returns an ERR_PTR, record the error,
reset sess->recycle_thread to NULL, and unwind resources via err_cleanup.
This prevents later calls to kthread_stop() in vdec_stop_streaming() from
dereferencing an ERR_PTR and causing a kernel panic. Fix this by adding the
label and invoking vdec_poweroff() to prevent hardware power leaks.

Additionally, reorder the error path to properly mirror the allocation
sequence clear the streamon status flags before emptying the M2M buffers
to avoid race conditions, and ensure DMA buffers are released gracefully
relative to the hardware state lifecycle.

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>
---
v4: new patch
[Severity: High]
This isn't a bug introduced by this patch, but does the driver verify if
kthread_run() returns an ERR_PTR when starting the recycle thread?

If thread creation fails in vdec_start_streaming() and returns an ERR_PTR,
could a later call to kthread_stop(sess->recycle_thread) in
vdec_stop_streaming() attempt to dereference that ERR_PTR and cause a
kernel panic?
---
 drivers/staging/media/meson/vdec/vdec.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 9244fb09eb36..8615a935e86d 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -337,29 +337,37 @@ 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;
+		}
+	}
 
 	sess->status = STATUS_INIT;
 	core->cur_sess = sess;
 	schedule_work(&sess->esparser_queue_work);
 	return 0;
 
+err_cleanup:
+	vdec_poweroff(sess);
 vififo_free:
 	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)
 		sess->streamon_out = 0;
 	else
 		sess->streamon_cap = 0;
 
+	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);
+
 	return ret;
 }
 
-- 
2.50.1

[PATCH v4 2/3] media: meson: vdec: Add error handling for recycle thread creation

[Anand Moon]

Add proper error handling for kthread_run() in vdec_start_streaming().
If thread creation fails and returns an ERR_PTR, record the error,
reset sess->recycle_thread to NULL, and unwind resources via err_cleanup.
This prevents later calls to kthread_stop() in vdec_stop_streaming() from
dereferencing an ERR_PTR and causing a kernel panic. Fix this by adding the
label and invoking vdec_poweroff() to prevent hardware power leaks.

Additionally, reorder the error path to properly mirror the allocation
sequence clear the streamon status flags before emptying the M2M buffers
to avoid race conditions, and ensure DMA buffers are released gracefully
relative to the hardware state lifecycle.

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>
---
v4: new patch
[Severity: High]
This isn't a bug introduced by this patch, but does the driver verify if
kthread_run() returns an ERR_PTR when starting the recycle thread?

If thread creation fails in vdec_start_streaming() and returns an ERR_PTR,
could a later call to kthread_stop(sess->recycle_thread) in
vdec_stop_streaming() attempt to dereference that ERR_PTR and cause a
kernel panic?
---
 drivers/staging/media/meson/vdec/vdec.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 9244fb09eb36..8615a935e86d 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -337,29 +337,37 @@ 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;
+		}
+	}
 
 	sess->status = STATUS_INIT;
 	core->cur_sess = sess;
 	schedule_work(&sess->esparser_queue_work);
 	return 0;
 
+err_cleanup:
+	vdec_poweroff(sess);
 vififo_free:
 	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)
 		sess->streamon_out = 0;
 	else
 		sess->streamon_cap = 0;
 
+	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);
+
 	return ret;
 }
 
-- 
2.50.1

[PATCH v4 2/3] media: meson: vdec: Add error handling for recycle thread creation

[Anand Moon]

Add proper error handling for kthread_run() in vdec_start_streaming().
If thread creation fails and returns an ERR_PTR, record the error,
reset sess->recycle_thread to NULL, and unwind resources via err_cleanup.
This prevents later calls to kthread_stop() in vdec_stop_streaming() from
dereferencing an ERR_PTR and causing a kernel panic. Fix this by adding the
label and invoking vdec_poweroff() to prevent hardware power leaks.

Additionally, reorder the error path to properly mirror the allocation
sequence clear the streamon status flags before emptying the M2M buffers
to avoid race conditions, and ensure DMA buffers are released gracefully
relative to the hardware state lifecycle.

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>
---
v4: new patch
[Severity: High]
This isn't a bug introduced by this patch, but does the driver verify if
kthread_run() returns an ERR_PTR when starting the recycle thread?

If thread creation fails in vdec_start_streaming() and returns an ERR_PTR,
could a later call to kthread_stop(sess->recycle_thread) in
vdec_stop_streaming() attempt to dereference that ERR_PTR and cause a
kernel panic?
---
 drivers/staging/media/meson/vdec/vdec.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 9244fb09eb36..8615a935e86d 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -337,29 +337,37 @@ 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;
+		}
+	}
 
 	sess->status = STATUS_INIT;
 	core->cur_sess = sess;
 	schedule_work(&sess->esparser_queue_work);
 	return 0;
 
+err_cleanup:
+	vdec_poweroff(sess);
 vififo_free:
 	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)
 		sess->streamon_out = 0;
 	else
 		sess->streamon_cap = 0;
 
+	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);
+
 	return ret;
 }
 
-- 
2.50.1


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

Re: [PATCH v4 2/3] media: meson: vdec: Add error handling for recycle thread creation

[sashiko-bot]

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

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.
--

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

media: meson: vdec: Add error handling for recycle thread creation

This commit adds error handling for kthread_run() in vdec_start_streaming()
to prevent a kernel panic when thread creation fails. It unwinds resources
and correctly reorders the error path to clear streamon flags before emptying
M2M buffers.

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 9244fb09eb36b..8615a935e86de 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -337,29 +337,37 @@ 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;

[Severity: High]
Does this error path leak sess->priv?

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.

Because kthread_run() failing prevents sess->status from being updated to
STATUS_INIT, any subsequent call to vdec_stop_streaming() will fail its
status check and bypass the entire cleanup block, permanently leaking
sess->priv and any associated DMA memory.

> +		}
> +	}
>  
>  	sess->status = STATUS_INIT;
>  	core->cur_sess = sess;

[Severity: Critical]
This is a pre-existing issue, but can an early hardware interrupt cause a
NULL pointer dereference here?

Because vdec_poweron() enables the decoding hardware and unmasks hardware
interrupts before core->cur_sess is assigned, if a hardware interrupt
triggers in between, the interrupt handler vdec_isr() will read a NULL
core->cur_sess. It then executes sess->last_irq_jiffies = get_jiffies_64(),
leading to a kernel panic. Adding the thread creation logic above widens
this vulnerable window.

[Severity: High]
This is a pre-existing issue, but can concurrent sessions bypass the hardware
exclusivity check and corrupt hardware state?

The exclusivity check at the start of vdec_start_streaming():

    if (core->cur_sess && core->cur_sess != sess) {

is executed without holding the core-level lock (core->lock). Because the
standard V4L2 queue lock (q->lock) is allocated per-session, two separate
processes opening the device concurrently can enter vdec_start_streaming()
at the same time, evaluate core->cur_sess == NULL as true, and concurrently
invoke vdec_poweron(sess), triggering simultaneous writes to the same
hardware registers.

>  	schedule_work(&sess->esparser_queue_work);
>  	return 0;
>  
> +err_cleanup:
> +	vdec_poweroff(sess);
>  vififo_free:
>  	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)
>  		sess->streamon_out = 0;
>  	else
>  		sess->streamon_cap = 0;
>  
> +	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);

[Severity: High]
This is a pre-existing issue, but will returning buffers for both queues upon
a single-queue failure orphan active queue buffers?

If vdec_start_streaming() fails (e.g., for the OUTPUT queue), the error path
unconditionally removes all buffers from both the source (OUTPUT) and
destination (CAPTURE) M2M queues and returns them with VB2_BUF_STATE_QUEUED.

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.

> +
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521073449.10057-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 v4 3/3] media: meson: vdec: Cancel esparser work in error and stop paths

[Anand Moon]

Ensure that esparser_queue_work is canceled before freeing the
session context. Add cancel_work_sync() in both the error path
of vdec_close() and vdec_start_streaming() and in vdec_stop_streaming().

This prevents background work from dereferencing a freed sess
structure and triggering a use-after-free.

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>
---
v4: new patch
 If vdec_close() calls kfree(sess) without first stopping or synchronizing
with this background work via cancel_work_sync(), could a concurrently
running esparser_queue_all_src() dereference the freed sess structure and
trigger a use-after-free?
---
 drivers/staging/media/meson/vdec/vdec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 8615a935e86d..a57bd4a8e33c 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -358,6 +358,8 @@ 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:
+	cancel_work_sync(&sess->esparser_queue_work);
+
 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
 		sess->streamon_out = 0;
 	else
@@ -415,6 +417,7 @@ static void vdec_stop_streaming(struct vb2_queue *q)
 		if (vdec_codec_needs_recycle(sess))
 			kthread_stop(sess->recycle_thread);
 
+		cancel_work_sync(&sess->esparser_queue_work);
 		vdec_poweroff(sess);
 		vdec_free_canvas(sess);
 		dma_free_coherent(sess->core->dev, sess->vififo_size,
@@ -937,6 +940,7 @@ static int vdec_close(struct file *file)
 	v4l2_m2m_ctx_release(sess->m2m_ctx);
 	v4l2_m2m_release(sess->m2m_dev);
 	v4l2_ctrl_handler_free(&sess->ctrl_handler);
+	cancel_work_sync(&sess->esparser_queue_work);
 	v4l2_fh_del(&sess->fh, file);
 	v4l2_fh_exit(&sess->fh);
 
-- 
2.50.1

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

[Anand Moon]

Ensure that esparser_queue_work is canceled before freeing the
session context. Add cancel_work_sync() in both the error path
of vdec_close() and vdec_start_streaming() and in vdec_stop_streaming().

This prevents background work from dereferencing a freed sess
structure and triggering a use-after-free.

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>
---
v4: new patch
 If vdec_close() calls kfree(sess) without first stopping or synchronizing
with this background work via cancel_work_sync(), could a concurrently
running esparser_queue_all_src() dereference the freed sess structure and
trigger a use-after-free?
---
 drivers/staging/media/meson/vdec/vdec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 8615a935e86d..a57bd4a8e33c 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -358,6 +358,8 @@ 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:
+	cancel_work_sync(&sess->esparser_queue_work);
+
 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
 		sess->streamon_out = 0;
 	else
@@ -415,6 +417,7 @@ static void vdec_stop_streaming(struct vb2_queue *q)
 		if (vdec_codec_needs_recycle(sess))
 			kthread_stop(sess->recycle_thread);
 
+		cancel_work_sync(&sess->esparser_queue_work);
 		vdec_poweroff(sess);
 		vdec_free_canvas(sess);
 		dma_free_coherent(sess->core->dev, sess->vififo_size,
@@ -937,6 +940,7 @@ static int vdec_close(struct file *file)
 	v4l2_m2m_ctx_release(sess->m2m_ctx);
 	v4l2_m2m_release(sess->m2m_dev);
 	v4l2_ctrl_handler_free(&sess->ctrl_handler);
+	cancel_work_sync(&sess->esparser_queue_work);
 	v4l2_fh_del(&sess->fh, file);
 	v4l2_fh_exit(&sess->fh);
 
-- 
2.50.1

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

[Anand Moon]

Ensure that esparser_queue_work is canceled before freeing the
session context. Add cancel_work_sync() in both the error path
of vdec_close() and vdec_start_streaming() and in vdec_stop_streaming().

This prevents background work from dereferencing a freed sess
structure and triggering a use-after-free.

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>
---
v4: new patch
 If vdec_close() calls kfree(sess) without first stopping or synchronizing
with this background work via cancel_work_sync(), could a concurrently
running esparser_queue_all_src() dereference the freed sess structure and
trigger a use-after-free?
---
 drivers/staging/media/meson/vdec/vdec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 8615a935e86d..a57bd4a8e33c 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -358,6 +358,8 @@ 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:
+	cancel_work_sync(&sess->esparser_queue_work);
+
 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
 		sess->streamon_out = 0;
 	else
@@ -415,6 +417,7 @@ static void vdec_stop_streaming(struct vb2_queue *q)
 		if (vdec_codec_needs_recycle(sess))
 			kthread_stop(sess->recycle_thread);
 
+		cancel_work_sync(&sess->esparser_queue_work);
 		vdec_poweroff(sess);
 		vdec_free_canvas(sess);
 		dma_free_coherent(sess->core->dev, sess->vififo_size,
@@ -937,6 +940,7 @@ static int vdec_close(struct file *file)
 	v4l2_m2m_ctx_release(sess->m2m_ctx);
 	v4l2_m2m_release(sess->m2m_dev);
 	v4l2_ctrl_handler_free(&sess->ctrl_handler);
+	cancel_work_sync(&sess->esparser_queue_work);
 	v4l2_fh_del(&sess->fh, file);
 	v4l2_fh_exit(&sess->fh);
 
-- 
2.50.1


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

Re: [PATCH v4 3/3] 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:

New issues:
- [Critical] AB-BA Deadlock in vdec_stop_streaming() and vdec_start_streaming().
- [High] Workqueue race condition and unexpected rescheduling in vdec_stop_streaming().

Pre-existing issues:
- [Critical] Use-After-Free in vdec_close() due to incorrect work cancellation order.
--

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

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

This commit attempts to fix a use-after-free issue by cancelling the
esparser_queue_work before freeing the session context. It adds
cancel_work_sync() calls in the error paths of vdec_start_streaming(),
in vdec_stop_streaming(), and 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
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -358,6 +358,8 @@ 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:
> +	cancel_work_sync(&sess->esparser_queue_work);
> +

[Severity: Critical]
Does this cancel_work_sync() call introduce a deadlock?

These are V4L2 queue callbacks, which are invoked by the V4L2 core with
the queue lock held. Specifically, __video_do_ioctl acquires &sess->lock
before calling the VIDIOC_STREAMOFF or VIDIOC_STREAMON ioctls.

However, the worker thread esparser_queue_all_src() begins execution by
attempting to acquire the exact same lock with mutex_lock(&sess->lock).

If a user calls VIDIOC_STREAMOFF while the worker thread is executing,
the ioctl thread will block indefinitely on cancel_work_sync() waiting
for the worker to finish, while the worker is permanently blocked waiting
for sess->lock held by the ioctl thread.

>  	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>  		sess->streamon_out = 0;

[ ... ]

> @@ -415,6 +417,7 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  		if (vdec_codec_needs_recycle(sess))
>  			kthread_stop(sess->recycle_thread);
>  
> +		cancel_work_sync(&sess->esparser_queue_work);
>  		vdec_poweroff(sess);

[Severity: High]
Could this placement of cancel_work_sync() leave the background work
pending after the stream is fully stopped?

The call to vdec_poweroff(sess) waits for the hardware to become inactive.
During this time, the hardware is still running and can complete a buffer,
triggering a codec interrupt.

The interrupt handler calls amvdec_dst_buf_done(), which explicitly calls
schedule_work(&sess->esparser_queue_work). Since the work is scheduled
after cancel_work_sync() has already returned, the work could be unexpectedly
left pending.

>  		vdec_free_canvas(sess);
>  		dma_free_coherent(sess->core->dev, sess->vififo_size,

[ ... ]

> @@ -937,6 +940,7 @@ static int vdec_close(struct file *file)
>  	v4l2_m2m_ctx_release(sess->m2m_ctx);
>  	v4l2_m2m_release(sess->m2m_dev);
>  	v4l2_ctrl_handler_free(&sess->ctrl_handler);
> +	cancel_work_sync(&sess->esparser_queue_work);

[Severity: Critical]
This isn't a bug introduced by this patch, but the patch's attempt to fix it
is incomplete.

Could placing cancel_work_sync() after v4l2_m2m_ctx_release() lead to
a use-after-free?

The function v4l2_m2m_ctx_release() explicitly frees sess->m2m_ctx. If
the background work is rescheduled before this final cancellation (for
example, due to the late hardware interrupt during vdec_stop_streaming()
described above), the worker thread esparser_queue_all_src() can execute.

The worker thread iterates over buffers using the freed context, which
dereferences sess->m2m_ctx before cancel_work_sync() can stop it. Should
the work be cancelled before releasing the context?

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

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

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