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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `mtk_vcodec_vcp_msg` ops structure lacks the `get_venc_capa` callback, causing a NULL pointer dereference when opening a VCP encoder.
- [High] The VCP memory address mapping breaks encoder initialization because encoder drivers were not updated to pass the new enum values.

Pre-existing issues:
- [High] Missing bounds validation for firmware-provided AV1 table sizes causes potential out-of-bounds reads.
- [High] `if (!ptr)` error checks fail to catch `ERR_PTR`, leading to kernel panic on mapping failure.
--

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

media: mediatek: vcodec: get share memory address

There is only one share memory for vcp architecture, need to
divide it into many 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
[ ... ]
> @@ -434,8 +434,26 @@ 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 void *mtk_vcodec_vcp_dm_addr(struct mtk_vcodec_fw *fw, u32 mem_type)
>  {

[Severity: High]
Does the VCP firmware operations structure mtk_vcodec_vcp_msg support encoders?

When opening an encoder device, fops_vcodec_open() unconditionally calls
mtk_vcodec_fw_get_venc_capa(), which invokes fw->ops->get_venc_capa(fw).
If mtk_vcodec_vcp_msg does not assign the .get_venc_capa callback, won't
opening a VCP encoder trigger a NULL pointer dereference?

> +	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;
> +	default:
> +		break;
> +	}
> +
>  	return NULL;
>  }

[Severity: High]
Are we sure all encoder calls have been updated to pass the new memory type
enums?

It looks like the encoder subsystem still passes physical device addresses
instead of the new enum mtk_vcp_mem_type. For example, handle_enc_init_msg()
in venc_vpu_if.c passes msg->vpu_inst_addr, and h264_enc_alloc_work_buf() in
venc_h264_if.c passes vpua.

Since this switch statement will fall through to default for those physical
addresses, won't mtk_vcodec_fw_map_dm_addr() now return NULL and trigger a
crash during encoder initialization?

> 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));

[Severity: High]
This is a pre-existing issue, but is it safe to use vsi->cdf_table_size without
validation?

In vdec_av1_slice_init_cdf_table(), the size vsi->cdf_table_size is provided
by the untrusted firmware and later used in a memory copy:

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

Could a compromised firmware provide an arbitrarily large size, leading to an
out-of-bounds read from the DMA region and a corresponding out-of-bounds write
into kernel memory?

>  	if (IS_ERR(remote_cdf_table)) {
>  		mtk_vdec_err(ctx, "failed to map cdf table\n");
>  		return PTR_ERR(remote_cdf_table);

> 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]
This is a pre-existing issue, but does this error check handle ERR_PTR returns
correctly?

When using SCP firmware, scp_mapping_dm_addr() returns ERR_PTR(-EINVAL) on
mapping failure. Since an error pointer evaluates as true, the
if (!remote_frame_ctx) check is bypassed.

Could this allow the invalid ERR_PTR to be passed to kmemdup() further down in
the function, causing a kernel panic?

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