Re: [PATCH v2] media: v4l2-ctrls: validate AV1 ref_frame_idx and primary_ref_frame

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

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

Le jeudi 09 juillet 2026 à 15:12 +0200, Doruk Tan Ozturk a écrit :
> The stateless AV1 frame control V4L2_CID_STATELESS_AV1_FRAME carries
> ref_frame_idx[V4L2_AV1_REFS_PER_FRAME] (signed, -128..127) and
> primary_ref_frame (__u8), both copied from userspace. validate_av1_frame()
> already checks flags, quantization, segmentation, loop filter, CDEF, loop
> restoration and superres, but never bounds these reference indices.
> 
> Decoders use them directly as array subscripts. In the MediaTek decoder
> vdec_av1_slice_setup_ref() does:
> 
> 	int ref_idx = ctrl_fh->ref_frame_idx[i];
> 	pfc->ref_idx[i] = ctrl_fh->reference_frame_ts[ref_idx];
> 	slot_id = frame->ref_frame_map[ref_idx];
> 
> indexing reference_frame_ts[8] and ref_frame_map[8] with an attacker
> controlled signed value. In the Rockchip/verisilicon decoder the CDF setup
> does:
> 
> 	rockchip_av1_get_cdfs(ctx,
> 		frame->ref_frame_idx[frame->primary_ref_frame]);
> 
> indexing ref_frame_idx[7] with the unbounded primary_ref_frame. Both are
> out-of-bounds reads driven by unvalidated userspace input.
> 
> Validate in the core, like the other AV1 frame fields, so every decoder is
> covered: reject ref_frame_idx entries outside
> [0, V4L2_AV1_TOTAL_REFS_PER_FRAME) and primary_ref_frame >=
> V4L2_AV1_TOTAL_REFS_PER_FRAME. The upper bound still permits the value 7
> (PRIMARY_REF_NONE), which decoders handle explicitly.
> 
> Found by 0sec's autonomous vulnerability analysis (https://0sec.ai).
> Found by static analysis; not yet runtime-reproduced (Rockchip/MediaTek SoC
> hardware required).
> 
> Fixes: 9de30f579980 ("media: Add AV1 uAPI")
> Assisted-by: 0sec:claude-opus-4.8
> Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
> ---
> v2: wrap a quoted decoder line in the commit message to satisfy checkpatch
>     (<=75 cols); no code change from v1.
> v1: https://lore.kernel.org/linux-media/20260628134105.21001-1-doruk@0sec.ai/
> 
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index ba047d7d8601..5096c48ea402 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -793,6 +793,7 @@ static int validate_av1_film_grain(struct v4l2_ctrl_av1_film_grain *fg)
>  static int validate_av1_frame(struct v4l2_ctrl_av1_frame *f)
>  {
>  	int ret = 0;
> +	u32 i;
>  
>  	ret = validate_av1_quantization(&f->quantization);
>  	if (ret)
> @@ -836,6 +837,14 @@ static int validate_av1_frame(struct v4l2_ctrl_av1_frame *f)
>  	if (f->superres_denom > GENMASK(2, 0) + 9)
>  		return -EINVAL;
>  
> +	for (i = 0; i < ARRAY_SIZE(f->ref_frame_idx); i++)
> +		if (f->ref_frame_idx[i] < 0 ||
> +		    f->ref_frame_idx[i] >= V4L2_AV1_TOTAL_REFS_PER_FRAME)
> +			return -EINVAL;

This is likely a breaking changes, and since you didn't test it I'm being very
cautious. Through code inspection, I see that GStreamer will fill the
ref_frame_idx to value -1 when reference is unused. This happens notably on
every keyframe.

> +
> +	if (f->primary_ref_frame >= V4L2_AV1_TOTAL_REFS_PER_FRAME)
> +		return -EINVAL;

As per spec, this shouldn't be bigger then AV1_PRIMARY_REF_NONE (7). But it does
seems that passing values above can cause issues in
rockchip_vpu981_av1_dec_set_prob() notably.

Nicolas

> +
>  	return 0;
>  }
>  

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