Re: [PATCH v2 09/17] media: apple: avd: add hevc support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] avd: heap buffer overflow due to missing array bounds validation on pps->column_width_minus1
- [High] avd: kernel panic and buffer overflow via unbounded sps->log2_diff shift
- [High] avd: kernel stack out-of-bounds read via unbounded pos variable in stream_slices
- [High] avd: double out-of-bounds array read in stream_slice_mv via sl->collocated_ref_idx
- [High] avd: heap out-of-bounds read via off-by-one check on slice_segment_addr
- [High] avd: arbitrary physical DMA read via integer underflow in slice size offset validation
- [High] avd: use-after-free and out-of-bounds read due to lockless dereference of V4L2 dynamic array pointers
- [High] avd: integer overflow in entry point offset validation allows heap out-of-bounds read
- [Medium] avd: hardware register corruption due to overlapping bitmasks for HEVC_TR_INTRA and HEVC_TR_INTER
--

commit a7dbbf6a1924ee26ecce4f5c5b73414f86b42959
Author: Sofus Forstreuter <sofus.c@icloud.com>

    media: apple: avd: add hevc support
    
    The fluster score is 143/147 for JCT-VC-HEVC_V1.
    
    RPS_E_qualcomm_5, WPP_D_ericsson_MAIN_2 and WPP_D_ericsson_MAIN10_2 are
    failing, but should be supported.

> diff --git a/drivers/media/platform/apple/avd/avd-hevc.c b/drivers/media/platform/apple/avd/avd-hevc.c
> new file mode 100644
> index 0000000000000..d90c7770006b7
> --- /dev/null
> +++ b/drivers/media/platform/apple/avd/avd-hevc.c
> @@ -0,0 +1,1340 @@

[ ... ]

> +#define HEVC_TR_INTRA(v)	FIELD_PREP(GENMASK(4, 1), v)
> +#define HEVC_TR_INTER(v)	FIELD_PREP(GENMASK(7, 4), v)

[Severity: Medium]
Do these bitmasks overlap? It looks like both GENMASK(4, 1) and GENMASK(7, 4) 
include bit 4. When these are ORed together in set_header() like so:

	HEVC_TR_INTER(sps->max_transform_hierarchy_depth_inter) |
	HEVC_TR_INTRA(sps->max_transform_hierarchy_depth_intra) |

could this clobber the overlapping bit and misconfigure the hardware decoder?

[ ... ]

