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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] media: apple: avd: OOB read of PPS tile arrays in compute_tiles_non_uniform()
- [Critical] media: apple: avd: tile size sum underflow in compute_tiles_non_uniform()
- [Critical] media: apple: avd: entry_point_offsets sum overflow in avd_hevc_run_preamble()
- [High] media: apple: avd: wrong plane length field in stream_slice_mv()
- [High] media: apple: avd: OOB stack read of row_bd in submit_slice_segment()
- [High] media: apple: avd: missing SPS block size and dimension validation
--

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

media: apple: avd: add hevc support

This patch adds stateless V4L2 HEVC decoding support to the Apple Video
Decoder (AVD) driver.

> 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..36ae24c640a10
> --- /dev/null
> +++ b/drivers/media/platform/apple/avd/avd-hevc.c

[ ... ]

> +#define HEVC_SCL_DIMS	0x127ffff
> +
> +/* this is for level 7.1 */
> +#define HEVC_MAX_TILE_COLS	40
> +#define HEVC_MAX_TILE_ROWS	44

[ ... ]

> +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)
> +{

[ ... ]

> +	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, "mv_colo", 0);
> +	}
> +}

[Severity: High]
Should stream_slice_mv() use ref->base.vb.vb2_buf.planes[0].length instead
of ref->base.vb.planes[0].length here?

In avd_hevc_run_preamble(), the destination buffer's mv_color address is
computed from vb2_buf.planes[0].length:

	dst_len = run->base.bufs.dst->vb2_buf.planes[0].length;
	mv_color_len = mv_color_size(fmt_width(ctx), fmt_height(ctx));
	run->addresses.mv_color = run->base.y_out + (dst_len - mv_color_len);

When capture buffers are queued using VB2_MEMORY_DMABUF with
v4l2_plane.length set to 0, __prepare_dmabuf() in videobuf2-core.c sets
vb2_buf.planes[0].length = dbuf->size, while leaving
vb2_v4l2_buffer.planes[0].length as 0.

When decoding a P or B slice with
V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_TEMPORAL_MVP_ENABLED and a non-intra
collocated reference frame, subtracting mv_color_size() from
ref->base.vb.planes[0].length (0) underflows the 32-bit unsigned value and
programs the hardware with an out-of-bounds DMA address.

Also, in set_slice(), coded_in and size are computed from sl->bit_size,
sl->data_byte_offset, and entry_point_offsets without bounds checking
against the source buffer plane size.

> +
> +static void set_slice(struct avd_ctx *ctx, struct avd_hevc_run *run,
> +		      const struct v4l2_ctrl_hevc_slice_params *sl, u32 size,
> +		      u32 offset, u32 flags)
> +{
> +	dma_addr_t coded_in =
> +		run->base.coded_in + offset + sl->data_byte_offset;
> +	push(AVD_OP_CODED_DATA | flags | AVD_OP_CODED_IN_HI(coded_in),
> +	     "cm3_cmd_set_coded_slice");
> +	push(AVD_OP_CODED_IN_LO(coded_in), "slc_bd8_slice_addr");
> +	push(size, "slc_bdc_slice_size");
> +}
> +
> +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)
> +{

[ ... ]

> +	} else {
> +		tile_boundary = AVD_OP_SL_LOC_Y(row_bd[row]) |
> +				AVD_OP_SL_LOC_X(col_bd[col]);
> +	}
> +
> +	if (coded_flags & NEW_TILE_ID) {
> +		push(AVD_OP_SL_DIM_START |
> +			     (coded_flags & NEW_SLICE ? tile_block :
> +							tile_boundary),
> +		     "cm3_cmd_set_ctb_xy");
> +
> +		/* tile boundary end */
> +		if (pps->flags & V4L2_HEVC_PPS_FLAG_TILES_ENABLED)
> +			push(AVD_SL_DIM_END_ROW((hflip ? 4 : 0) |
> +						(vflip ? 8 : 0)) |
> +				     AVD_SL_DIM_END_COL(col) |
> +				     AVD_SL_DIM_END_Y(row_bd[row + 1] - 1) |
> +				     AVD_SL_DIM_END_X(col_bd[col + 1] - 1),
> +			     "cm3_set_ctb_xy");

[Severity: High]
Can submit_slice_segment() read past the end of row_bd[] on the stack?

In stream_slices(), pos is incremented for every segment where
(slice_flag & NEW_TILE_ID) is set, and row is computed as:

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

Across up to 1800 slices and 1760 entry point offsets, pos can reach up to
3560 while num_cols (pps->num_tile_columns_minus1 + 1) can be 1. Because
row is not checked against pps->num_tile_rows_minus1 or HEVC_MAX_TILE_ROWS,
submit_slice_segment() can index row_bd[row] and row_bd[row + 1] past the
end of run.tile_info.row_bd[HEVC_MAX_TILE_ROWS + 1].

Note also that submit_slice_segment() declares u32 col_bd[23] and
u32 row_bd[23], which do not match HEVC_MAX_TILE_COLS + 1 and
HEVC_MAX_TILE_ROWS + 1.

[ ... ]

> +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;
> +
> +	sum = 0;
> +	for (i = 0; i < pps->num_tile_rows_minus1; i++) {
> +		row_height[i] = MIN(pps->row_height_minus1[i] + 1,
> +				    pic_in_cts_height);
> +		sum += row_height[i];
> +	}
> +	row_height[i] = pic_in_cts_height - sum;
> +}

