Re: [PATCH 14/14] media: apple: avd: add av1 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [Critical] media: apple: avd: integer underflow in DMA offsets from unvalidated AV1 frame dimensions
- [High] media: apple: avd: out-of-bounds read due to unvalidated `tile_cols` and `tile_rows`
- [High] media: apple: avd: out-of-bounds read via unvalidated tile group row/col indices
- [High] media: apple: avd: nested out-of-bounds reads from unvalidated reference indices
- [High] media: apple: avd: integer truncation in `submit_num` causes hardware hang
- [High] media: apple: avd: ignored return value from `avd_av1_alloc_work_bufs` leads to uninitialized DMA access
- [High] media: apple: avd: division by zero in `calc_upscale` with `upscaled_width` 0
- [High] media: apple: avd: integer overflow in sb_cols DMA size calculation
- [High] media: apple: avd: silent DMA address truncation due to missing 256-byte alignment enforcement
- [High] media: apple: avd: hardware FIFO overflow from blind MMIO execution loops
- [Medium] media: apple: avd: memory leak of `above_info` DMA buffer in `avd_av1_stop`
- [Medium] media: apple: avd: missing cleanup in `avd_av1_run` upon job initialization failure
- [Medium] media: apple: avd: uninitialized DMA address pushed for segmentation map
- [Medium] media: apple: avd: missing DMA cache synchronization around CPU memcpy
--

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

    media: apple: avd: add av1 support

    This adds AV1 decoding support for newer Apple SoCs, utilizing
    entropy tables adapted from verisilicon drivers. The implementation
    passes the AV1-TEST-VECTORS test suite with a 238/242 score.