> +static void stream_slice_mv(struct avd_ctx *ctx, struct avd_hevc_run *run,
> +			    const struct v4l2_ctrl_hevc_slice_params *sl,
> +			    bool is_first)
> +{
> +	const struct v4l2_ctrl_hevc_decode_params *decode = run->decode;
> +	struct avd_decoded_buffer *dst, *ref;
> +	bool ref_valid;
> +	const u8 *ref_list;
> +	int ref_idx = 0;
> +
> +	ref_list = sl->slice_type == V4L2_HEVC_SLICE_TYPE_P ? sl->ref_idx_l0 :
> +		   sl->flags & V4L2_HEVC_SLICE_PARAMS_FLAG_COLLOCATED_FROM_L0 ?
> +							      sl->ref_idx_l0 :
> +							      sl->ref_idx_l1;
> +
> +	if (sl->slice_type == V4L2_HEVC_SLICE_TYPE_I) {
> +		push(AVD_OP_SL_REF |
> +			     AVD_OP_SL_REF_SLICE_I(sl->slice_type ==
> +						   V4L2_HEVC_SLICE_TYPE_I),
> +		     "slc_a8c_cmd_ref_type");
> +		return;
> +	}
> +	/* bidirectional prediction */
> +	if (sl->collocated_ref_idx < V4L2_HEVC_DPB_ENTRIES_NUM_MAX &&
> +	    ref_list[sl->collocated_ref_idx] < V4L2_HEVC_DPB_ENTRIES_NUM_MAX)
> +		ref_idx = ref_list[sl->collocated_ref_idx];
> +
> +	dst = vb2_to_avd_decoded_buf(&run->base.bufs.dst->vb2_buf);
> +	ref = avd_get_ref_buf(ctx, &dst->base.vb,
> +			      decode->dpb[ref_idx].timestamp);
> +
> +	ref_valid = !(sl->flags &
> +		      V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT) &&
> +		    (sl->flags &
> +		     V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_TEMPORAL_MVP_ENABLED) &&
> +		    is_first && !ref->hevc.is_intra;
> +
> +	push(AVD_OP_SL_REF |
> +		     AVD_OP_SL_REF_MAX_MERGE(
> +			     5 - sl->five_minus_max_num_merge_cand) |
> +		     AVD_OP_SL_REF_FLAG_CABAC(
> +			     sl->flags &
> +			     V4L2_HEVC_SLICE_PARAMS_FLAG_CABAC_INIT) |
> +		     AVD_OP_SL_REF_FLAG0(
> +			     (sl->flags &
> +			      V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_TEMPORAL_MVP_ENABLED) &&
> +			     !(sl->flags &
> +			       V4L2_HEVC_SLICE_PARAMS_FLAG_COLLOCATED_FROM_L0)) |
> +		     AVD_OP_SL_REF_FLAG1(
> +			     !(sl->flags &
> +			       V4L2_HEVC_SLICE_PARAMS_FLAG_MVD_L1_ZERO)) |
> +		     AVD_OP_SL_REF_FLAG2(
> +			     (sl->flags &
> +			      V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_TEMPORAL_MVP_ENABLED) ||
> +			     (sl->flags &
> +			      V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT)) |
> +		     AVD_OP_SL_REF_NUM_L0(sl->num_ref_idx_l0_active_minus1) |
> +		     AVD_OP_SL_REF_NUM_L1(sl->num_ref_idx_l1_active_minus1) |
> +		     AVD_OP_SL_REF_SLICE_P(sl->slice_type ==
> +					   V4L2_HEVC_SLICE_TYPE_P) |
> +		     AVD_OP_SL_REF_SLICE_B(ref_valid),
> +	     "slc_a8c_cmd_ref_type");
> +
> +	if (ref_valid) {
> +		dma_addr_t mv_color_addr =
> +			vb2_dma_contig_plane_dma_addr(&ref->base.vb.vb2_buf,
> +						      0) +
> +			(ref->base.vb.planes[0].length -
> +			 mv_color_size(fmt_width(ctx), fmt_height(ctx)));
> +		pusha(mv_color_addr, "slc_bd4_sps_tile_addr2_lsb8",
> +		      decode->dpb[ref_list[sl->collocated_ref_idx]]
> +			      .pic_order_cnt_val);

[Severity: High]
Is it possible to have an out-of-bounds read here? 

If sl->collocated_ref_idx (which is controlled by userspace) is >= 16 
(V4L2_HEVC_DPB_ENTRIES_NUM_MAX), it seems to bypass the initial bounds check
which updates ref_idx. Later, when pushing the command:

		      decode->dpb[ref_list[sl->collocated_ref_idx]]

could this read ref_list out-of-bounds, yielding a garbage index, and then 
subsequently use that to read out-of-bounds in decode->dpb, programming 
arbitrary memory values into the hardware?

> +	}
> +}

[ ... ]