[Severity: Critical]
Can compute_tiles_non_uniform() read out of bounds from
pps->column_width_minus1[] and pps->row_height_minus1[]?

In include/uapi/linux/v4l2-controls.h, struct v4l2_ctrl_hevc_pps defines:

	__u8 column_width_minus1[20];
	__u8 row_height_minus1[22];

However, HEVC_MAX_TILE_COLS is 40 and HEVC_MAX_TILE_ROWS is 44, so
avd_hevc_validate_pps() allows pps->num_tile_columns_minus1 up to 39 and
pps->num_tile_rows_minus1 up to 43. When V4L2_HEVC_PPS_FLAG_TILES_ENABLED
is set without V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING, these loops read
pps->column_width_minus1[i] up to index 38 and pps->row_height_minus1[i]
up to index 42, reading past both arrays and the end of struct
v4l2_ctrl_hevc_pps.

[Severity: Critical]
In addition, what happens in compute_tiles_non_uniform() if the cumulative
sum of column widths or row heights exceeds pic_in_cts_width or
pic_in_cts_height?

Clamping each individual column or row with MIN() does not prevent sum from
exceeding pic_in_cts_width or pic_in_cts_height across multiple columns or
rows. When sum > pic_in_cts_width (or sum > pic_in_cts_height), the final
assignment:

	column_width[i] = pic_in_cts_width - sum;
	...
	row_height[i] = pic_in_cts_height - sum;

underflows in u16 to a large value (up to 65535). compute_bd() then
computes col_bd and row_bd boundaries exceeding pic_in_ctbs_width and
pic_in_ctbs_height, causing compute_tile_ids() to read
ctb_addr_rs_to_ts[y * pic_in_ctbs_width + x] and write tile_ids[...] out of
bounds on the heap:

> +static void compute_tile_ids(struct avd_hevc_run *run, u32 pic_in_ctbs_width,
> +			     u32 *col_bd, u32 *row_bd, u32 *ctb_addr_rs_to_ts,
> +			     u32 *tile_ids)
> +{
> +	const struct v4l2_ctrl_hevc_pps *pps = run->pps;
> +	int j, i, y, x;
> +	u32 tile_idx;
> +
> +	for (j = 0, tile_idx = 0; j <= pps->num_tile_rows_minus1; j++)
> +		for (i = 0; i <= pps->num_tile_columns_minus1; i++, tile_idx++)
> +			for (y = row_bd[j]; y < row_bd[j + 1]; y++)
> +				for (x = col_bd[i]; x < col_bd[i + 1]; x++)
> +					tile_ids[ctb_addr_rs_to_ts
> +							 [y * pic_in_ctbs_width +
> +							  x]] = tile_idx;
> +}

[ ... ]

> +static int avd_hevc_validate_sps(struct avd_ctx *ctx,
> +				 const struct v4l2_ctrl_hevc_sps *sps)
> +{
> +	if (sps->pic_width_in_luma_samples > ctx->coded_fmt.fmt.pix_mp.width ||
> +	    sps->pic_height_in_luma_samples > ctx->coded_fmt.fmt.pix_mp.height)
> +		return -EINVAL;
> +
> +	if (sps->bit_depth_chroma_minus8 != sps->bit_depth_luma_minus8)
> +		return -EINVAL;
> +
> +	if (sps->bit_depth_luma_minus8 > 2)
> +		/* not implemented */
> +		return -EINVAL;
> +
> +	return 0;
> +}

[Severity: High]
Does avd_hevc_validate_sps() also need to validate that picture dimensions
are non-zero and bound log2_min_luma_coding_block_size_minus3 and
log2_diff_max_min_luma_coding_block_size?

In avd_hevc_run(), avd_hevc_compute_tiles() runs before
avd_hevc_alloc_scratch() (where the !max_cu_width check is located) and
executes:

	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;
	pic_in_ctbs_height = (height + max_cu_width - 1) / max_cu_width;
	pic_in_ctbs_size = pic_in_ctbs_height * pic_in_ctbs_width;

If the shift exponent is >= 31, 1 << ... triggers signed 32-bit shift
undefined behavior; if it is >= 32 (or wraps to 0), dividing by max_cu_width
divides by zero, and if it is 31, max_cu_width is 0x80000000, making
pic_in_ctbs_width = 0 and causing division by zero in compute_rs_to_ts() or
submit_slice_segment(), as well as 32-bit overflow in
((max_col * max_cu_width) / 4) * bit_depth in avd_hevc_alloc_scratch().

