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

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

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

Le vendredi 10 juillet 2026 à 16:34 -0400, Nicolas Dufresne a écrit :
> 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;
> > +

Ah, got confused in triaging, this one got actually replaced by what we already
merged, which is just these two lines. Marking as obsolete, feel free to follow-
up.

cheers,
Nicolas

> >  		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 --]