Re: [PATCH v5 11/14] dt-bindings: media: mediatek: vcodec: add decoder dt-bindings for mt8196

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The VCP message queue processing thread uses the untrusted `obj->id` directly as an array index without bounds checking, leading to out-of-bounds memory access and potential arbitrary code execution.
- [High] The VCP message queue kthreads spawned during firmware load are leaked, leading to a fatal Use-After-Free kernel panic on driver unbind.
- [High] The `ipi_id_ack` array stores negative error codes into `bool` elements, truncating them to `1`, and is never reset on success, which breaks error handling and permanently poisons the IPC state.
- [Medium] The patch adds the MT8196 compatible string but completely omits the corresponding hardware constraints mentioned in the commit message, breaking device tree validation for this SoC.
--

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

dt-bindings: media: mediatek: vcodec: add decoder dt-bindings for mt8196

This commit adds the device tree binding compatible string for the MT8196
decoder. The commit message outlines hardware changes, including 12 clock
inputs, 2 power domains, and the use of VCP instead of SCP.

> The MT8196 decoder differs from previous generations in several
> key aspects, most notably in its use of VCP instead of SCP.
> Additionally, the MT8196 enhances codec capabilities by supporting
> HEVC Main10 profile decoding. To accommodate these hardware changes,
> the binding constraints specify a total of 12 clock inputs,
> consisting of 9 decoder clocks and 3 VCP interface clocks,
> along with 2 power domains covering both the decoder and VCP
> subsystems.

[Severity: Medium]
The commit message mentions that the binding constraints specify a total of 12
clock inputs, 2 power domains, and the use of VCP instead of SCP. However,
are these constraints actually added in this patch?

> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev-decoder.yaml
> index bf8082d87ac03..74e1d88d30568 100644
> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev-decoder.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev-decoder.yaml
> @@ -76,6 +76,7 @@ properties:
>        - mediatek,mt8186-vcodec-dec
>        - mediatek,mt8188-vcodec-dec
>        - mediatek,mt8195-vcodec-dec
> +      - mediatek,mt8196-vcodec-dec

Looking at the diff, it appears only the compatible string is added. The
required conditional blocks and property constraints for the clocks, power
domains, and the mediatek,vcp property seem to be missing. Would this cause
valid MT8196 device tree files to falsely fail schema validation?

I also have some questions regarding the VCP message queue handling in the
related C driver files.

In drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c:

[Severity: High]
Does this code leak the vcp_enc_msq_thread kthread?

mtk_vcodec_fw_vcp.c:mtk_vcodec_vcp_load_firmware() {
    ...
    kthread_run(mtk_vcodec_vcp_msg_process_thread, fw, "vcp_enc_msq_thread");
    ...
}

It seems the task_struct pointer returned by kthread_run() is discarded, so
kthread_stop() cannot be called later in mtk_vcodec_vcp_release().

If the thread is leaked, could this lead to a use-after-free crash on driver
unbind? The leaked thread appears to block indefinitely on the waitqueue inside
the devres-managed fw->vcp struct:

mtk_vcodec_fw_vcp.c:mtk_vcodec_vcp_msg_process_thread() {
    ...
    ret = wait_event_interruptible(fw->vcp->msg_queue.wq,
                                   atomic_read(&fw->vcp->msg_queue.cnt) > 0);
    ...
}


[Severity: Critical]
Can an invalid firmware payload trigger an out-of-bounds memory access here?

mtk_vcodec_fw_vcp.c:mtk_vcodec_vcp_msg_process_thread() {
    ...
    mtk_vcodec_vcp_ipi_lock(fw->vcp, obj->id);
    handler = fw->vcp->ipi_desc[obj->id].handler;
    ...
    handler(msg, obj->len, fw->vcp->ipi_desc[obj->id].priv);
    ...
}

The obj->id value comes from the firmware IPC message payload. If this value
exceeds the VCP_IPI_MAX bound, it looks like it could directly index outside
the ipi_desc array, fetching an unvalidated function pointer and immediately
executing it. Should we add a bounds check for obj->id?


[Severity: High]
Are standard negative error codes truncated in this structure?

drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.h:struct mtk_vcp {
    ...
    bool ipi_id_ack[VCP_IPI_MAX];
    ...
}

Because ipi_id_ack is declared as a boolean array, assigning negative error
codes to it will truncate them to 1 (true).

Additionally, it seems the flag is never reset to false on success:

mtk_vcodec_fw_vcp.c:mtk_vcodec_vcp_msg_ipi_send() {
    ...
    return vcp->ipi_id_ack[id];
}

If a single error or timeout sets this flag to true, won't all subsequent
successful IPC sends falsely report failure because they read the stale true
state?

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