Re: [PATCH v6 02/14] media: mediatek: vcodec: add driver to support vcp

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Destructive `request_module` polling loop causes a massive modprobe fork bomb and breaks deferred probe.
- [High] A timeout in `mtk_vcodec_vcp_msg_ipi_send()` leaves the system vulnerable to late ACKs, causing host-firmware state desynchronization.
- [High] `ipi_mutex` serializes the entire `wait_event_interruptible_timeout()`, breaking IPI concurrency and risking deadlocks.
- [Medium] Early return in `mtk_vcodec_vcp_release()` leaks remoteproc device reference and breaks teardown state.
- [Medium] Missing IPC message length validation enables potential out-of-bounds reads from untrusted firmware.
- [Low] Unused variable `fw_type` in encoder and decoder probe functions.
--

--- Patch [2]: [PATCH v6 02/14] media: mediatek: vcodec: add driver to support vcp ---
commit 06d72e77d54f36511b46df20abe564b636b8dc79
Author: Kyrie Wu <kyrie.wu@mediatek.com>
Subject: media: mediatek: vcodec: add driver to support vcp

This patch changes the processor from scp to vcp for the mt8196 platform.
It adds a new firmware interface to handle communication between the kernel
and the vcp.

> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
[ ... ]
> +static int mtk_vcodec_vcp_msg_process_thread(void *arg)
> +{
[ ... ]
> +		handler = fw->vcp->ipi_desc[obj->id].handler;
> +		if (!handler) {
> +			dev_err(&fw->pdev->dev, "invalid ack ipi handler id = %d\n", obj->id);
> +			mtk_vcodec_vcp_ipi_unlock(fw->vcp, obj->id);
> +			mtk_vcodec_vcp_free_msg_node(fw, msg_node);
> +			continue;
> +		}
> +
> +		handler(msg, obj->len, fw->vcp->ipi_desc[obj->id].priv);

[Severity: Medium]
Could this pass an arbitrarily large length to the handler?
The firmware-provided obj->len does not appear to be validated against the
boundaries of VCP_SHARE_BUF_SIZE (64 bytes). If the firmware provides an
out-of-bounds length, it might lead to out-of-bounds reads in the receiving
handlers.

> +		mtk_vcodec_vcp_ipi_unlock(fw->vcp, obj->id);
[ ... ]
> +static int mtk_vcodec_vcp_msg_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
> +				       unsigned int len, unsigned int wait)
> +{
[ ... ]
> +	mutex_lock(&vcp->ipi_mutex);
> +	feature_id = VDEC_FEATURE_ID;
> +	mailbox_id = IPI_OUT_VDEC_1;
[ ... ]
> +wait_ack:
> +	/* wait for VCP's ACK */
> +	ret = wait_event_interruptible_timeout(*msg_wq, *msg_signaled,
> +					       msecs_to_jiffies(IPI_TIMEOUT_MS));

[Severity: High]
Does holding ipi_mutex across this sleep serialize all VCP communication?
Acquiring the mutex before sending the IPC message and holding it while
waiting for the ACK seems to single-thread all communication, which might
defeat the purpose of the per-ID waitqueues.

Also, if any registered IPI handler executing in the receiving msg_thread
needs to send a followup command via mtk_vcodec_vcp_msg_ipi_send(), could
it deadlock trying to acquire ipi_mutex while this thread holds it?

> +	if (!ret) {
> +		vcp->ipi_id_ack[id] = VCODEC_IPI_MSG_STATUS_FAIL;
> +		dev_err(&fw->pdev->dev, "wait ipi ack timeout! %d %d\n", ret, vcp->ipi_id_ack[id]);

[Severity: High]
Can a timeout here lead to state desynchronization with the firmware?
If the wait times out, the function returns an error and releases ipi_mutex
without invalidating the transaction (e.g., via a sequence number).

If the firmware later completes the abandoned task and sends a delayed ACK,
msg_thread will set msg_signaled[id] = true and call wake_up(). If another
thread subsequently reuses this id for a new command, could it be prematurely
awakened by the stale ACK, causing the kernel to reuse or free memory buffers
while the firmware is still accessing them?

[ ... ]
> +static int mtk_vcodec_vcp_get_vcp_device(struct mtk_vcodec_fw *fw)
> +{
> +	struct device *dev = &fw->pdev->dev;
> +	int retry = 0, retry_cnt = 10000;
> +	phandle vcp_phandle;
> +
> +	while (!try_then_request_module(mtk_vcodec_vcp_driver_loaded(fw), "mtk-vcp")) {
> +		if (++retry > retry_cnt) {
> +			dev_err(dev, "failed to load mtk-vcp module");
> +			return -EPROBE_DEFER;
> +		}
> +		usleep_range(1000, 2000);
> +	}

[Severity: High]
Could this polling loop cause a modprobe fork bomb?
If the mtk-vcp module is missing or unable to load, this loop executes up to
10,000 times, synchronously calling request_module() on each iteration. This
will spawn an excessive number of user-space modprobe processes.

[ ... ]
> +static int mtk_vcodec_vcp_load_firmware(struct mtk_vcodec_fw *fw)
> +{
[ ... ]
> +	if (mtk_vcodec_vcp_get_vcp_device(fw) < 0) {
> +		dev_err(&fw->pdev->dev, "vcp device is null.\n");
> +		return -EINVAL;
> +	}

[Severity: High]
Is it intentional to overwrite the -EPROBE_DEFER return value from
mtk_vcodec_vcp_get_vcp_device() with -EINVAL here? This will permanently
break the deferred probe mechanism for this device if the module is simply
not loaded yet.

[ ... ]
> +static void mtk_vcodec_vcp_release(struct mtk_vcodec_fw *fw)
> +{
[ ... ]
> +	ret = vcp_device->ops->deregister_feature(vcp_device, fw->vcp->feature_id);
> +	if (ret < 0) {
> +		dev_err(dev, "deregister feature_id(%d) fail(%d)\n", fw->vcp->feature_id, ret);
> +		return;
> +	}
> +
> +	fw->vcp->is_register_done = false;
> +	fw->vcp->is_init_done = false;
> +
> +put_device:
> +	mtk_vcodec_vcp_put_device(fw);
> +}

[Severity: Medium]
Will this early return leak the remoteproc device reference?
If deregister_feature fails, returning early skips mtk_vcodec_vcp_put_device(),
which leaves the rproc reference elevated and prevents the remoteproc module
from being cleanly unloaded.

> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
[ ... ]
> @@ -376,19 +376,22 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
>  	if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
>  				  &rproc_phandle)) {
>  		fw_type = VPU;
> +		dev->fw_init = mtk_vcodec_fw_vpu_init;
>  	} else if (!of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
>  					 &rproc_phandle)) {
>  		fw_type = SCP;
> +		dev->fw_init = mtk_vcodec_fw_scp_init;
>  	} else if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vcp",
>  					 &rproc_phandle)) {
>  		fw_type = VCP;
> +		dev->fw_init = mtk_vcodec_fw_vcp_init;
>  	} else {

[Severity: Low]
It looks like the fw_type variable is assigned here, but is never used anywhere
else in the function since the firmware selection was refactored to use
fw_init. Is there any remaining need for fw_type?

> --- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c
[ ... ]
> @@ -259,16 +259,18 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
>  	if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
>  				  &rproc_phandle)) {
>  		fw_type = VPU;
> +		dev->fw_init = mtk_vcodec_fw_vpu_init;
>  	} else if (!of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
>  					 &rproc_phandle)) {
>  		fw_type = SCP;
> +		dev->fw_init = mtk_vcodec_fw_scp_init;

[Severity: Low]
Similarly, fw_type appears to be dead code here as well, since it is no longer
passed to mtk_vcodec_fw_select().

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