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

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] media: mediatek: vcodec: CPU lockup due to unhandled signal in wait_event_interruptible
- [High] media: mediatek: vcodec: modprobe fork bomb DoS in vcp device polling
- [High] media: mediatek: vcodec: Use-After-Free and state desynchronization on IPI timeout
- [High] media: mediatek: vcodec: rproc reference leak on vcp deregister failure
- [Medium] media: mediatek: vcodec: build failure due to missing MTK_VCP_RPROC module dependency
- [Medium] media: mediatek: vcodec: compilation error due to undeclared VCP_SYNC_TIMEOUT_MS
--

commit d524a6ee370f9efe82015b1c8256b28c7fb3bc55
Author: Kyrie Wu [off-list ref]

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.

[ ... ]

[Severity: Medium]
Will this cause a link failure if VIDEO_MEDIATEK_VCODEC is built-in (=y) and
MTK_VCP_RPROC is a module (=m)? Because this select statement doesn't
enforce a modular state dependency, mtk_vcodec_fw_vcp.c could be built-in
while attempting to call exported symbols from the MTK_VCP_RPROC module.

[ ... ]

[ ... ]

+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 ||
+					       kthread_should_stop());
+		if (ret < 0) {
+			dev_err(&fw->pdev->dev, "wait msg queue ack timeout %d %d\n",
+				ret, atomic_read(&fw->vcp->msg_queue.cnt));
+			continue;

[Severity: High]
Could this lead to an infinite busy loop if a signal is delivered to the
kthread? When wait_event_interruptible() returns -ERESTARTSYS due to a
pending signal, the continue statement immediately restarts the loop. Since
the signal is never cleared, it seems this will continuously spin and lock up
the CPU.

+		}

[ ... ]

+		handler(msg, obj->len, fw->vcp->ipi_desc[obj->id].priv);
+		mtk_vcodec_vcp_ipi_unlock(fw->vcp, obj->id);
+
+		fw->vcp->msg_signaled[obj->id] = true;
+		wake_up(&fw->vcp->msg_wq[obj->id]);

[Severity: High]
Is it possible that this accesses freed memory if the original request timed
out? In mtk_vcodec_vcp_msg_ipi_send(), if the wait for msg_wq times out,
it returns an error to the caller, which might free the associated context.
When the delayed ACK finally arrives, this handler could dereference the
freed context or wake up a completely new request reusing the same ID.

+
+		mtk_vcodec_vcp_free_msg_node(fw, msg_node);
+	} while (!kthread_should_stop());
+
+	return ret;
+}

[ ... ]

+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;
+
+	timeout_jiffies = jiffies + msecs_to_jiffies(VCP_SYNC_TIMEOUT_MS);

[Severity: Medium]
Will this cause a compilation error? VCP_SYNC_TIMEOUT_MS doesn't seem to be
defined anywhere in this patch or the kernel tree.

+	while (!vcp_device->ops->vcp_is_ready(vcp_device, feature_id)) {

[ ... ]

+wait_ack:
+	/* wait for VCP's ACK */
+	ret = wait_event_interruptible_timeout(*msg_wq, *msg_signaled,
+					       msecs_to_jiffies(IPI_TIMEOUT_MS));
+	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]
As mentioned above regarding mtk_vcodec_vcp_msg_process_thread(), when this
timeout is reached, the hardware transaction appears to remain pending. Does
this leave the firmware state desynchronized and risk memory corruption when
the delayed ACK eventually arrives?

+	} else if (ret == -ERESTARTSYS) {

[ ... ]

+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 loop result in a modprobe fork bomb? try_then_request_module()
invokes the userspace modprobe helper. If the module is genuinely missing or
fails to load, this loops 10,000 times, potentially spawning 10,000 modprobe
processes over 15 seconds, exhausting system resources.

+
+	if (of_property_read_u32(dev->of_node, "mediatek,vcp", &vcp_phandle)) {

[ ... ]

+static void mtk_vcodec_vcp_release(struct mtk_vcodec_fw *fw)
+{

[ ... ]

+	if (!fw->vcp->is_register_done) {
+		fw->vcp->is_init_done = false;
+		goto put_device;
+	}
+
+	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;

[Severity: High]
Does this early return cause a leak of the rproc reference count? Returning
here bypasses the put_device label and skips mtk_vcodec_vcp_put_device(),
which would prevent the module from ever being unloaded if deregistration
fails.

+	}
+
+	fw->vcp->is_register_done = false;
+	fw->vcp->is_init_done = false;
+
+put_device:
+	mtk_vcodec_vcp_put_device(fw);
+}

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