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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] media: mtk_vcodec: encoder initialization fails due to unmapped IPI ID for VCP
- [High] media: mtk_vcodec: delayed ACK causes protocol desynchronization in VCP IPC
- [High] media: vcodec: vcp encoder fails to initialize due to out-of-bounds ipi_id
- [High] media: mediatek: vcodec: UAF in IPI message handlers on device close due to IPC timeout
--

commit c1db7dc6cad08382fb97ce9b64a0f609116bfae6
Author: Kyrie Wu <kyrie.wu@mediatek.com>

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

This commit adds encoder support for the VCP interface to allow communication
between the encoder and VCP hardware.

> 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
> index 150d842c7d226..ece4e3e87e62d 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c

[ ... ]

> @@ -223,8 +225,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;
> +	}
>  
>  	timeout_jiffies = jiffies + msecs_to_jiffies(VCP_SYNC_TIMEOUT_MS);

[Severity: High]
Will this initialization succeed for encoders?

The encoder driver appears to initialize its ID to SCP_IPI_VENC_H264 (4)
or IPI_VENC_H264 (3), as seen in h264_enc_init():

    if (is_ext)
        inst->vpu_inst.id = SCP_IPI_VENC_H264;
    else
        inst->vpu_inst.id = IPI_VENC_H264;

However, earlier in mtk_vcodec_vcp_msg_ipi_send() and in 
mtk_vcodec_vcp_ipi_register(), the code checks mtk_vcodec_vcp_ipi_id_valid(),
which strictly requires the ID to be less than VCP_IPI_MAX.

Since VCP_IPI_MAX is 3, these encoder IDs will always fail the validation
check, returning -EINVAL and preventing the VCP encoder from communicating.


[Severity: High]
Could a delayed ACK from the firmware cause memory corruption or state
desynchronization?

When waiting for an ACK later in mtk_vcodec_vcp_msg_ipi_send(), the function
uses a hardcoded 100ms timeout and relies on a shared boolean:

wait_ack:
    ret = wait_event_interruptible_timeout(*msg_wq, *msg_signaled,
                                           msecs_to_jiffies(IPI_TIMEOUT_MS));

If the firmware takes longer than 100ms to reply, the wait times out
and returns an error. However, the firmware might still process the message
and send the ACK later.

This appears to lead to two potential issues:

1. If the timeout occurs during device teardown, such as when sending a
DEINIT message before closing the device, the driver proceeds to free
the instance via kfree(inst). When the delayed ACK eventually arrives,
the asynchronous message thread calls the IPI handler:

vpu_enc_ipi_handler()
    vpu = (struct venc_vpu_inst *)(unsigned long)msg->venc_inst;
    mtk_venc_debug(vpu->ctx, ...);

The handler extracts the freed instance from the message and dereferences
vpu->ctx, resulting in a use-after-free.

2. Because there are no sequence numbers matching ACKs to requests, a
delayed ACK could arrive during a subsequent message on the same ID.
The new message resets the shared flag and waits, but the delayed ACK
from the previous message would immediately wake it up prematurely,
desynchronizing the state between the driver and the firmware.

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