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

[Anand Moon <linux.amoon@gmail.com>]

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