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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider:
- [Critical] The untrusted firmware-provided message ID is used directly as an array index without bounds checking, enabling arbitrary out-of-bounds memory accesses.
- [High] The Kconfig symbol MTK_VCP_RPROC used in a select statement does not exist in the kernel tree.
- [High] The driver fails to stop the message processing kthread, unregister the IPC mailbox, and unregister the notifier during module removal, leading to a Use-After-Free and kernel panic.
- [High] Inverted logic in `check_vcp_loaded()` causes a 15-second busy-loop and kernel log spam when the VCP driver is not loaded.
- [High] The kernel message processing thread permanently exits upon encountering an unregistered message ID, causing a Denial of Service for all subsequent IPC.
- [High] The per-IPI mutex for `VCP_IPI_CORE_DECODER` is never initialized, leading to a kernel panic when locked.
- [High] Negative error codes are assigned to the boolean array `ipi_id_ack`, which truncates them to `true` (1) and silently masks IPC failures.
- [High] Failing to unwind previously registered callbacks on the error path in `mtk_vcodec_vcp_load_firmware` leaves dangling references.
- [High] A stale ACK from a timed-out IPC message can cause subsequent IPC messages to prematurely succeed without waiting.
- [Medium] Failure to create the IPC processing kthread is silently ignored.
- [Medium] The driver bus-spins the CPU with `mdelay(1)` while holding a mutex, causing significant latency spikes.
- [Medium] The message processing kthread uses `wait_event_interruptible()` without including `kthread_should_stop()` in the wake condition, guaranteeing a deadlock if `kthread_stop()` is called.
- [Medium] The VCP device handle is retrieved but never released, leaking the reference.
- [Low] The code uses undefined macros and headers, guaranteeing compilation failure.
- [Low] Dead code in `mtk_vcodec_vcp_msg_ipi_send()` checks for impossible negative return values from `wait_event_timeout()`.
--

commit a24833f019af1320e17d5afbc734268e762b3e20
Author: Yunfei Dong <yunfei.dong@mediatek.com>

media: mediatek: vcodec: add driver to support vcp

The processor is changed from scp to vcp in mt8196 platform.
Adding new firmware interface to communicate kernel with vcp
for the communication method is changed.

> diff --git a/drivers/media/platform/mediatek/vcodec/Kconfig b/drivers/media/platform/mediatek/vcodec/Kconfig
> index bc8292232530b..d23dad5c78ce3 100644
> --- a/drivers/media/platform/mediatek/vcodec/Kconfig
> +++ b/drivers/media/platform/mediatek/vcodec/Kconfig
> @@ -21,6 +24,7 @@ config VIDEO_MEDIATEK_VCODEC
>  	select V4L2_MEM2MEM_DEV
>  	select VIDEO_MEDIATEK_VCODEC_VPU if VIDEO_MEDIATEK_VPU
>  	select VIDEO_MEDIATEK_VCODEC_SCP if MTK_SCP
> +	select VIDEO_MEDIATEK_VCODEC_VCP if MTK_VCP_RPROC

[Severity: High]
Does this select statement reference a missing config symbol? According to the
Kconfig subsystem guidelines, referencing a non-existent config symbol in a
select statement leaves the intended target unselectable or causes silent
failures. A search across the kernel tree shows that MTK_VCP_RPROC is never
defined.

> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> new file mode 100644
> index 0000000000000..2f3bfcd97958a
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> @@ -0,0 +1,505 @@
[ ... ]
> +#include <linux/remoteproc/mtk_vcp_public.h>
> +#include <linux/firmware/mediatek/mtk-vcp-ipc.h>

[Severity: Low]
Does the mtk_vcp_public.h header exist in the kernel tree? Relying on
undeclared headers will lead to direct build errors.

[ ... ]
> +static int mtk_vcodec_vcp_msg_process_thread(void *arg)
> +{
[ ... ]
> +	do {
> +		ret = wait_event_interruptible(fw->vcp->msg_queue.wq,
> +					       atomic_read(&fw->vcp->msg_queue.cnt) > 0);

[Severity: Medium]
Will this wait condition prevent the kthread from properly stopping if
kthread_stop() is called during driver unload? Since kthread_should_stop()
is not included in the wait condition, waking the thread for teardown could
cause it to immediately go back to sleep.

[ ... ]
> +		obj = &msg_node->ipi_data;
> +		msg = (struct vdec_vpu_ipi_ack *)obj->share_buf;
[ ... ]
> +		mtk_vcodec_vcp_ipi_lock(fw->vcp, obj->id);
> +		handler = fw->vcp->ipi_desc[obj->id].handler;

[Severity: Critical]
Can this code cause an out-of-bounds memory access? The untrusted
firmware-provided obj->id is used directly as an array index without bounds
checking. A compromised or buggy firmware returning an ID >= VCP_IPI_MAX
could cause the kernel to access arbitrary memory.

> +		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);
> +			return -EINVAL;
> +		}

