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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Untrusted coprocessor input `obj->id` is used directly as an array index without bounds checking, causing out-of-bounds memory access and potential arbitrary code execution.
- [High] The kthread_run() task pointer is discarded, causing a memory leak and a fatal use-after-free when the device is unbound.
- [High] The msg_signaled flag is not reset before waiting, allowing a stale, delayed ACK to trigger a false positive for a subsequent IPC message.
- [High] The driver registers an IPC mailbox and a notifier but fails to unregister them during device teardown, leading to a Use-After-Free.
- [High] The message processing thread exits permanently upon receiving an unhandled message ID, hanging the entire driver.
- [High] The driver fails to initialize the `ipi_desc` mutex for the CORE decoder, leading to lockdep warnings and potential crashes.
--

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

media: mediatek: vcodec: add driver to support vcp encoder

Encoder also need to call vcp interface to communicate with vcp,
add driver to support encoder.

> 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
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> @@ -226,8 +228,13 @@ static int mtk_vcodec_vcp_msg_ipi_send(struct mtk_vcodec_fw *fw, int id, void *b
>  	}
>  
>  	mutex_lock(&vcp->ipi_mutex);
> -	feature_id = VDEC_FEATURE_ID;
> -	mailbox_id = IPI_OUT_VDEC_1;
> +	if (fw->fw_use == ENCODER) {
> +		feature_id = VENC_FEATURE_ID;
> +		mailbox_id = IPI_OUT_VENC_0;
> +	} else {
> +		feature_id = VDEC_FEATURE_ID;
> +		mailbox_id = IPI_OUT_VDEC_1;
> +	}

[Severity: High]
This is a pre-existing issue, but does mtk_vcodec_vcp_msg_ipi_send()
properly handle stale delayed ACKs?

If wait_event_timeout() times out later in this function, it appears it sets
the msg_signaled flag to false and returns. However, if a delayed ACK arrives
afterward, the worker thread will set the flag to true.

Since mtk_vcodec_vcp_msg_ipi_send() does not re-initialize the msg_signaled
flag to false before sending the next IPC message, could a subsequent wait
immediately return success without actually waiting for the new message to
be acknowledged?

[ ... ]

> @@ -383,6 +397,20 @@ static int mtk_vcodec_vcp_load_firmware(struct mtk_vcodec_fw *fw)
>  	mutex_init(&fw->vcp->ipi_desc[ipi_id].lock);
>  	mutex_init(&fw->vcp->ipi_mutex);

[Severity: High]
This isn't a bug introduced by this patch, but are all the ipi_desc mutexes
properly initialized?

Here, mutex_init() is only called for a single ipi_id. If this path is taken
for the decoder, it initializes the lock for VCP_IPI_LAT_DECODER, but it omits
VCP_IPI_CORE_DECODER.

When the decoder client later registers its CORE handler,
mtk_vcodec_vcp_ipi_register() will attempt to acquire the lock. Could locking
the uninitialized CORE mutex lead to lockdep splats or undefined behavior?

>  
> +	if (fw->fw_use == ENCODER) {
> +		kthread_run(mtk_vcodec_vcp_msg_process_thread, fw, "vcp_enc_msq_thread");
> +

[Severity: High]
This is a pre-existing issue, but is it safe to discard the task pointer
returned by kthread_run()?

If the pointer is not saved, kthread_stop() cannot be called in
mtk_vcodec_vcp_release() during device teardown. Because the fw structure
is allocated with devm_kzalloc, the memory will be automatically freed when
the driver unbinds.

Could this cause a use-after-free when the unstopped thread wakes up
and accesses the freed fw->vcp structures?

[Severity: Critical]
This is a pre-existing issue, but does mtk_vcodec_vcp_msg_process_thread()
validate the obj->id from the coprocessor?

Looking at the thread function, it appears the ID from the IPC payload is
used directly as an array index:

handler = fw->vcp->ipi_desc[obj->id].handler;

If a compromised or malicious coprocessor sends an out-of-bounds ID, could
this read out-of-bounds function pointers and execute them when calling
handler(), leading to arbitrary kernel code execution?

[Severity: High]
This isn't a bug introduced by this patch, but how does
mtk_vcodec_vcp_msg_process_thread() handle invalid message IDs?

If the handler is null for a message, the function executes return -EINVAL;
instead of using continue; to process the next message.

Does this permanently terminate the kthread loop? If the thread exits,
wouldn't all subsequent valid IPC messages be queued but never processed,
permanently hanging active decode or encode instances?

[ ... ]

> @@ -438,8 +466,8 @@ static void mtk_vcodec_vcp_release(struct mtk_vcodec_fw *fw)
>  	if (!fw->vcp->is_register_done)
>  		return;
>  
> -	feature_id = VDEC_FEATURE_ID;
> -	ret = vcp_device->ops->vcp_deregister_feature(vcp_device, VDEC_FEATURE_ID);
> +	feature_id = fw->fw_use == ENCODER ? VENC_FEATURE_ID : VDEC_FEATURE_ID;
> +	ret = vcp_device->ops->vcp_deregister_feature(vcp_device, feature_id);
>  	if (ret < 0) {
>  		dev_err(dev, "deregister feature_id(%d) fail(%d)\n", feature_id, ret);
>  		return;

[Severity: High]
This is a pre-existing issue, but does this release function completely
tear down the IPC mailbox and notifier?

The driver registers an IPC mailbox via mtk_vcp_mbox_ipc_register() and a
notifier via vcp_register_notify() in mtk_vcodec_vcp_load_firmware().
However, these do not appear to be unregistered here during teardown.

Because fw->vcp is managed by devm_kzalloc, could this leave dangling
callbacks in the IPC or notifier frameworks that might trigger a
use-after-free if an event arrives after the device is unbound?

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