Similarly, if width or height is 0 (for example, if streaming starts before
setting V4L2_CID_STATELESS_HEVC_SPS), pic_in_ctbs_size is 0 and
kzalloc_objs() returns ZERO_SIZE_PTR, which is then dereferenced. Also,
because avd_hevc_validate_sps() is only called from avd_hevc_try_ctrl(),
decreasing the coded format dimensions via VIDIOC_S_FMT after setting the
SPS control leaves stale SPS dimensions larger than the format.

> +
> +static int avd_hevc_validate_pps(struct avd_ctx *ctx,
> +				 const struct v4l2_ctrl_hevc_pps *pps)
> +{
> +	if (pps->num_tile_columns_minus1 + 1 > HEVC_MAX_TILE_COLS ||
> +	    pps->num_tile_rows_minus1 + 1 > HEVC_MAX_TILE_ROWS)
> +		return -EINVAL;
> +
> +	return 0;
> +}

[ ... ]

> +static int avd_hevc_run_preamble(struct avd_ctx *ctx, struct avd_hevc_run *run)
> +{

[ ... ]

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

[Severity: Critical]
Can sum overflow when accumulating (*run->sl)[i].num_entry_point_offsets in
avd_hevc_run_preamble()?

(*run->sl)[i].num_entry_point_offsets is an unvalidated __u32 accumulated
into a 32-bit signed int sum. If two or more slices have
num_entry_point_offsets values whose 32-bit sum wraps to 0 or to
run->num_entry_point_offsets, this check passes while avd_init_job() only
allocates run.num_slices + run.num_entry_point_offsets + 1 segments.

In stream_slices():

	to = tiles_enabled ? sl->num_entry_point_offsets + 1 : 1;

	for (i = 0; i < to; i++) {
		...
		if (tiles_enabled && to > 1) {
			if (i < sl->num_entry_point_offsets) {
				size = (*run->entry_point_offsets)
					       [entry_point_idx++];

When tiles_enabled is true, this loop can then read
(*run->entry_point_offsets)[entry_point_idx++] out of bounds on the heap
and write past the end of ctx->job.segments via push() and
avd_end_segment(). Moreover, if sl->num_entry_point_offsets >= 0x7fffffff,
int to = sl->num_entry_point_offsets + 1 overflows to <= 0, skipping the
inner loop and its sl->slice_segment_addr >= pic_in_ctbs_size check for
slice s, so slice s + 1 indexes
tile_info->ctb_addr_rs_to_ts[(*run->sl)[s].slice_segment_addr] out of
bounds.

> +
> +	dst_len = run->base.bufs.dst->vb2_buf.planes[0].length;
> +
> +	mv_color_len = mv_color_size(fmt_width(ctx), fmt_height(ctx));
> +
> +	run->addresses.mv_color = run->base.y_out + (dst_len - mv_color_len);
> +	return 0;
> +}

[ ... ]

> +static int avd_hevc_run(struct avd_ctx *ctx)
> +{
> +	struct avd_hevc_run run = {};
> +	struct avd_decoded_buffer *dst;
> +	int ret;
> +
> +	ret = avd_hevc_run_preamble(ctx, &run);
> +	if (ret)
> +		goto postamble;
> +
> +	dst = vb2_to_avd_decoded_buf(&run.base.bufs.dst->vb2_buf);
> +	update_dec_buf_info(dst, &(*run.sl)[0]);
> +
> +	ret = avd_hevc_compute_tiles(ctx, &run);
> +	if (ret)
> +		goto done;
> +
> +	ret = avd_hevc_alloc_scratch(ctx, &run);
> +	if (ret)
> +		goto done;
> +
> +	ret = avd_init_job(ctx, AVD_CODEC_HEVC,
> +			   run.num_slices + run.num_entry_point_offsets + 1);
> +	if (ret)
> +		goto done;

[ ... ]

> +	return ret;
> +}

[ ... ]

> diff --git a/drivers/media/platform/apple/avd/avd-v4l2.c b/drivers/media/platform/apple/avd/avd-v4l2.c
> index 0f83a17f16fae..34db42e008332 100644
> --- a/drivers/media/platform/apple/avd/avd-v4l2.c
> +++ b/drivers/media/platform/apple/avd/avd-v4l2.c

[ ... ]

> +	{
> +		.cfg.id = V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS,
> +		.cfg.flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
> +		.cfg.dims = { 1760 },
> +		.cfg.max = 0xffffffff,
> +		.cfg.step = 1,
> +	},

[ ... ]

> diff --git a/drivers/media/platform/apple/avd/avd.h b/drivers/media/platform/apple/avd/avd.h
> index bd6177ab1eb25..828d25f85902e 100644
> --- a/drivers/media/platform/apple/avd/avd.h
> +++ b/drivers/media/platform/apple/avd/avd.h

[ ... ]

> +struct avd_hevc_decoded_buffer_info {
> +	bool is_intra;
> +};

[ ... ]

>  extern const struct v4l2_ctrl_ops avd_ctrl_ops;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-avd-v3-0-85cbaa2125d8@icloud.com?part=9