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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Out-of-bounds write to VPU shared memory in `vdec_av1_slice_vsi_to_remote()` due to unconditional struct memcpy.

Pre-existing issues:
- [High] Missing bounds check on `tile_cols` and `tile_rows` leads to heap buffer overflow during AV1 tile setup.
--

commit 704ba9a36ee79563029e1b285c390dcbc873258d
Author: Yunfei Dong <yunfei.dong@mediatek.com>

media: mediatek: decoder: support av1 extend vsi
    
The driver can't access tile buffer address for extend architecture,
set tile group information in vcp and share it with 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 a0c7e89b8ae4d..e9265b112bfbe 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 this struct size increase cause an out-of-bounds write in
vdec_av1_slice_vsi_to_remote()?

Since vdec_av1_slice_vsi_to_remote() unconditionally uses sizeof(*vsi):

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

and this patch adds the large tile_group structure to it, won't this copy the
new larger size?

For non-EXT devices, the remote firmware buffer remains AV1_NON_EXT_VSI_SIZE
(0xD50), so won't this write past the end of the VPU shared memory?

[ ... ]
> @@ -1402,17 +1409,29 @@ static void vdec_av1_slice_setup_uh(struct vdec_av1_slice_instance *instance,
>  	vdec_av1_slice_setup_tile(frame, &ctrl_fh->tile_info);
>  }

[Severity: High]
This is a pre-existing issue, but does vdec_av1_slice_setup_tile()
lack bounds checking for tile->tile_cols and tile->tile_rows?

vdec_av1_slice_setup_tile() reads ctrl_tile->tile_cols and
ctrl_tile->tile_rows from userspace control without validation:

static void vdec_av1_slice_setup_tile(struct vdec_av1_slice_frame *frame,
				      struct v4l2_av1_tile_info *ctrl_tile)
{
	...
	tile->tile_cols = ctrl_tile->tile_cols;
	tile->tile_rows = ctrl_tile->tile_rows;
	...
	for (i = 0; i < tile->tile_cols + 1; i++)
		tile->mi_col_starts[i] =
			ALIGN(ctrl_tile->mi_col_starts[i], BIT(mib_size_log2)) >> mib_size_log2;

	for (i = 0; i < tile->tile_rows + 1; i++)
		tile->mi_row_starts[i] =
			ALIGN(ctrl_tile->mi_row_starts[i], BIT(mib_size_log2)) >> mib_size_log2;
}

Since mi_col_starts and mi_row_starts are fixed-size arrays (65 elements) in
struct vdec_av1_slice_tile, could a malicious application supply values
greater than 64 and cause a heap buffer overflow when copying from
ctrl_tile?

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