Re: [PATCH v7 14/14] media: mediatek: decoder: support av1 extend vsi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: mediatek: out-of-bounds write in vdec_av1_slice_vsi_to_remote()
--

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

media: mediatek: decoder: support av1 extend vsi

This commit introduces support for an extended VSI architecture in the Mediatek
AV1 decoder. It adds tile group information directly to the VSI structure so it
can be shared with the kernel.

> 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 14639098fd463..1ef561dae18c7 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

[ ... ]

> @@ -665,6 +670,8 @@ struct vdec_av1_slice_vsi {
>  	struct vdec_av1_slice_frame frame;
>  	struct vdec_av1_slice_state state;
>  	u32 cur_lst_tile_id;
> +	struct vdec_av1_slice_tile_group tile_group;
> +	unsigned int reserved[4];
>  };

[Severity: High]
Does adding these fields inflate the size of struct vdec_av1_slice_vsi beyond
the space allocated for legacy hardware?

It looks like the compile-time size of the struct is now much larger (over
0x1D50 bytes, as the tile_group struct is around 4KB).

[ ... ]

> @@ -1942,9 +1978,18 @@ static int vdec_av1_slice_init(struct mtk_vcodec_dec_ctx *ctx)
>  		goto error_vsi;
>  	}
>  
> -	if (vsi->vsi_size != sizeof(struct vdec_av1_slice_vsi))
> -		mtk_vdec_err(ctx, "remote vsi size 0x%x mismatch! expected: 0x%zx\n",
> -			     vsi->vsi_size, sizeof(struct vdec_av1_slice_vsi));
> +	if (IS_VDEC_SUPPORT_EXT(ctx->dev->dec_capability)) {
> +		vsi_size = sizeof(struct vdec_av1_slice_vsi);
> +		vsi->iq_table_size = AV1_IQ_TABLE_SIZE;
> +		vsi->cdf_table_size = AV1_CDF_SIZE;
> +	}
> +
> +	if (vsi->vsi_size != vsi_size) {
> +		mtk_vdec_err(ctx, "remote vsi size 0x%x mismatch! expected: 0x%x\n",
> +			     vsi->vsi_size, vsi_size);
> +		ret = -EINVAL;
> +		goto error_vsi;
> +	}

[Severity: High]
For legacy (non-EXT) hardware, vsi_size is permitted to stay at the much
smaller AV1_NON_EXT_VSI_SIZE (0xD50). Could this lead to an out-of-bounds
write when synchronizing the VSI buffer?

In unmodified code, vdec_av1_slice_vsi_to_remote() performs an unconditional
copy:

static inline void vdec_av1_slice_vsi_to_remote(struct vdec_av1_slice_vsi *vsi,
						struct vdec_av1_slice_vsi *remote_vsi)
{
	memcpy(remote_vsi, vsi, sizeof(*vsi));
}

Since sizeof(*vsi) uses the new expanded struct size, but the firmware buffer
for non-EXT hardware is still validated against the smaller 0xD50 size, won't
this memcpy() overwrite adjacent VPU memory and potentially cause crashes?

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