Re: [PATCH v8 04/14] media: mediatek: vcodec: get different firmware ipi id

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: mtk-vcodec: OOB write via unvalidated -EINVAL from fw_get_ipi
--

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

media: mediatek: vcodec: get different firmware ipi id

This commit updates the driver to get the inter-processor interrupt (IPI) ID
dynamically based on the firmware type and hardware index. This allows
supporting different architectures instead of using hardcoded IDs.

> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.c
> index 552d0d8a84444..8630e871d78e2 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.c
> @@ -3,6 +3,20 @@
>  #include "../decoder/mtk_vcodec_dec_drv.h"
>  #include "../encoder/mtk_vcodec_enc_drv.h"
>  #include "mtk_vcodec_fw_priv.h"
> +#include "mtk_vcodec_fw_vcp.h"
> +
> +int mtk_vcodec_fw_get_ipi(enum mtk_vcodec_fw_type type, int hw_id)
> +{
> +	switch (type) {
> +	case SCP:
> +		return hw_id == MTK_VDEC_LAT0 ? SCP_IPI_VDEC_LAT : SCP_IPI_VDEC_CORE;
> +	case VCP:
> +		return hw_id == MTK_VDEC_LAT0 ? VCP_IPI_LAT_DECODER : VCP_IPI_CORE_DECODER;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(mtk_vcodec_fw_get_ipi);

[ ... ]

> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> index 10359ce9b934e..69d60717181a6 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> @@ -1204,6 +1204,7 @@ static int vdec_h264_slice_single_decode(void *h_vdec, struct mtk_vcodec_mem *bs
>  
>  static int vdec_h264_slice_init(struct mtk_vcodec_dec_ctx *ctx)
>  {
> +	enum mtk_vcodec_fw_type fw_type = ctx->dev->fw_handler->type;
>  	struct vdec_h264_slice_inst *inst;
>  	int err, vsi_size;
>  	unsigned char *temp;
> @@ -1214,8 +1215,8 @@ static int vdec_h264_slice_init(struct mtk_vcodec_dec_ctx *ctx)
>  
>  	inst->ctx = ctx;
>  
> -	inst->vpu.id = SCP_IPI_VDEC_LAT;
> -	inst->vpu.core_id = SCP_IPI_VDEC_CORE;
> +	inst->vpu.id = mtk_vcodec_fw_get_ipi(fw_type, MTK_VDEC_LAT0);
> +	inst->vpu.core_id = mtk_vcodec_fw_get_ipi(fw_type, MTK_VDEC_CORE);

[Severity: High]
Does this code handle cases where mtk_vcodec_fw_get_ipi() returns an error
code?

If an unsupported firmware type (such as VPU) is provided,
mtk_vcodec_fw_get_ipi() will return -EINVAL (-22), which is then assigned
directly to inst->vpu.id and inst->vpu.core_id without validation.

When this propagates to vpu_ipi_register() via vpu_dec_init():

drivers/media/platform/mediatek/vpu/mtk_vpu.c:vpu_ipi_register() {
    ...
    if (id < IPI_VPU_MAX && handler) {
        ipi_desc = vpu->ipi_desc;
        ipi_desc[id].name = name;
    ...
}

On ARM64 (AArch64), the AAPCS64 dictates that the underlying type for an enum
whose values all fit in a signed 32-bit integer is a signed int, making
enum ipi_id signed.

The bounds check evaluates -22 < IPI_VPU_MAX as true. This bypasses the check
and executes a negative index out-of-bounds write to the ipi_desc array.

Could this lead to memory corruption (slab underflow) before the mtk_vpu
struct if a malformed Device Tree pairs a stateless decoder compatible string
with a VPU firmware handler?

>  	inst->vpu.ctx = ctx;
>  	inst->vpu.codec_type = ctx->current_codec;
>  	inst->vpu.capture_type = ctx->capture_fourcc;

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