[Severity: High]
Should this return -EINVAL instead be a continue? Returning an error here
aborts the while loop and permanently terminates the kthread context. All
subsequent VCP IPC messages will time out, completely breaking the decoder
until the driver is reloaded.

[ ... ]
> +static int mtk_vcodec_vcp_msg_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
> +				       unsigned int len, unsigned int wait)
> +{
> +	struct mtk_vcp *vcp = fw->vcp;
[ ... ]
> +	mutex_lock(&vcp->ipi_mutex);
> +	feature_id = VDEC_FEATURE_ID;
> +	mailbox_id = IPI_OUT_VDEC_1;
> +
> +	timeout_jiffies = jiffies + msecs_to_jiffies(VCP_SYNC_TIMEOUT_MS);

[Severity: Low]
Is VCP_SYNC_TIMEOUT_MS defined anywhere in the kernel tree? Relying on an
undeclared macro will cause compilation failures.

> +	while (!vcp_device->ops->vcp_is_ready(feature_id)) {
> +		if (time_after(jiffies, timeout_jiffies)) {
> +			vcp->ipi_id_ack[id] = -EINVAL;

[Severity: High]
Does assigning a negative error code to ipi_id_ack mask IPC failures?
Because ipi_id_ack is defined as a boolean array in struct mtk_vcp,
negative error codes like -EINVAL will truncate to true (1). Since the
function returns this value at the end, callers expecting standard negative
error codes might incorrectly assume the IPC succeeded.

> +			ret = -EINVAL;
> +			goto error;
> +		}
> +		mdelay(1);
> +	}

[Severity: Medium]
Is it safe to use mdelay(1) in a loop while holding a mutex? Because the
code holds vcp->ipi_mutex, it is in a sleepable context. Using mdelay() here
bus-spins the CPU for up to the timeout duration, wasting CPU cycles and
causing significant latency spikes.

[ ... ]
> +wait_ack:
> +	/* wait for VCP's ACK */
> +	ret = wait_event_timeout(*msg_wq, *msg_signaled, msecs_to_jiffies(IPI_TIMEOUT_MS));
> +	if (!ret || retry_cnt > 5) {
> +		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]);
> +	} else if (ret == -ERESTARTSYS) {
> +		dev_err(&fw->pdev->dev, "wait ipi ack err (%d)\n", vcp->ipi_id_ack[id]);
> +		retry_cnt++;
> +		goto wait_ack;
> +	} else if (ret < 0) {
> +		dev_err(&fw->pdev->dev, "wait ipi ack fail ret %d %d\n", ret, vcp->ipi_id_ack[id]);
> +		vcp->ipi_id_ack[id] = VCODEC_IPI_MSG_STATUS_FAIL;
> +	}

[Severity: Low]
Are the checks for -ERESTARTSYS and negative return values dead code?
wait_event_timeout() strictly returns a positive value (remaining jiffies)
or 0 (timeout). It never returns negative error codes.

[ ... ]
> +	*msg_signaled = false;
> +	mutex_unlock(&vcp->ipi_mutex);

[Severity: High]
Should msg_signaled be cleared before sending the IPC message instead of
after? If a previous IPC request times out, and the firmware delivers a late
ACK, the ISR sets the flag to true. The next call to send a message would
then see the flag as already true and return immediately without waiting,
potentially reading stale data.

