Re: [PATCH v7 01/19] media: meson: vdec: Fix m2m device lifetime and cleanup path

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

New issues:
- [High] Moving `v4l2_m2m_dev` allocation to the driver core introduces an exploitable Use-After-Free (UAF) during device removal when file descriptors remain open.

Pre-existing issues:
- [High] The `amvdec_core` structure is improperly allocated using `devm_kzalloc`, leading to Use-After-Free bugs when the device is unbound.
--

--- Patch [1]: [PATCH v7 01/19] media: meson: vdec: Fix m2m device lifetime and cleanup path ---
commit f97dc3a43da5c78cbae7f710eb7770593a75ff3e
Author: Anand Moon <[email protected]>

media: meson: vdec: Fix m2m device lifetime and cleanup path

The vdec driver was incorrectly initializing a new v4l2_m2m device
instance per session inside vdec_open() and releasing it in vdec_close().
This design is faulty because the m2m device models the core hardware
engine and must persist across multiple sessions.

Closes: https://lore.kernel.org/all/[email protected]/#t

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index a039d925c0fe5..6ae3471155a87 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c

[ ... ]

> @@ -1105,6 +1106,7 @@ static void vdec_remove(struct platform_device *pdev)
>  	struct amvdec_core *core = platform_get_drvdata(pdev);
>  
>  	video_unregister_device(core->vdev_dec);
> +	v4l2_m2m_release(core->m2m_dev);

[Severity: High]
Does releasing the m2m device here introduce a use-after-free if a file
descriptor remains open?

If a user unbinds the driver, vdec_remove() synchronously frees core->m2m_dev.
However, if an application still holds a file descriptor open, it will
eventually close it, triggering vdec_close():

vdec_close()
  v4l2_m2m_ctx_release(sess->m2m_ctx)
    v4l2_m2m_cancel_job()
      spin_lock_irqsave(&m2m_dev->job_spinlock, flags);

Since core->m2m_dev was already freed during unbind, could this dereference
cause memory corruption or a crash?

[Severity: High]
This is a pre-existing issue, but does returning from vdec_remove() without
waiting for open file descriptors to close lead to a use-after-free of the
core driver structure?

The core structure and its mutex are allocated via devres (devm_kzalloc)
during vdec_probe(). If the device is unbound, devres will automatically free
the core structure as soon as vdec_remove() returns.

If an ioctl is currently blocked waiting on vdev->lock (which points to
core->lock), it will attempt to unlock a freed mutex when it resumes.

Additionally, if the asynchronous esparser_queue_work runs after unbind, it
will dereference the freed sess->core.

Could the driver synchronize active sessions or use a different lifecycle for
the core structure to prevent this?

>  	v4l2_device_unregister(&core->v4l2_dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic