Re: [PATCH v2 10/17] media: apple: avd: add av1 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] media: avd: av1: Unvalidated frame dimensions lead to integer overflows and OOB DMA access
- [High] media: avd: av1: AV1_LF_SHARPNESS bitmask overlaps with loop filter delta macros
- [High] media: avd: av1: Unvalidated ref_frame_idx causes OOB read from reference_frame_ts array
- [High] media: avd: av1: Missing tile_cols/tile_rows validation causes off-by-one OOB access and overflows
- [High] media: avd: av1: Hardware accesses physical address 0x0 for unallocated auxiliary buffers
- [High] media: avd: av1: NULL pointer dereference from unchecked vb2_plane_vaddr()
- [Medium] media: avd: av1: Fallback logic copies uninitialized CDF memory from the destination buffer
- [Medium] media: avd: av1: Incorrect global motion shear calculation rejects valid negative alpha parameters
- [Low] media: avd: av1: Lockless read of active V4L2 controls risks torn parameters
--

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

media: apple: avd: add av1 support

This commit adds AV1 decoding support to the Apple Video Decoder (AVD)
driver. It includes entropy mode helpers, V4L2 format definitions, and
the main decoding flow for AV1 frames.

> 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..357dd1d0eca5c
> --- /dev/null
> +++ b/drivers/media/platform/apple/avd/avd-av1.c
> @@ -0,0 +1,1432 @@
[ ... ]
> +#define AV1_LF_DELTA_ENABLED(v)	FIELD_PREP(BIT(27), !!(v))
> +#define AV1_LF_DELTA_MULTI(v)	FIELD_PREP(BIT(28), !!(v))
> +#define AV1_LF_DELTA_PRESENT(v)	FIELD_PREP(BIT(31), !!(v))
> +#define AV1_LF_DELTA_RES(v)	FIELD_PREP(GENMASK(30, 29), v)
> +#define AV1_LF_LV3(v)		FIELD_PREP(GENMASK(5, 0), v)
> +#define AV1_LF_LV2(v)		FIELD_PREP(GENMASK(11, 6), v)
> +#define AV1_LF_LV1(v)		FIELD_PREP(GENMASK(17, 12), v)
> +#define AV1_LF_LV0(v)		FIELD_PREP(GENMASK(23, 18), v)
> +
> +#define AV1_LF_SHARPNESS(v)	FIELD_PREP(GENMASK(31, 28), v)

[Severity: High]
Does this bitmask overlap with the loop filter delta macros? It looks like
bits 28 through 31 are shared with AV1_LF_DELTA_MULTI, AV1_LF_DELTA_RES,
and AV1_LF_DELTA_PRESENT, which could cause the hardware to be misconfigured
when both are used in set_header().

