Re: [PATCH v6 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:
- [Critical] Buffer overflow into firmware shared memory due to unconditional sizeof() in vdec_av1_slice_vsi_to_remote()

Pre-existing issues:
- [High] Use-After-Free (UAF) due to lockless access to dynamic V4L2 control array in vdec_av1_slice_setup_tile_group()
--

--- Patch [14]: [PATCH v6 14/14] media: mediatek: decoder: support av1 extend vsi ---
commit ed7b2651547150bfaed8f1355f43d152991a7bc1
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: decoder: support av1 extend vsi

This commit introduces support for AV1 extend VSI architecture in the
Mediatek decoder. It adds tile group information to the VPU shared memory
structure for extended architectures.

> 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
[ ... ]
> @@ -641,6 +644,8 @@ struct vdec_av1_slice_fb {
>   * @frame:		current frame info
>   * @state:		status after decode done
>   * @cur_lst_tile_id:	tile id for large scale
> + * @tile_group:	tile group info
> + * @reserved:		reserved
>   */
>  struct vdec_av1_slice_vsi {
>  	/* lat */
> @@ -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: Critical]
Does the addition of this large struct cause a buffer overflow on
non-extended architectures?

The firmware allocates only 3408 bytes (AV1_NON_EXT_VSI_SIZE) for this
memory mapping on non-extended architectures. However, since
vdec_av1_slice_vsi_to_remote() unconditionally copies the entire struct
using sizeof(*vsi), won't this write out-of-bounds data into the adjacent
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);
>  }
>  
> +static
> +struct vdec_av1_slice_tile_group *vdec_av1_get_tile_group(struct vdec_av1_slice_instance *instance,
> +							  struct vdec_av1_slice_vsi *vsi)
> +{
> +	if (IS_VDEC_SUPPORT_EXT(instance->ctx->dev->dec_capability))
> +		return &vsi->tile_group;
> +	else
> +		return &instance->tile_group;
> +}
> +
>  static int vdec_av1_slice_setup_tile_group(struct vdec_av1_slice_instance *instance,
>  					   struct vdec_av1_slice_vsi *vsi)
>  {
>  	struct v4l2_ctrl_av1_tile_group_entry *ctrl_tge;
> -	struct vdec_av1_slice_tile_group *tile_group = &instance->tile_group;
> +	struct vdec_av1_slice_tile_group *tile_group;
>  	struct vdec_av1_slice_uncompressed_header *uh = &vsi->frame.uh;
>  	struct vdec_av1_slice_tile *tile = &uh->tile;
>  	struct v4l2_ctrl *ctrl;
>  	u32 tge_size;
>  	int i;
>  
> +	tile_group = vdec_av1_get_tile_group(instance, vsi);
> +
>  	ctrl = v4l2_ctrl_find(&instance->ctx->ctrl_hdl, V4L2_CID_STATELESS_AV1_TILE_GROUP_ENTRY);
>  	if (!ctrl)
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this function access the dynamically
allocated V4L2 control payload locklessly?

vdec_av1_slice_setup_tile_group() fetches the control and later accesses
ctrl->p_cur.p without holding the control handler lock (ctrl_hdl->lock).
Since this executes in a workqueue, could a concurrent VIDIOC_S_EXT_CTRLS
ioctl cause the V4L2 core to reallocate the array and free the old memory?

This would result in a use-after-free read of the old array payload, potentially
tricking the hardware into performing out-of-bounds DMA.

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