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

[sashiko-bot@kernel.org]

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