[ ... ]
> +static int check_vcp_loaded(struct mtk_vcodec_fw *fw)
> +{
> +	struct device *dev = &fw->pdev->dev;
> +	struct device_driver *drv;
> +
> +	drv = driver_find("mtk-vcp", &platform_bus_type);
> +	if (!drv) {
> +		dev_err(dev, "find mtk-vcp driver failed, need to reload.");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +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(check_vcp_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]
Does this loop inadvertently busy-wait based on an inverted condition?
check_vcp_loaded() returns -EINVAL when the driver is not found.
try_then_request_module() evaluates non-zero values as true. Consequently,
if the driver is missing, this evaluates as true, skipping the module request
and keeping the while loop running for up to 10,000 iterations while flooding
the kernel log.

[ ... ]
> +	fw->vcp->vcp_device = mtk_vcp_get_by_phandle(vcp_phandle);
> +	if (!fw->vcp->vcp_device) {
> +		dev_err(dev, "get vcp device failed\n");
> +		return -ENODEV;
> +	}

[Severity: Medium]
Is the reference acquired by mtk_vcp_get_by_phandle() ever released?
APIs retrieving a device by phandle increment the reference count, but there
is no corresponding put operation in the teardown or release code, which
permanently pins the remote processor driver in memory.

[ ... ]
> +static int mtk_vcodec_vcp_load_firmware(struct mtk_vcodec_fw *fw)
> +{
[ ... ]
> +	ret = mtk_vcp_mbox_ipc_register(vcp_get_ipidev(vcp_device), mailbox_id,
> +					mtk_vcodec_vcp_msg_ack_isr, fw, &fw->vcp->share_data);
[ ... ]
> +	fw->vcp->vcp_notify.notifier_call = mtk_vcodec_vcp_notifier;
> +	fw->vcp->vcp_notify.priority = 1;
> +	vcp_device->ops->vcp_register_notify(feature_id, &fw->vcp->vcp_notify);
> +
> +	if (!fw->vcp->is_register_done) {
> +		ret = vcp_device->ops->vcp_register_feature(vcp_device, feature_id);
> +		if (ret < 0) {
> +			dev_err(&fw->pdev->dev, "%d register to vcp fail(%d)\n", feature_id, ret);
> +			return -EINVAL;
> +		}

[Severity: High]
Should the error path deregister the IPC mailbox and notifier?
Returning directly leaves dangling pointers in the remote processor subsystem
pointing to the driver's memory, which could cause a use-after-free.

> +		fw->vcp->is_register_done = true;
> +	}
> +
> +	fw->vcp->is_init_done = true;
> +
> +	mutex_init(&fw->vcp->ipi_desc[ipi_id].lock);
> +	mutex_init(&fw->vcp->ipi_mutex);

[Severity: High]
Is the per-IPI mutex for VCP_IPI_CORE_DECODER initialized anywhere?
Here, ipi_id is hardcoded to VCP_IPI_LAT_DECODER (0). If the core decoder
attempts to register or process an IPI message, it will lock an uninitialized
mutex.

> +	kthread_run(mtk_vcodec_vcp_msg_process_thread, fw, "vcp_vdec_msq_thread");

[Severity: Medium]
Should the return value of kthread_run() be verified? If thread creation
fails, the driver will complete initialization successfully but permanently
hang on the first IPC communication because no thread exists to process ACKs.

[ ... ]
> +static void mtk_vcodec_vcp_release(struct mtk_vcodec_fw *fw)
> +{
> +	struct mtk_vcp_device *vcp_device = fw->vcp->vcp_device;
> +	struct device *dev = &fw->pdev->dev;
> +	int ret, feature_id;
> +
> +	if (!fw->vcp->vcp_device) {
> +		dev_err(dev, "vcp device is null\n");
> +		return;
> +	}
> +
> +	if (!fw->vcp->is_register_done)
> +		return;
> +
> +	feature_id = VDEC_FEATURE_ID;
> +	ret = vcp_device->ops->vcp_deregister_feature(vcp_device, VDEC_FEATURE_ID);
> +	if (ret < 0) {
> +		dev_err(dev, "deregister feature_id(%d) fail(%d)\n", feature_id, ret);
> +		return;
> +	}
> +
> +	fw->vcp->is_register_done = false;
> +}

[Severity: High]
Does this release function adequately stop the kthread, unregister the IPC
mailbox, and unregister the notifier? When the driver is unbound and the
devm_kzalloc managed memory is freed, the still-running kthread and remote
callbacks will access freed memory, causing a kernel panic.

> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.h b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.h
> new file mode 100644
> index 0000000000000..1072c37078f42
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.h
[ ... ]
> +struct mtk_vcp {
[ ... ]
> +	struct mtk_vcp_ipi_desc ipi_desc[VCP_IPI_MAX];
> +	bool ipi_id_ack[VCP_IPI_MAX];

[Severity: High]
Should ipi_id_ack be an integer type instead of a bool?
The code assigns negative error constants to this array upon failure.
Those non-zero values truncate to true (1), masking any IPC errors and
preventing callers from receiving accurate error codes.

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