[ ... ]
> +static void
> +avd_av1_dec_get_shear_params(const u32 *params, s64 *alpha,
> +			     s64 *beta, s64 *gamma, s64 *delta)
> +{
> +	const int *mat = params;
> +	short shift;
> +	short y;
> +	long long gv, dv;
> +
> +	if (mat[2] <= 0)
> +		return;

[Severity: Medium]
Will this improperly reject valid negative alpha parameters? According to the
AV1 specification, the transform calculation should only abort if the alpha
coefficient (mat[2]) is exactly zero to avoid division by zero.

[ ... ]
> +static void set_ref_hdr(struct avd_ctx *ctx, struct avd_av1_run *run)
> +{
> +	const struct v4l2_ctrl_av1_frame *frame = run->frame;
> +	u8 ref_slots[7] = { 1, 2, 3, 4, 5, 6, 7 };
> +	int sign_bias = 0;
> +	int ref_slot = 0;
> +	int i, j;
> +
> +	/* point to the first index where the frames equal */
> +	for (i = 0; i < 6; i++) {
> +		for (j = i + 1; j < 7; j++) {
> +			if (frame->reference_frame_ts[frame->ref_frame_idx[i]] ==

[Severity: High]
Can an unvalidated ref_frame_idx from userspace cause an out-of-bounds read
here? The V4L2 framework does not natively bounds-check ref_frame_idx against
V4L2_AV1_TOTAL_REFS_PER_FRAME (8), which might allow a payload to index out of
the reference_frame_ts array.

[ ... ]
> +/* 7.16. Upscaling process */
> +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: Critical]
Could a maliciously crafted frame with an upscaled_width of 0 result in a
division by zero panic here? There doesn't appear to be a prior non-zero
check on the upscaled dimensions.

[ ... ]
> +static void set_header(struct avd_ctx *ctx, struct avd_av1_run *run)
> +{
[ ... ]
> +	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: High]
Is it possible for the hardware to access physical address 0x0 here?
The bufs.seg buffer is only allocated in avd_av1_alloc_work_bufs() if
V4L2_AV1_SEGMENTATION_FLAG_UPDATE_MAP is set. If segmentation is enabled
but UPDATE_MAP is not set, this unconditionally pushes a 0 address to the
hardware, which might cause an IOMMU fault.

[ ... ]
> +static int set_tiles(struct avd_ctx *ctx, struct avd_av1_run *run)
> +{
> +	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;
> +	dma_addr_t coded_in;
> +	int row, col, sb_row, sb_col, tile_id, tile_row, tile_col;
> +	int sb_shift =
> +		seq->flags & V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK ? 5 :
> +									     4;
> +
> +	for (row = 0; row < tile_info->tile_rows; row++) {
> +		for (col = 0; col < tile_info->tile_cols; col++) {
> +			tile_id = row * tile_info->tile_cols + col;
> +			tile_group = &run->tile_group[tile_id];
> +			coded_in = run->base.coded_in +
> +					      tile_group->tile_offset;
> +			tile_row = tile_group->tile_row;
> +			tile_col = tile_group->tile_col;
> +
> +			if (tile_col > V4L2_AV1_MAX_TILE_COLS ||
> +			    tile_row > V4L2_AV1_MAX_TILE_ROWS)
> +				return -EINVAL;

[Severity: High]
Does this strict greater-than check allow an off-by-one out-of-bounds access?
If tile_col is exactly V4L2_AV1_MAX_TILE_COLS (64), it bypasses this check and
would read exactly at the bounds of the width_in_sbs_minus_1 array below.

[ ... ]
> +			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_row]) |
> +				     AVD_SL_DIM_END_X(
> +					     sb_col +
> +					     tile_info->width_in_sbs_minus_1
> +						     [tile_col]),
> +			     "tile_op_end");

[ ... ]
> +static void avd_av1_set_prob(struct avd_ctx *ctx, struct avd_av1_run *run)
> +{
[ ... ]
> +	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_av1_get_ref_buf(ctx, run,
> +					  run->frame->primary_ref_frame);
> +		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: High]
Is it safe to use vb2_plane_vaddr() without checking for a NULL return? If
userspace provides a buffer via V4L2_MEMORY_DMABUF that cannot be mapped into
the kernel virtual address space, this will result in a NULL pointer
dereference.

[Severity: Medium]
If avd_av1_get_ref_buf() cannot find the reference and falls back to returning
the destination buffer (dst), does this copy uninitialized CDF memory from the
current uninitialized frame?

[Severity: Critical]
Because avd_color_size() lacks unvalidated dimension protection from
avd_av1_try_ctrl(), could malicious frame dimensions cause an integer overflow,
forcing the AVD_AV1_CDFS_OFFSET macro to underflow and generate an arbitrary
DMA offset? This would result in out-of-bounds kernel memory corruption during
the memcpy.

[ ... ]
> +static int avd_av1_run_preamble(struct avd_ctx *ctx, struct avd_av1_run *run)
> +{
> +	struct v4l2_ctrl *ctrl;
> +	int dst_len, tlb_len;
> +
> +	avd_run_preamble(ctx, &run->base);
> +
> +	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl, V4L2_CID_STATELESS_AV1_SEQUENCE);
> +	if (WARN_ON(!ctrl))
> +		return -EINVAL;
> +	run->seq = ctrl->p_cur.p;

[Severity: Low]
Could a lockless read of the active V4L2 control here lead to torn parameters
if userspace concurrently modifies the control via VIDIOC_S_EXT_CTRLS while the
decoder workqueue is running?

[ ... ]
> +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;
> +	u32 ret, max_sb_col = 0, max_sb_row = 0, i, sb_cols1 = 0, sb_cols2 = 0,
> +		 sb;
> +	u32 w = frame->frame_width_minus_1 + 1;
> +	u32 h = frame->frame_height_minus_1 + 1;
> +	u8 sb_shift =
> +		seq->flags & V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK ? 1 :
> +									     0;
> +	u8 bit_depth = seq->bit_depth;
> +
> +	for (i = 0; i < tile_info->tile_cols; i++) {
> +		sb = tile_info->width_in_sbs_minus_1[i] + 1;

[Severity: High]
Since tile_cols can be up to 256, does this read past the end of the 64-element
width_in_sbs_minus_1 array? It seems this could result in out-of-bounds reads
and drastically undersized work buffer allocations if it overflows.

[ ... ]
> +static int avd_av1_try_ctrl(struct avd_ctx *ctx, struct v4l2_ctrl *ctrl)
> +{
> +	if (ctrl->id == V4L2_CID_STATELESS_AV1_SEQUENCE)
> +		return avd_av1_validate_sequence(ctx,
> +						 ctrl->p_new.p_av1_sequence);
> +	/* width should also be > 8 */
> +	return 0;
> +}

[Severity: Critical]
Should we validate the AV1 frame dimensions against the negotiated queue
format here? Unvalidated dimensions lead to the integer overflows in
calc_upscale() and avd_color_size() mentioned earlier, resulting in
OOB DMA accesses.

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