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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] media: avd: vp9: heap buffer overflow via unvalidated tile_rows_log2
- [High] media: avd: vp9: integer underflow in set_tiles payload size calculation
- [High] media: avd: vp9: out-of-bounds read in avd_vp9_done due to wrong sizeof
- [High] media: avd: vp9: missing arithmetic carry in 64-bit DMA address calculation
- [High] media: avd: vp9: submit_num integer overflow causes missing submissions
- [High] media: avd: vp9: DMA buffer overflow due to missing tile_cols_log2 limit
- [High] media: avd: vp9: TOCTOU race via lockless v4l2_ctrl_vp9_frame access
- [Medium] media: avd: vp9: memory leak on error paths in avd_vp9_alloc_bufs
--

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

media: apple: avd: add vp9 support

This commit adds hardware video decoding support for VP9 on Apple Silicon
devices via the Apple Video Decoder (AVD). It implements VP9-specific V4L2
controls and formats, along with state management and hardware command
submission.

> diff --git a/drivers/media/platform/apple/avd/avd-vp9.c b/drivers/media/platform/apple/avd/avd-vp9.c
> new file mode 100644
> index 0000000000000..76599a1a39811
> --- /dev/null
> +++ b/drivers/media/platform/apple/avd/avd-vp9.c
> @@ -0,0 +1,1072 @@
[ ... ]
> +struct avd_vp9_ctx {
> +	struct v4l2_vp9_frame_symbol_counts cnts;
> +	struct v4l2_vp9_frame_context probability_tables;
> +	struct v4l2_vp9_frame_context frame_context[4];
> +	struct avd_vp9_bufs {
[ ... ]
> +		struct avd_buf probs;
> +	} bufs;
> +	struct avd_vp9_frame_info cur;
> +	struct avd_vp9_frame_info last;
> +	u8 submit_num;

[Severity: High]
Since submit_num in struct avd_vp9_ctx is an 8-bit integer, will it overflow
if a frame contains up to 4096 tiles (e.g. 64 rows * 64 columns)?

> +};
[ ... ]
> +static void set_tiles(struct avd_ctx *ctx, struct avd_vp9_run *run)
> +{
> +	const struct v4l2_ctrl_vp9_frame *frame = run->decode_params;
> +	struct avd_vp9_ctx *vp9_ctx = ctx->priv;
> +	struct vb2_v4l2_buffer *src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> +	const u8 *data = vb2_plane_vaddr(&src->vb2_buf, 0);
> +
> +	u32 offset =
> +		frame->uncompressed_header_size + frame->compressed_header_size;
> +	u32 size = vb2_get_plane_payload(&src->vb2_buf, 0) - offset;

[Severity: High]
If offset is larger than the actual buffer payload, could size underflow
and become a very large 32-bit integer?

This would bypass the bounds check below:

	if (tile_size > size - 4)
		return;

and allow out-of-bounds kernel memory reads when reading the tile size.

> +	u32 num_tile_rows = 1 << frame->tile_rows_log2;
> +	u32 num_tile_cols = 1 << frame->tile_cols_log2;
> +	u32 tile_size;
[ ... ]
> +			if (row == num_tile_rows - 1 &&
> +			    col == num_tile_cols - 1) {
> +				tile_size = size;
> +			} else {
> +				tile_size = get_unaligned_be32(&data[offset]);
> +				/* i have crashed my computer because of this */
> +				if (tile_size > size - 4)
> +					return;
> +				offset += 4;
> +				size -= 4;
> +			}
> +			push(AVD_OP_CODED_DATA |
> +				     AVD_OP_CODED_DATA_ADDR(
> +					     run->base.coded_in >> 32),
> +			     "cm3_cmd_set_slice_data");
> +			push((u32)((run->base.coded_in + offset) & 0xffffffff),
> +			     "til_ab4_tile_addr_low");

[Severity: High]
Does this DMA address calculation drop the arithmetic carry? 

The upper 32 bits use run->base.coded_in >> 32, but the lower 32 bits are
(run->base.coded_in + offset) & 0xffffffff.

If adding offset causes the lower 32 bits to wrap, the hardware will receive
an incorrect 64-bit address for DMA reads.

> +			push(tile_size, "til_ab8_tile_size");
[ ... ]
> +static int avd_vp9_alloc_bufs(struct avd_ctx *ctx)
> +{
> +	struct avd_dev *avd = ctx->dev;
> +	struct avd_vp9_ctx *vp9_ctx = ctx->priv;
> +	int ret, w, h, bit_depth;
> +
> +	w = fmt_width(ctx);
> +	h = fmt_height(ctx);
> +	bit_depth = (ctx->image_fmt == AVD_IMG_FMT_420_10BIT ||
> +		     ctx->image_fmt == AVD_IMG_FMT_422_10BIT) ?
> +			    10 :
> +			    8;
> +
> +	ret = avd_buf_alloc(avd, &vp9_ctx->bufs.probs,
> +			    sizeof(struct avd_vp9_probs));
> +	if (ret)
> +		return ret;

[Severity: Medium]
If any allocation fails here and returns early, doesn't this leak the already
allocated DMA buffers?

The error path in avd_vp9_start() only frees vp9_ctx without calling
avd_buf_free() on the successful allocations.

[ ... ]
> +	ret = avd_buf_alloc(avd, &vp9_ctx->bufs.ip_above,
> +			    DIV_ROUND_UP(w, 16) * 4 * bit_depth +
> +				    (VP9_MAX_TILE_COLS - 1) * 128);

[Severity: High]
Since ip_above is sized based on VP9_MAX_TILE_COLS (which is hardcoded to 16),
what happens if userspace requests a frame with up to 64 tile columns? 

Without bounds checking on tile_cols_log2 in validate_dec_params(), the
hardware could overwrite past the end of this DMA buffer.

> +	if (ret)
> +		return ret;
[ ... ]
> +static int validate_dec_params(struct avd_ctx *ctx,
> +			       const struct v4l2_ctrl_vp9_frame *dec_params)
> +{
> +	unsigned int aligned_width, aligned_height;
> +
> +	if (dec_params->bit_depth > 10)
> +		/* not implemented */
> +		return -EINVAL;
> +
> +	if (dec_params->profile == 1 || dec_params->profile > 2)
> +		return -EINVAL;
> +
> +	if (dec_params->frame_height_minus_1 + 1 < 64 ||
> +	    dec_params->frame_width_minus_1 + 1 < 64)
> +		return -EINVAL;

[Severity: High]
Should there be a check here to ensure dec_params->tile_cols_log2 <= 4 to
prevent the hardware from exceeding the statically sized buffers based on
VP9_MAX_TILE_COLS?

[ ... ]
> +static int avd_vp9_run_preamble(struct avd_ctx *ctx, struct avd_vp9_run *run)
> +{
> +	struct v4l2_ctrl *ctrl;
> +	const struct v4l2_ctrl_vp9_frame *dec_params;
> +	struct avd_vp9_ctx *vp9_ctx = ctx->priv;
> +	unsigned int fctx_idx;
> +	int ret;
> +
> +	avd_run_preamble(ctx, &run->base);
> +
> +	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
> +			      V4L2_CID_STATELESS_VP9_COMPRESSED_HDR);
> +	if (WARN_ON(!ctrl))
> +		return -EINVAL;
> +	run->prob_updates = ctrl->p_cur.p;
> +
> +	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl, V4L2_CID_STATELESS_VP9_FRAME);
> +	if (WARN_ON(!ctrl))
> +		return -EINVAL;
> +	dec_params = ctrl->p_cur.p;
> +
> +	ret = validate_dec_params(ctx, dec_params);
> +	if (ret)
> +		return ret;
> +
> +	run->decode_params = dec_params;

[Severity: High]
Is it safe to read ctrl->p_cur.p locklessly without v4l2_ctrl_lock()? 

A concurrent VIDIOC_S_EXT_CTRLS ioctl could potentially modify dec_params
fields (like tile_cols_log2) after they pass validate_dec_params(), resulting
in a Time-of-Check to Time-of-Use race condition.

> +
> +	vp9_ctx->cur.tx_mode = run->prob_updates->tx_mode;
[ ... ]
> +static int avd_vp9_run(struct avd_ctx *ctx)
> +{
> +	struct avd_vp9_run run;
> +	struct avd_vp9_ctx *vp9_ctx;
> +	struct avd_decoded_buffer *dst;
> +	int ret;
> +
> +	ret = avd_vp9_run_preamble(ctx, &run);
> +	if (ret) {
> +		avd_run_postamble(ctx, &run.base);
> +		return ret;
> +	}
> +
> +	ret = avd_init_job(
> +		ctx, AVD_CODEC_VP9,
> +		(1 << run.decode_params->tile_rows_log2) *
> +				(1 << run.decode_params->tile_cols_log2) +
> +			1);

[Severity: Critical]
Can this multiplication overflow a signed 32-bit integer? 

Because tile_rows_log2 and tile_cols_log2 are an unvalidated u8, setting them
to large values (e.g. 255) could cause the multiplication to wrap around to 0,
leaving avd_init_job() to allocate a drastically undersized buffer.

Then, during the loop in set_tiles():

	for (int row = 0; row < num_tile_rows; row++)
		for (int col = 0; col < num_tile_cols; col++) {
			ctx->job.num++;

The iteration count would far exceed the allocation, and push() would write
out-of-bounds on the heap.

> +	if (ret)
> +		return ret;
[ ... ]
> +static void avd_vp9_done(struct avd_ctx *ctx, struct vb2_v4l2_buffer *src_buf,
> +			 struct vb2_v4l2_buffer *dst_buf,
> +			 enum vb2_buffer_state result)
> +{
[ ... ]
> +			cnts = vp9_ctx->bufs.counts.cpu;
> +
> +			for (i = 0; i < ARRAY_SIZE(cnts->tx16p); ++i)
> +				memcpy(tx16p[i], cnts->tx16p[i],
> +				       sizeof(tx16p[0]));

[Severity: High]
Does this read out of bounds? 

tx16p is u32[2][4], so sizeof(tx16p[0]) is 16 bytes. But the source array
cnts->tx16p[i] in struct avd_vp9_frame_symbol_counts is sized u32[3] (12
bytes), meaning this will over-read the source buffer.

> +
> +			for (i = 0; i < 2; i++) {
[ ... ]
> +static void avd_vp9_submit(struct avd_ctx *ctx)
> +{
> +	struct avd_vp9_ctx *vp9_ctx = ctx->priv;
> +	struct avd_dev *avd = ctx->dev;
> +	u32 submit_mask = ctx->dev->variant->revision == 3 ?
> +				  AVD_OP_EXEC_REV3_VP9_MASK :
> +				  0;
> +
> +	writel(AVD_OP_EXEC | submit_mask |
> +		       AVD_OP_EXEC_FLAG_START_REV4(avd->variant->revision ==
> +						   4) |
> +		       AVD_OP_EXEC_FLAG_START_REV3(avd->variant->revision ==
> +						   3) |
> +		       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 < vp9_ctx->submit_num - 1; i++)

[Severity: High]
If submit_num wrapped to 0 due to an overflow from a large number of tiles,
would 0 - 1 promote to a signed -1 (int), causing the i < -1 loop to never
execute and dropping the necessary hardware submissions?

> +		writel(AVD_OP_EXEC | submit_mask |
> +			       AVD_OP_EXEC_FIFO_IDX(ctx->fifo_idx) |
> +			       AVD_OP_EXEC_FIFO_MASK(avd->variant->fifo_slots),
> +		       avd->ctrl + avd->variant->submit_offset);
> +}

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