> diff --git a/drivers/media/platform/apple/avd/avd-av1.c b/drivers/media/platform/apple/avd/avd-av1.c
> new file mode 100644
> index 0000000000000..802341b230e26
> --- /dev/null
> +++ b/drivers/media/platform/apple/avd/avd-av1.c
> @@ -0,0 +1,1447 @@
> +// SPDX-License-Identifier: GPL-2.0
[ ... ]
> +struct avd_av1_ctx {
> +	u8 submit_num;

[Severity: High]
Does this regression truncate the result of tile_cols * tile_rows? The
submit_num field is declared as a u8.

[ ... ]
> +static void calc_upscale(int frame_width, int upscaled_width, int sub_x,
> +			 int *step_x, int *initial_subpel_x)
> +{
> +	int downscaled_plane_w, upscaled_plane_w, err;
> +
> +	downscaled_plane_w = AV1_DIV_ROUND_UP_POW2(frame_width, sub_x);
> +	upscaled_plane_w = AV1_DIV_ROUND_UP_POW2(upscaled_width, sub_x);
> +	*step_x = ((downscaled_plane_w << SUPERRES_SCALE_BITS) +
> +		   (upscaled_plane_w / 2)) /
> +		  upscaled_plane_w;

[Severity: High]
Is there a risk of a division by zero regression here? If userspace sets
upscaled_width to 0 in the V4L2_CID_STATELESS_AV1_FRAME control,
upscaled_plane_w could evaluate to 0.

[ ... ]
> +	pusha(run->addresses.probs_out, "probs_out", 0);
> +	pusha(av1_ctx->bufs.probs.addr, "probs", 1);
> +	pusha(av1_ctx->bufs.above_info.addr, "col", 0);
> +	pusha(av1_ctx->bufs.seg.addr, "seg", 0);

[Severity: Medium]
Can this push an uninitialized DMA address to the hardware? This regression
can occur because the seg buffer is only allocated if
V4L2_AV1_SEGMENTATION_FLAG_UPDATE_MAP is set. If the flag isn't set on the
first frame of a context, av1_ctx->bufs.seg.addr will be 0.

[ ... ]
> +static void set_tiles(struct avd_ctx *ctx, struct avd_av1_run *run)
> +{
> +	struct avd_av1_ctx *av1_ctx = ctx->priv;
> +	const struct v4l2_ctrl_av1_frame *frame = run->frame;
> +	const struct v4l2_ctrl_av1_sequence *seq = run->seq;
> +	const struct v4l2_ctrl_av1_tile_group_entry *tile_group;
> +	const struct v4l2_av1_tile_info *tile_info = &frame->tile_info;
> +	int row, col, sb_row, sb_col, tile_id;
> +	int sb_shift =
> +		seq->flags & V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK ? 5 :
> +									     4;
> +
> +	av1_ctx->submit_num = tile_info->tile_cols * tile_info->tile_rows;

[Severity: High]
As mentioned earlier, could this multiplication truncate and result in 0?
If it does, the submission loop later would see 0 - 1, which is -1, and
bypass the execution loop entirely.

> +
> +	for (row = 0; row < tile_info->tile_rows; row++) {
> +		for (col = 0; col < tile_info->tile_cols; col++) {
> +			ctx->job.num++;
> +			tile_id = row * tile_info->tile_cols + col;
> +			tile_group = &run->tile_group[tile_id];

[Severity: High]
Could this loop generate a tile_id that reads far out of bounds? If
tile_cols and tile_rows are unvalidated and reach their maximums, it
might exceed the dimensions of run->tile_group which is bounded by
V4L2_AV1_MAX_TILE_COUNT.

> +
> +			push(AVD_OP_CODED_DATA |
> +				     AVD_OP_CODED_DATA_ADDR(
> +					     (run->base.coded_in +
> +					      tile_group->tile_offset) >>
> +					     32),
> +			     "tile_start");
> +			push((u32)((run->base.coded_in +
> +				    tile_group->tile_offset) &
> +				   0xffffffff),
> +			     "tile_addr");
> +			push(tile_group->tile_size, "tile_size");
> +
> +			sb_row =
> +				tile_info->mi_row_starts[tile_group->tile_row] >>
> +				sb_shift;
> +			sb_col =
> +				tile_info->mi_col_starts[tile_group->tile_col] >>
> +				sb_shift;
> +			push(AVD_OP_SL_DIM_START |
> +				     AVD_OP_SL_DIM_START_Y(sb_row) |
> +				     AVD_OP_SL_DIM_START_X(sb_col),
> +			     "tile_op_start");
> +
> +			push(AVD_SL_DIM_END_ROW(
> +				     !(frame->flags &
> +				       V4L2_AV1_FRAME_FLAG_DISABLE_FRAME_END_UPDATE_CDF) &&
> +						     tile_id ==
> +							     tile_info->context_update_tile_id ?
> +					     8 :
> +					     0) |
> +				     AVD_SL_DIM_END_COL(col) |
> +				     AVD_SL_DIM_END_Y(
> +					     sb_row +
> +					     tile_info->height_in_sbs_minus_1
> +						     [tile_group->tile_row]) |

[Severity: High]
Could this regression cause out-of-bounds reads into mi_row_starts and
height_in_sbs_minus_1? The tile_row and tile_col fields come directly
from the V4L2_CID_STATELESS_AV1_TILE_GROUP_ENTRY control payload. If
userspace provides large values, it might read past the bounds of these
small fixed-size arrays and push those values to the hardware.

[ ... ]
> +static void avd_av1_set_prob(struct avd_ctx *ctx, struct avd_av1_run *run)
> +{
> +	struct avd_av1_ctx *av1_ctx = ctx->priv;
> +	bool error_resilient_mode = !!(
> +		run->frame->flags & V4L2_AV1_FRAME_FLAG_ERROR_RESILIENT_MODE);
> +	bool frame_is_intra =
> +		((run->frame->frame_type == V4L2_AV1_KEY_FRAME) ||
> +		 (run->frame->frame_type == V4L2_AV1_INTRA_ONLY_FRAME));
> +	struct avd_decoded_buffer *dst, *ref;
> +
> +	dst = vb2_to_avd_decoded_buf(&run->base.bufs.dst->vb2_buf);
> +
> +	if (error_resilient_mode || frame_is_intra ||
> +	    run->frame->primary_ref_frame == 7) {
> +		avd_av1_default_coeff_probs(run->frame->quantization.base_q_idx,
> +					    av1_ctx->bufs.probs.cpu);
> +		avd_av1_set_default_cdfs(av1_ctx->bufs.probs.cpu);
> +	} else {
> +		ref = avd_get_ref_buf(
> +			ctx, &dst->base.vb,
> +			run->frame->reference_frame_ts
> +				[run->frame->ref_frame_idx
> +					 [run->frame->primary_ref_frame]]);

[Severity: High]
Is it possible for primary_ref_frame or the fetched ref_frame_idx to
exceed the bounds of their respective arrays? If userspace supplies a
primary_ref_frame greater than 7, this could lead to a nested
out-of-bounds read regression when looking up the reference buffer.

> +		memcpy(av1_ctx->bufs.probs.cpu,
> +		       vb2_plane_vaddr(&ref->base.vb.vb2_buf, 0) +
> +			       AVD_AV1_CDFS_OFFSET(
> +				       ref->base.vb.vb2_buf.planes[0].length,
> +				       ref->av1.color_size),
> +		       sizeof(struct avd_av1_cdfs));

[Severity: Medium]
Is a DMA cache synchronization needed before accessing this buffer with the
CPU? The reference buffer is mapped for device access, and copying from it
without a prior sync might yield stale cache data.

> +	}
> +
> +	/*
> +	 * AVD writes nothing when DISABLE_FRAME_END_UPDATE_CDF is
> +	 * enabled
> +	 */
> +	memcpy(vb2_plane_vaddr(&dst->base.vb.vb2_buf, 0) +
> +		       AVD_AV1_CDFS_OFFSET(dst->base.vb.vb2_buf.planes[0].length,
> +					   dst->av1.color_size),
> +	       av1_ctx->bufs.probs.cpu, sizeof(struct avd_av1_cdfs));

[Severity: Critical]
Can the av1.color_size calculation lead to an integer underflow regression
when passed to AVD_AV1_CDFS_OFFSET?

If userspace provides huge dimensions, avd_color_size might return an
enormous value. Subtracting it from the plane length could underflow,
leading to a massive positive offset that causes memcpy to overwrite
memory far outside the mapped buffer.

[ ... ]
> +static int avd_av1_run_preamble(struct avd_ctx *ctx, struct avd_av1_run *run)
> +{
[ ... ]
> +	dst_len = run->base.bufs.dst->vb2_buf.planes[0].length;
> +	tlb_len = avd_color_size(run->frame->frame_width_minus_1 + 1,
> +				 run->frame->frame_height_minus_1 + 1);
> +
> +	run->addresses.color =
> +		run->base.y_out + AVD_AV1_COLOR_OFFSET(dst_len, tlb_len);
> +
> +	run->addresses.probs_out =
> +		run->base.y_out + AVD_AV1_CDFS_OFFSET(dst_len, tlb_len);

[Severity: High]
Does this offset logic require dst_len to be 256-byte aligned? If an
unaligned length is provided, the resulting DMA pointer may become
unaligned. When passed to pusha() later, the unaligned bits might be
silently truncated, causing a regression where hardware writes to a
shifted boundary.

[ ... ]
> +static int avd_av1_alloc_work_bufs(struct avd_ctx *ctx, struct avd_av1_run *run)
> +{
> +	struct avd_dev *avd = ctx->dev;
> +	struct avd_av1_ctx *av1_ctx = ctx->priv;
> +	const struct v4l2_ctrl_av1_frame *frame = run->frame;
> +	const struct v4l2_ctrl_av1_sequence *seq = run->seq;
> +	const struct v4l2_av1_tile_info *tile_info = &frame->tile_info;
> +	int ret, max_sb_col = 0, max_sb_row = 0, i, sb_cols1 = 0, sb_cols2 = 0,
> +		 sb;
> +	int sb_shift =
> +		seq->flags & V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK ? 1 :
> +									     0;
> +	int w = frame->frame_width_minus_1 + 1;
> +	int h = frame->frame_height_minus_1 + 1;
> +	int bit_depth = seq->bit_depth;
> +
> +	for (i = 0; i < tile_info->tile_cols; i++) {
> +		sb = tile_info->width_in_sbs_minus_1[i] + 1;
> +		max_sb_col = sb > max_sb_col ? sb : max_sb_col;
> +		sb_cols1 += ALIGN((sb << sb_shift) * 44, 128);
> +		/* wrong alignment? */
> +		sb_cols2 += ALIGN((sb << sb_shift) * 12, 128);
> +	}

[Severity: High]
Could this loop read past the end of the width_in_sbs_minus_1 array? This
array has 64 elements, but tile_cols can iterate up to 255 if unvalidated.

Additionally, does the size calculation overflow? The operation
(sb << sb_shift) * 44 uses signed 32-bit arithmetic, which could wrap
around with extremely large values of sb, resulting in a tiny allocation
and an out-of-bounds DMA write regression.

[ ... ]
> +static int avd_av1_run(struct avd_ctx *ctx)
> +{
> +	struct avd_av1_run run;
> +	struct avd_decoded_buffer *dst;
> +	int ret;
> +
> +	ret = avd_av1_run_preamble(ctx, &run);
> +	if (ret) {
> +		avd_run_postamble(ctx, &run.base);
> +		return ret;
> +	}
> +
> +	ret = avd_init_job(ctx, AVD_CODEC_AV1,
> +			   run.frame->tile_info.tile_cols *
> +					   run.frame->tile_info.tile_rows +
> +				   1);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If avd_init_job() fails, does the context need to be cleaned up?
avd_av1_run_preamble() was successfully called right before this, so
it looks like a regression where avd_run_postamble() is missing to
release the preamble state before returning.

> +
> +	dst = vb2_to_avd_decoded_buf(&run.base.bufs.dst->vb2_buf);
> +	update_dec_buf_info(dst, run.seq, run.frame);
> +
> +	avd_av1_set_prob(ctx, &run);
> +	avd_av1_alloc_work_bufs(ctx, &run);
> +
> +	set_header(ctx, &run);

[Severity: High]
What happens if avd_av1_alloc_work_bufs() fails? The function returns an
error code on allocation failure, but it appears to be ignored here. If it
fails, the hardware might be commanded to use unallocated (zeroed) DMA
addresses, causing a regression.

[ ... ]
> +static void avd_av1_stop(struct avd_ctx *ctx)
> +{
> +	struct avd_av1_ctx *av1_ctx = ctx->priv;
> +	struct avd_dev *avd = ctx->dev;
> +
> +	avd_buf_free(avd, &av1_ctx->bufs.rf_above_info);
> +	avd_buf_free(avd, &av1_ctx->bufs.az_above);
> +	avd_buf_free(avd, &av1_ctx->bufs.ip_above);
> +	avd_buf_free(avd, &av1_ctx->bufs.lf_above);
> +	avd_buf_free(avd, &av1_ctx->bufs.lf_above_info);
> +	avd_buf_free(avd, &av1_ctx->bufs.lf_left);

[Severity: Medium]
Should av1_ctx->bufs.above_info be freed here? It seems this buffer is
allocated in avd_av1_alloc_work_bufs but is missing from this teardown
sequence, resulting in a memory leak regression.

[ ... ]
> +static void avd_av1_submit(struct avd_ctx *ctx)
> +{
> +	struct avd_av1_ctx *av1_ctx = ctx->priv;
> +	struct avd_dev *avd = ctx->dev;
> +
> +	writel(AVD_OP_EXEC |
> +		       AVD_OP_EXEC_FLAG_START_REV4(avd->variant->revision ==
> +						   4) |
> +		       AVD_OP_EXEC_FIFO_IDX(ctx->fifo_idx) |
> +		       AVD_OP_EXEC_FIFO_MASK(avd->variant->fifo_slots),
> +	       avd->ctrl + avd->variant->submit_offset);
> +	for (int i = 0; i < av1_ctx->submit_num - 1; i++) {
> +		writel(AVD_OP_EXEC | AVD_OP_EXEC_FIFO_IDX(ctx->fifo_idx) |
> +			       AVD_OP_EXEC_FIFO_MASK(avd->variant->fifo_slots),
> +		       avd->ctrl + avd->variant->submit_offset);
> +	}

[Severity: High]
Could this tight loop overrun the hardware FIFO? The MMIO writes are
executed sequentially without polling or checking if the hardware can
accept them, which might cause command drops if submit_num exceeds the
available FIFO depth.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-avd-v1-0-49977931f455@icloud.com?part=14