Re: [PATCH v6 05/14] media: mediatek: vcodec: get share memory address

[sashiko-bot@kernel.org]

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

New issues:
- [High] mtk_vcodec_fw_map_dm_addr() has inconsistent error semantics across firmware types, leading to NULL or ERR_PTR dereferences.
- [High] mtk_vcodec_vcp_dm_addr() calculates shared memory offsets blindly without verifying they fit within the allocated firmware memory (vsi_size).

Pre-existing issues:
- [High] Firmware-provided sizes vsi->cdf_table_size and vsi->iq_table_size are used for memory allocation and memcpy without validation.
--

--- Patch [5]: [PATCH v6 05/14] media: mediatek: vcodec: get share memory address ---
commit 4c9f9286cbe2e7417003bc83c03e8482665fcca4
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: vcodec: get share memory address

This commit adds support to get shared memory addresses for the VCP
architecture by dividing a single shared memory block into different
functions.

> 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
[ ... ]
> @@ -483,8 +483,31 @@ static unsigned int mtk_vcodec_vcp_get_vdec_capa(struct mtk_vcodec_fw *fw)
>  	       MTK_VDEC_IS_SUPPORT_10BIT | MTK_VDEC_IS_SUPPORT_EXT;
>  }
>  
> -static void *mtk_vcodec_vcp_dm_addr(struct mtk_vcodec_fw *fw, u32 dtcm_dmem_addr)
> +static unsigned int mtk_vcodec_vcp_get_venc_capa(struct mtk_vcodec_fw *fw)
>  {
> +	return 0;
> +}
> +
> +static void *mtk_vcodec_vcp_dm_addr(struct mtk_vcodec_fw *fw, u32 mem_type)
> +{
> +	unsigned char *vsi_core = fw->vcp->vsi_core_addr;
> +
> +	switch (mem_type) {
> +	case ENCODER_MEM:
> +	case VCODEC_LAT_MEM:
> +		return fw->vcp->vsi_addr;
> +	case VCODEC_CORE_MEM:
> +		return vsi_core;
> +	case VP9_FRAME_MEM:
> +		return vsi_core + VCODEC_VSI_LEN;
> +	case AV1_CDF_MEM:
> +		return vsi_core + VCODEC_VSI_LEN + VP9_FRAME_SIZE;
> +	case AV1_IQ_MEM:
> +		return vsi_core + VCODEC_VSI_LEN + VP9_FRAME_SIZE + AV1_CDF_SIZE;

[Severity: High]
Are these offsets guaranteed to fit within the memory provided by the
hardware?

When fw->vcp->vsi_size is initialized via get_mem_size() in
mtk_vcodec_vcp_load_firmware(), the driver doesn't seem to validate that
the allocated block is large enough to contain these offsets. If a
malformed device tree or mismatched firmware provides a smaller memory
block, could this lead to out-of-bounds accesses into adjacent memory?

> +	default:
> +		break;
> +	}
> +
> +	return NULL;
> +}

[Severity: High]
Is returning NULL here safe when other firmware types return an ERR_PTR?

In drivers like vpu and scp, mapping functions return ERR_PTR(-EINVAL)
on failure. Because this VCP function returns NULL instead, callers have
mixed expectations.

For example, in vdec_av1_slice_init_cdf_table(), the code checks:

        remote_cdf_table = mtk_vcodec_fw_map_dm_addr(...);
        if (IS_ERR(remote_cdf_table)) {
                ...
        }

If VCP mapping fails and returns NULL, it passes the IS_ERR() check and
can cause a NULL pointer dereference during the subsequent memcpy().

> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
[ ... ]
> @@ -775,7 +784,7 @@ static int vdec_av1_slice_init_cdf_table(struct vdec_av1_slice_instance *instanc
>  	ctx = instance->ctx;
>  	vsi = instance->vpu.vsi;
>  	remote_cdf_table = mtk_vcodec_fw_map_dm_addr(ctx->dev->fw_handler,
> -						     (u32)vsi->cdf_table_addr);
> +						     vdec_av1_get_cdf_table_addr(ctx, vsi));
>  	if (IS_ERR(remote_cdf_table)) {
>  		mtk_vdec_err(ctx, "failed to map cdf table\n");
>  		return PTR_ERR(remote_cdf_table);

[Severity: High]
This is a pre-existing issue, but does vsi->cdf_table_size need to be
validated against the maximum allowed size before it is used?

Shortly after this mapping, the code does:

        memcpy(instance->cdf_table.va, remote_cdf_table, vsi->cdf_table_size);

Since vsi->cdf_table_size is provided by the firmware via shared memory, a
compromised or malformed firmware could provide a size larger than the
AV1_CDF_SIZE (0xFE80) memory region. Could this result in an out-of-bounds
read of adjacent kernel memory?

> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
[ ... ]
> @@ -514,7 +523,7 @@ static int vdec_vp9_slice_init_default_frame_ctx(struct vdec_vp9_slice_instance
>  		return -EINVAL;
>  
>  	remote_frame_ctx = mtk_vcodec_fw_map_dm_addr(ctx->dev->fw_handler,
> -						     (u32)vsi->default_frame_ctx);
> +						     vdec_vp9_get_frame_ctx_addr(ctx, vsi));
>  	if (!remote_frame_ctx) {
>  		mtk_vdec_err(ctx, "failed to map default frame ctx\n");
>  		return -EINVAL;

[Severity: High]
Does this !remote_frame_ctx check properly handle the ERR_PTR(-EINVAL) that
can be returned by mtk_vcodec_fw_map_dm_addr() when using VPU or SCP?

Since the VPU and SCP mapping paths return an ERR_PTR on failure, this check
will bypass them, resulting in kmemdup() attempting to dereference the
error pointer:

vdec_vp9_slice_init_default_frame_ctx() {
    ...
    if (!remote_frame_ctx) {
        return -EINVAL;
    }
    ...
    frame_ctx = kmemdup(remote_frame_ctx, sizeof(*frame_ctx), GFP_KERNEL);
}

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