> +static int submit_slice_segment(struct avd_ctx *ctx, struct avd_hevc_run *run,
> +				const struct v4l2_ctrl_hevc_slice_params *sl,
> +				int row, int col, u32 col_bd[23],
> +				u32 row_bd[23], u32 pic_in_cts_width,
> +				u32 pic_in_cts_height, bool first_slice,
> +				bool hflip, bool vflip, u32 coded_flags,
> +				u32 last_tile_block)
> +{
> +	const struct v4l2_ctrl_hevc_pps *pps = run->pps;
> +	u32 tb_x, tb_y, tile_block, tile_boundary;
> +
> +	if (coded_flags & NEW_SLICE) {
> +		tb_x = sl->slice_segment_addr % pic_in_cts_width;
> +		tb_y = sl->slice_segment_addr / pic_in_cts_width;
> +
> +		tile_block = AVD_OP_SL_LOC_Y(tb_y) | AVD_OP_SL_LOC_X(tb_x);
> +
> +		if (!(sl->flags &
> +		      V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT))
> +			last_tile_block = tile_block;
> +
> +		/*
> +		 * tile block start
> +		 * CABAC window
> +		 */
> +		push(AVD_OP_SL_LOC | last_tile_block, "cm3_cmd_set_cabac_xy");
> +
> +		stream_slice_dqtblk(ctx, run, sl);
> +	} else {
> +		tile_boundary = AVD_OP_SL_LOC_Y(row_bd[row]) |
> +				AVD_OP_SL_LOC_X(col_bd[col]);

[Severity: High]
Can the 'row' index here exceed the bounds of the 45-element row_bd kernel
stack array?

In stream_slices(), the row index is calculated from an unbounded 'pos' 
counter which increments for each entry point offset:

			row = pos / num_cols;
			col = pos % num_cols;

Since there can be up to 1760 entry point offsets, 'row' is not bounded
against 44. Could this result in an out-of-bounds stack read?

> +	}

[ ... ]

> +static void compute_tiles_non_uniform(struct avd_hevc_run *run,
> +				      u16 log2_min_cb_size, u16 width,
> +				      u16 height, u32 pic_in_cts_width,
> +				      u32 pic_in_cts_height, u16 *column_width,
> +				      u16 *row_height)
> +{
> +	const struct v4l2_ctrl_hevc_pps *pps = run->pps;
> +	u32 sum = 0;
> +	int i;
> +
> +	for (i = 0; i < pps->num_tile_columns_minus1; i++) {
> +		column_width[i] = MIN(pps->column_width_minus1[i] + 1,
> +				      pic_in_cts_width);
> +		sum += column_width[i];
> +	}
> +	column_width[i] = pic_in_cts_width - sum;

[Severity: Critical]
Does this code overflow column_width_minus1[] and also underflow
column_width[i]?

The validation allows pps->num_tile_columns_minus1 up to 39, but the V4L2
payload array column_width_minus1 only has 20 elements. This could cause an
out-of-bounds read of the struct.

Additionally, since sum is not validated, if sum exceeds pic_in_cts_width, 
does pic_in_cts_width - sum underflow into a huge unsigned 16-bit integer? 

This huge dimension seems to propagate into compute_bd() and compute_tile_ids(), 
potentially causing thousands of out-of-bounds memory writes on the kernel heap 
when indexing ctb_addr_rs_to_ts and tile_ids.

> +
> +	sum = 0;

[ ... ]

> +static void stream_slices(struct avd_ctx *ctx, struct avd_hevc_run *run)
> +{
> +	const struct v4l2_ctrl_hevc_sps *sps = run->sps;
> +	const struct v4l2_ctrl_hevc_pps *pps = run->pps;
> +	const struct v4l2_ctrl_hevc_slice_params *sl;
> +	struct avd_hevc_tile_info *tile_info = &run->tile_info;
> +	bool tiles_enabled, first_slice, first_segment;
> +	bool hflip, vflip;
> +	int slice_segment_offset, entry_point_idx = 0, pos = 0, offset = 0;
> +	int row, col, i, s, to;
> +	int slice_flag, size, new_offset;
> +	int tile_id, last_tile_id;
> +	u16 log2_min_cb_size, width, height;
> +	u32 max_cu_width, pic_in_ctbs_width, pic_in_ctbs_height;
> +	u32 pic_in_ctbs_size, num_cols, last_tile_block = 0;
> +	struct sl_ctx last = {
> +		.q1_col = -1,
> +		.q1_row = -1,
> +	};
> +
> +	width = sps->pic_width_in_luma_samples;
> +	height = sps->pic_height_in_luma_samples;
> +
> +	tiles_enabled = !!(pps->flags & V4L2_HEVC_PPS_FLAG_TILES_ENABLED);
> +
> +	log2_min_cb_size = sps->log2_min_luma_coding_block_size_minus3 + 3;
> +
> +	num_cols = pps->num_tile_columns_minus1 + 1;
> +
> +	max_cu_width = 1 << (sps->log2_diff_max_min_luma_coding_block_size +
> +			     log2_min_cb_size);
> +	pic_in_ctbs_width = (width + max_cu_width - 1) / max_cu_width;
> +	pic_in_ctbs_height = (height + max_cu_width - 1) / max_cu_width;
> +	pic_in_ctbs_size = pic_in_ctbs_width * pic_in_ctbs_height;
> +
> +	for (s = 0; s < run->num_slices; s++) {
> +		sl = &(*run->sl)[s];
> +		slice_segment_offset = 0;
> +
> +		/* entry_points are not needed for WPP */
> +		to = tiles_enabled ? sl->num_entry_point_offsets + 1 : 1;
> +
> +		for (i = 0; i < to; i++) {
> +			first_segment = i == 0;
> +			first_slice = s == 0;
> +
> +			if (tiles_enabled && to > 1) {
> +				if (i < sl->num_entry_point_offsets) {
> +					size = (*run->entry_point_offsets)
> +						       [entry_point_idx++];

[Severity: High]
Is it safe to locklessly dereference the V4L2 dynamic array pointer here?

In avd_hevc_run_preamble(), the pointer to the dynamic array control is cached
without acquiring the V4L2 state lock:

	run->entry_point_offsets = ctrl ? (const u32 **)&ctrl->p_cur.p : NULL;
	...
	run->sl = ctrl ?
		(const struct v4l2_ctrl_hevc_slice_params **)&ctrl->p_cur.p
		: NULL;

If a concurrent VIDIOC_S_EXT_CTRLS ioctl reallocates the array to a smaller size
while the decoder is running, *run->entry_point_offsets will point to the new
smaller array, but the cached bounds will remain large, leading to out-of-bounds 
heap reads here. Also, dereferencing p_cur.p concurrently while it is being freed 
could trigger a Use-After-Free.

[Severity: High]
Does this code allow an integer overflow during validation?

In avd_hevc_run_preamble(), the total number of entry points is accumulated
into a signed 32-bit int:

	for (i = 0; i < run->num_slices; i++)
		sum += (*run->sl)[i].num_entry_point_offsets;

If userspace provides extremely large num_entry_point_offsets, could this
sum wrap around and exactly match run->num_entry_point_offsets to bypass the 
validation? If so, entry_point_idx could iterate out-of-bounds and read past 
the end of the allocated entry_point_offsets array here.

> +					new_offset = size;
> +				} else {
> +					size = sl->bit_size / 8 -
> +					       sl->data_byte_offset -
> +					       slice_segment_offset;
> +					new_offset =
> +						size + sl->data_byte_offset;
> +				}
> +			} else {
> +				size = (sl->bit_size) / 8 -
> +				       sl->data_byte_offset;
> +				new_offset = size + sl->data_byte_offset;

[Severity: High]
Can the subtraction here trigger an integer underflow?

Userspace could supply sl->data_byte_offset or entry point offsets such that 
they exceed sl->bit_size / 8. This would cause an underflow, resulting in a 
huge integer for 'size'. 

When set_slice() is called:

	dma_addr_t coded_in =
		run->base.coded_in + offset + sl->data_byte_offset;

this uncontrolled size, alongside an unvalidated memory address, is programmed 
into the hardware, potentially causing it to perform arbitrary DMA memory reads.

> +			}
> +
> +			if (sl->slice_segment_addr > pic_in_ctbs_size)
> +				return;
> +
> +			tile_id = tile_info->tile_ids
> +					  [tile_info->ctb_addr_rs_to_ts
> +						   [sl->slice_segment_addr]];

[Severity: High]
Does this validation allow an off-by-one error?

Because it checks for > rather than >=, could sl->slice_segment_addr exactly 
equal pic_in_ctbs_size, leading to a heap out-of-bounds read of 
ctb_addr_rs_to_ts? The resulting garbage value might then be used to read 
tile_ids out-of-bounds.

> +			last_tile_id =

[ ... ]

> +static int avd_hevc_compute_tiles(struct avd_ctx *ctx, struct avd_hevc_run *run)
> +{
> +	const struct v4l2_ctrl_hevc_sps *sps = run->sps;
> +	const struct v4l2_ctrl_hevc_pps *pps = run->pps;
> +	bool tiles_enabled;
> +	struct avd_hevc_tile_info *tile_info = &run->tile_info;
> +	u16 log2_min_cb_size, width, height;
> +	u32 max_cu_width, pic_in_ctbs_width, pic_in_ctbs_height,
> +		pic_in_ctbs_size;
> +
> +	width = sps->pic_width_in_luma_samples;
> +	height = sps->pic_height_in_luma_samples;
> +
> +	log2_min_cb_size = sps->log2_min_luma_coding_block_size_minus3 + 3;
> +
> +	max_cu_width = 1 << (sps->log2_diff_max_min_luma_coding_block_size +
> +			     log2_min_cb_size);
> +	pic_in_ctbs_width = (width + max_cu_width - 1) / max_cu_width;

[Severity: High]
Does this code permit an unbounded bit shift?

Userspace provides sps->log2_diff_max_min_luma_coding_block_size and 
sps->log2_min_luma_coding_block_size_minus3 without bounds checking. If their
sum exceeds 31, the shift will result in undefined behavior. This could yield 0,
triggering a zero division on the very next line.

Alternatively, if it evaluates to a huge value (like 1 << 30), could this 
overflow the 32-bit math during scratch allocations in avd_hevc_alloc_scratch() 
like so:

	ret = avd_buf_alloc(avd, &hevc_ctx->bufs.ip_above,
			    ((max_col * max_cu_width) / 4) * bit_depth);

causing it to wrap around to a dangerously small size, leading to hardware
memory corruption?

> +	pic_in_ctbs_height = (height + max_cu_width - 1) / max_cu_width;
> +	pic_in_ctbs_size = pic_in_ctbs_height * pic_in_ctbs_width;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926-avd-v2-0-ecebe6a3648e@icloud.com?part=9