Re: [PATCH v6 1/8] media: meson: vdec: Fix memory leaks and lifetime of m2m device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-After-Free of `m2m_dev` and `core` on driver unbind.
- [Medium] Logically backwards error unwinding in `vdec_open()` introduces dead code and fails to fix a resource leak.
--

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

media: meson: vdec: Fix memory leaks and lifetime of m2m device

This commit shifts the v4l2_m2m_init() call to vdec_probe() and releases
it in vdec_remove() to address a design flaw where the m2m device was bound
to the session. It also attempts to resolve resource leaks in the driver's
error handling paths.

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")

> 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
[ ... ]
> @@ -873,23 +873,16 @@ static int vdec_open(struct file *file)
>  
>  	sess->core = core;
>  
> -	sess->m2m_dev = v4l2_m2m_init(&vdec_m2m_ops);
> -	if (IS_ERR(sess->m2m_dev)) {
> -		dev_err(dev, "Fail to v4l2_m2m_init\n");
> -		ret = PTR_ERR(sess->m2m_dev);
> -		goto err_free_sess;
> -	}
> -
> -	sess->m2m_ctx = v4l2_m2m_ctx_init(sess->m2m_dev, sess, m2m_queue_init);
> +	sess->m2m_ctx = v4l2_m2m_ctx_init(core->m2m_dev, sess, m2m_queue_init);
>  	if (IS_ERR(sess->m2m_ctx)) {
>  		dev_err(dev, "Fail to v4l2_m2m_ctx_init\n");
>  		ret = PTR_ERR(sess->m2m_ctx);
> -		goto err_m2m_release;
> +		goto err_fh_del;

[Severity: Medium]
Is this error unwinding logic in the correct order? Jumps to err_fh_del here
will call v4l2_fh_exit() on sess->fh before it has been initialized further
down in vdec_open().

>  	}
>  
>  	ret = vdec_init_ctrls(sess);
>  	if (ret)
> -		goto err_m2m_release;
> +		goto err_free_sess;

[Severity: Medium]
Does skipping err_fh_del here lead to a memory leak? If vdec_init_ctrls()
fails, jumping directly to err_free_sess bypasses freeing the successfully
allocated sess->m2m_ctx.

[ ... ]
> @@ -925,9 +918,9 @@ static int vdec_close(struct file *file)
>  	struct amvdec_session *sess = file_to_amvdec_session(file);
>  
>  	v4l2_m2m_ctx_release(sess->m2m_ctx);

[Severity: High]
Can this result in a use-after-free if the user holds the file descriptor
open after the driver is unbound?

During driver unbind, vdec_remove() explicitly frees core->m2m_dev and
devres cleans up the amvdec_core structure.

If a user subsequently closes an open file descriptor, vdec_close()
calls v4l2_m2m_ctx_release(), which will dereference the freed m2m_dev
pointer inside v4l2_m2m_cancel_job().

Additionally, any pending jobs may trigger vdec_m2m_job_abort(), which
dereferences the freed sess->core and sess->core->m2m_dev.

> -	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);
[ ... ]
> @@ -1104,6 +1106,7 @@ static void vdec_remove(struct platform_device *pdev)
>  
>  	video_unregister_device(core->vdev_dec);
>  	v4l2_device_unregister(&core->v4l2_dev);
> +	v4l2_m2m_release(core->m2m_dev);
>  }

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

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