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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: vcodec: missing IS_ERR check on fw_map_dm_addr return in VP9/AV1
- [High] media: mediatek: OOB read via unvalidated firmware sizes and VCP offsets
--

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

    media: mediatek: vcodec: get share memory address
    
    This commit divides the single shared memory for the VCP architecture
    into multiple functional areas. It introduces helpers to retrieve
    specific memory addresses and offsets for different codec components.

> 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 ece4e3e87e62d..468c4bf4e48ba 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
[ ... ]
> +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]
Does this code compute memory pointers based on hardcoded offsets without
first verifying that the configured VCP memory size (fw->vcp->vsi_size) is
actually large enough to contain them?

If the device tree memory region is insufficiently sized, could this expose
the kernel to out-of-bounds reads?

> +	default:
> +		break;
> +	}
> +
>  	return NULL;
>  }
[ ... ]
> 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
> index 756fbb7778b1f..4932ef4695946 100644
> --- 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));

[Severity: High]
Is memory being copied based on an untrusted size parameter
(vsi->cdf_table_size) provided by firmware?

Looking at vdec_av1_slice_init_cdf_table(), if a compromised firmware
supplies an excessively large size, couldn't this cause the kernel to read
out of bounds from remote_cdf_table into the dynamically sized DMA buffer?
This seems unmitigated for non-EXT capability devices.

>  	if (IS_ERR(remote_cdf_table)) {
>  		mtk_vdec_err(ctx, "failed to map cdf table\n");
>  		return PTR_ERR(remote_cdf_table);
[ ... ]
> @@ -1904,8 +1931,8 @@ static int vdec_av1_slice_init(struct mtk_vcodec_dec_ctx *ctx)
>  		goto error_vsi;
>  	}
>  	instance->init_vsi = vsi;
> -	instance->core_vsi = mtk_vcodec_fw_map_dm_addr(ctx->dev->fw_handler, (u32)vsi->core_vsi);
> -
> +	instance->core_vsi = mtk_vcodec_fw_map_dm_addr(ctx->dev->fw_handler,
> +						       vdec_av1_get_core_vsi_addr(ctx, vsi));
>  	if (!instance->core_vsi) {

[Severity: High]
Could an error pointer bypass this check when mtk_vcodec_fw_map_dm_addr()
fails?

Since mtk_vcodec_fw_map_dm_addr() returns ERR_PTR(-EINVAL) on VPU and SCP
failures, it seems an error pointer would slip past this NULL check and
lead to invalid memory accesses in subsequent memcpy operations.

>  		mtk_vdec_err(ctx, "failed to get AV1 core vsi\n");
>  		ret = -EINVAL;
[ ... ]
> 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
> index b492686784ec3..ee4d1cd82426f 100644
> --- 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) {

[Severity: High]
Is it possible for mtk_vcodec_fw_map_dm_addr() to return an ERR_PTR here?

If mapping fails, does this check miss the error pointer, allowing an
invalid memory location to be passed to kmemdup() below?

>  		mtk_vdec_err(ctx, "failed to map default frame ctx\n");
>  		return -EINVAL;
[ ... ]
> @@ -1864,7 +1882,7 @@ static int vdec_vp9_slice_init(struct mtk_vcodec_dec_ctx *ctx)
>  	}
>  	instance->init_vsi = vsi;
>  	instance->core_vsi = mtk_vcodec_fw_map_dm_addr(ctx->dev->fw_handler,
> -						       (u32)vsi->core_vsi);
> +						       vdec_vp9_get_core_vsi_addr(ctx, vsi));
>  	if (!instance->core_vsi) {

[Severity: High]
What happens if mtk_vcodec_fw_map_dm_addr() returns an error pointer?

If it only checks for NULL, it appears the error pointer could be stored
in instance->core_vsi and erroneously used as a valid memory location later.

>  		mtk_vdec_err(ctx, "failed to get VP9 core vsi\n");
>  		ret = -EINVAL;

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