Re: [PATCH v6 01/14] media: mediatek: vcodec: support vcp architecture

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch adds support for the VCP architecture but fails to handle the VCP enum in mtk_vcodec_fw_select(), resulting in guaranteed probe failure.

Pre-existing issues:
- [Critical] The driver destroys decode_workqueue before unregistering the video device, allowing concurrent ioctls to queue work to a destroyed workqueue.
- [Critical] The driver state structure (dev) is managed by devm_kzalloc but is accessed by file operations that outlive the device binding, leading to a Use-After-Free.
- [High] The video device allocated via video_device_alloc() leaks on multiple probe error paths.
- [High] Child platform devices created by of_platform_populate() are never cleaned up.
- [High] The ordered workqueue 'dev->core_workqueue' leaks in mtk_vcodec_dec_remove().
--

--- Patch [1]: [PATCH v6 01/14] media: mediatek: vcodec: support vcp architecture ---
commit 86e187a5e23e9a4483af39ff82e14ad66126f95a
Author: Kyrie Wu <kyrie.wu@mediatek.com>
Subject: media: mediatek: vcodec: support vcp architecture

This commit adds support for the VCP architecture for the video codec on
some platforms. It detects the VCP coprocessor in the device tree and sets
the VCP architecture type.

> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.h b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.h
> index 300363a40158c..c1642fb09b42a 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.h
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.h
> @@ -14,6 +14,7 @@ struct mtk_vcodec_enc_dev;
>  enum mtk_vcodec_fw_type {
>  	VPU,
>  	SCP,
> +	VCP,
>  };
>  
>  enum mtk_vcodec_fw_use {

[ ... ]

> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> index e936ed8dffbaf..d220b645e4556 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> @@ -379,6 +379,9 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
>  	} else if (!of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
>  					 &rproc_phandle)) {
>  		fw_type = SCP;
> +	} else if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vcp",
> +					 &rproc_phandle)) {
> +		fw_type = VCP;

[Severity: High]
If the mediatek,vcp property is present, probe sets fw_type = VCP and later
calls mtk_vcodec_fw_select(). However, since mtk_vcodec_fw_select() does not
have a case for VCP, won't it hit the default case and return ERR_PTR(-EINVAL),
causing probe to unconditionally fail?

mtk_vcodec_fw.c:mtk_vcodec_fw_select() {
        ...
        default:
                dev_err(&plat_dev->dev, "Invalid vcodec fw type");
                return ERR_PTR(-EINVAL);
}

[Severity: High]
This is a pre-existing issue, but does the video device allocated by
video_device_alloc() leak on multiple error paths in mtk_vcodec_probe()?

If initialization steps like v4l2_m2m_init(), alloc_ordered_workqueue(), or
of_platform_populate() fail, the code jumps to error labels like
err_dec_alloc, err_event_workq, or err_reg_cont, which skip calling
video_device_release(vfd_dec):

err_dec_alloc:
        v4l2_device_unregister(&dev->v4l2_dev);

[Severity: High]
This isn't a bug introduced by this patch, but are the child platform devices
created by of_platform_populate() in mtk_vcodec_probe() ever cleaned up?

If probe fails after of_platform_populate(), or when the driver is unbound,
mtk_vcodec_dec_remove() executes but appears to never call
of_platform_depopulate() to unregister the child devices. Can this cause state
corruption and resource leaks on subsequent probes?

[Severity: High]
This is a pre-existing issue, but does the ordered workqueue
dev->core_workqueue leak when the driver is unbound?

In mtk_vcodec_probe(), alloc_ordered_workqueue() allocates dev->core_workqueue
for the VDEC_LAT architecture. However, mtk_vcodec_dec_remove() destroys
decode_workqueue but seems to completely omit destroying core_workqueue:

mtk_vcodec_dec_drv.c:mtk_vcodec_dec_remove() {
        ...
        destroy_workqueue(dev->decode_workqueue);
        ...
}

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free window during
driver removal?

In mtk_vcodec_dec_remove(), destroy_workqueue(dev->decode_workqueue) is
executed before video_unregister_device(dev->vfd_dec). During this window,
could userspace invoke V4L2 ioctls that call m2mops_vdec_device_run(), which
executes queue_work() on the already freed workqueue?

mtk_vcodec_dec.c:m2mops_vdec_device_run() {
        ...
        queue_work(dev->decode_workqueue, &ctx->decode_work);
}

[Severity: Critical]
This is a pre-existing issue, but can the dev state structure be accessed
after it is freed by devres?

The driver state dev is allocated with devm_kzalloc in mtk_vcodec_probe() and
is automatically freed when mtk_vcodec_dec_remove() returns. If a process
holds /dev/videoX open during unbind, the file descriptor remains valid.

When the application later closes it, fops_vcodec_release() executes and
dereferences the freed dev pointer to acquire dev->dev_mutex:

mtk_vcodec_dec_drv.c:fops_vcodec_release() {
        struct mtk_vcodec_dec_dev *dev = video_drvdata(file);
        ...
        mutex_lock(&dev->dev_mutex);
}

>  	} else {
>  		dev_dbg(&pdev->dev, "Could not get vdec IPI device");
>  		return -ENODEV;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720012056.1026551-1-kyrie.wu@mediatek.com?part=1