Re: [PATCH v2] media: cedrus: reject invalid active H.264 ref indices

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 6547 bytes --]

Le jeudi 09 avril 2026 à 21:30 +0800, Pengpeng Hou a écrit :
> Cedrus consumes the active ref_pic_list0/ref_pic_list1 entries and uses
> their indices to look up decode->dpb[] in _cedrus_write_ref_list().
> 
> Those active portions are the first
> num_ref_idx_l0_active_minus1 + 1 / num_ref_idx_l1_active_minus1 + 1
> entries in the two reference lists.
> 
> An out-of-range index in that active portion can therefore read past the
> fixed V4L2_H264_NUM_DPB_ENTRIES array.
> 
> Checking this in cedrus_try_ctrl() is awkward because the request-local
> DPB state may not have been applied yet. Instead, validate the active
> entries at the actual Cedrus use site and fail setup with -EINVAL if one
> points past decode->dpb[].
> 
> Entries beyond the active reference counts remain ignored as before, so
> this does not change how Cedrus treats unused tail data in the
> reference-list controls.
> 
> Fixes: 6eb9b758e307 ("media: cedrus: Add H264 decoding support")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
> Changes since v1:
> - reject invalid indices in the active reference list entries instead of
>   silently skipping them
> - keep the validation at the Cedrus use site, but propagate -EINVAL back
>   through setup
> 
>  drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 32 ++++++++++++++--
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index 3e2843ef6cce..58c411c580f3 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -186,10 +186,10 @@ static int cedrus_write_frame_list(struct cedrus_ctx *ctx,
>  
>  #define CEDRUS_MAX_REF_IDX	32
>  
> -static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
> -				   struct cedrus_run *run,
> -				   const struct v4l2_h264_reference *ref_list,
> -				   u8 num_ref, enum cedrus_h264_sram_off sram)
> +static int _cedrus_write_ref_list(struct cedrus_ctx *ctx,
> +				  struct cedrus_run *run,
> +				  const struct v4l2_h264_reference *ref_list,
> +				  u8 num_ref, enum cedrus_h264_sram_off sram)
>  {
>  	const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
>  	struct vb2_queue *cap_q;
> @@ -210,6 +210,9 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  		u8 dpb_idx;
>  
>  		dpb_idx = ref_list[i].index;
> +		if (dpb_idx >= V4L2_H264_NUM_DPB_ENTRIES)
> +			return -EINVAL;
> +
>  		dpb = &decode->dpb[dpb_idx];
>  
>  		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> @@ -229,28 +232,30 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  
>  	size = min_t(size_t, ALIGN(num_ref, 4), sizeof(sram_array));
>  	cedrus_h264_write_sram(dev, sram, &sram_array, size);
> +
> +	return 0;
>  }
>  
> -static void cedrus_write_ref_list0(struct cedrus_ctx *ctx,
> -				   struct cedrus_run *run)
> +static int cedrus_write_ref_list0(struct cedrus_ctx *ctx,
> +				  struct cedrus_run *run)
>  {
>  	const struct v4l2_ctrl_h264_slice_params *slice = run->h264.slice_params;
>  
> -	_cedrus_write_ref_list(ctx, run,
> -			       slice->ref_pic_list0,
> -			       slice->num_ref_idx_l0_active_minus1 + 1,
> -			       CEDRUS_SRAM_H264_REF_LIST_0);
> +	return _cedrus_write_ref_list(ctx, run,
> +				      slice->ref_pic_list0,
> +				      slice->num_ref_idx_l0_active_minus1 + 1,
> +				      CEDRUS_SRAM_H264_REF_LIST_0);
>  }
>  
> -static void cedrus_write_ref_list1(struct cedrus_ctx *ctx,
> -				   struct cedrus_run *run)
> +static int cedrus_write_ref_list1(struct cedrus_ctx *ctx,
> +				  struct cedrus_run *run)
>  {
>  	const struct v4l2_ctrl_h264_slice_params *slice = run->h264.slice_params;
>  
> -	_cedrus_write_ref_list(ctx, run,
> -			       slice->ref_pic_list1,
> -			       slice->num_ref_idx_l1_active_minus1 + 1,
> -			       CEDRUS_SRAM_H264_REF_LIST_1);
> +	return _cedrus_write_ref_list(ctx, run,
> +				      slice->ref_pic_list1,
> +				      slice->num_ref_idx_l1_active_minus1 + 1,
> +				      CEDRUS_SRAM_H264_REF_LIST_1);
>  }
>  
>  static void cedrus_write_scaling_lists(struct cedrus_ctx *ctx,
> @@ -338,8 +343,8 @@ static void cedrus_skip_bits(struct cedrus_dev *dev, int num)
>  	}
>  }
>  
> -static void cedrus_set_params(struct cedrus_ctx *ctx,
> -			      struct cedrus_run *run)
> +static int cedrus_set_params(struct cedrus_ctx *ctx,
> +			     struct cedrus_run *run)
>  {
>  	const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
>  	const struct v4l2_ctrl_h264_slice_params *slice = run->h264.slice_params;
> @@ -351,6 +356,7 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
>  	size_t slice_bytes = vb2_get_plane_payload(src_buf, 0);
>  	unsigned int pic_width_in_mbs;
>  	bool mbaff_pic;
> +	int ret;
>  	u32 reg;
>  
>  	cedrus_write(dev, VE_H264_VLD_LEN, slice_bytes * 8);
> @@ -393,11 +399,17 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
>  
>  	if ((slice->slice_type == V4L2_H264_SLICE_TYPE_P) ||
>  	    (slice->slice_type == V4L2_H264_SLICE_TYPE_SP) ||
> -	    (slice->slice_type == V4L2_H264_SLICE_TYPE_B))
> -		cedrus_write_ref_list0(ctx, run);
> +	    (slice->slice_type == V4L2_H264_SLICE_TYPE_B)) {
> +		ret = cedrus_write_ref_list0(ctx, run);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	if (slice->slice_type == V4L2_H264_SLICE_TYPE_B)
> -		cedrus_write_ref_list1(ctx, run);
> +	if (slice->slice_type == V4L2_H264_SLICE_TYPE_B) {
> +		ret = cedrus_write_ref_list1(ctx, run);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	// picture parameters
>  	reg = 0;
> @@ -478,6 +490,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
>  		     VE_H264_CTRL_SLICE_DECODE_INT |
>  		     VE_H264_CTRL_DECODE_ERR_INT |
>  		     VE_H264_CTRL_VLD_DATA_REQ_INT);
> +
> +	return 0;
>  }
>  
>  static enum cedrus_irq_status
> @@ -531,9 +545,7 @@ static int cedrus_h264_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	if (ret)
>  		return ret;
>  
> -	cedrus_set_params(ctx, run);
> -
> -	return 0;
> +	return cedrus_set_params(ctx, run);
>  }
>  
>  static int cedrus_h264_start(struct cedrus_ctx *ctx)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]