Re: [PATCH] accel/ethosu: validate all tiles and the packed-channel peak in the cmdstream verifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing lower-bound tracking for negative DMA strides allows the NPU to access memory before the start of the buffer.
- [Critical] Missing feature matrix configuration parameters result in `-1` defaults that cause integer underflow in bounds checks, leading to out-of-bounds DMA.
--

--- Patch [1]: [PATCH] accel/ethosu: validate all tiles and the packed-channel peak in the cmdstream verifier ---
commit 272f98612406533d182249a7aefbd612bfb04a39
Author: Doruk Tan Ozturk <doruk@0sec.ai>
Subject: accel/ethosu: validate all tiles and the packed-channel peak in the cmdstream verifier

The patch addresses an issue where feat_matrix_length() computed the maximum
byte offset by evaluating a single presumed-worst corner. This allowed
unprivileged processes to craft command streams that bypassed submit checks
with undersized output buffers, leading to out-of-bounds DMA writes. The fix
evaluates the extent at every corner that can bound the populated tiles and
at the packed-channel peak.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 3401883e207fc..dfc499d7fb0c1 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c
> @@ -192,17 +192,19 @@ static u64 dma_length(struct ethosu_validated_cmdstream_info *info,
>  	return len;
>  }

[Severity: Critical]
This is a pre-existing issue, but is there a missing lower-bound check for
negative DMA strides in dma_length()?

When dma->stride[0] is negative, this function reduces the computed length,
which allows the upper-bound check (len + offset) to pass:

	if (mode >= 1) {
		if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len)
			return U64_MAX;
		len += dma->stride[0];
		...

However, there is no lower-bound validation to ensure the minimum accessed
address does not drop below 0. Could an unprivileged user submit a command
stream with a negative stride and an offset near 0, causing the hardware
DMA engine to drop below offset 0 and access memory preceding the GEM buffer?

>  
> -static u64 feat_matrix_length(struct ethosu_validated_cmdstream_info *info,
> -			      struct feat_matrix *fm,
> -			      u32 x, u32 y, u32 c)
> +/*
> + * Compute the byte offset the NPU addresses for one feature-matrix coordinate
> + * (x, y, c). Storage modes 0 and 1 route disjoint (x, y) sub-rectangles to up
> + * to four independent tile bases; the caller must only pass coordinates whose
> + * tiles it means to bound. Returns U64_MAX if the coordinate lands on a tile
> + * whose base was never set by the command stream.
> + */
> +static u64 feat_matrix_addr(struct feat_matrix *fm, u32 x, u32 y, u32 c)
>  {

[ ... ]

> @@ -240,11 +242,66 @@ static u64 feat_matrix_length(struct ethosu_validated_cmdstream_info *info,
>  		break;
>  	}
>  
> -	info->region_size[fm->region] = max(info->region_size[fm->region], addr + 1);
> -
>  	return addr;
>  }

[Severity: Critical]
This is a pre-existing issue, but does feat_matrix_addr() handle missing
configuration parameters correctly?

cmd_state_init() initializes cmd_state fields to 0xff, setting unprovided
s64 strides to -1. If a command stream omits a stride command, the stride
remains -1.

In feat_matrix_addr(), these -1 values act as subtractive strides:

	addr = fm->base[tile] + y * fm->stride_y;
	switch ((fm->precision >> 6) & 0x3) { // format
	case 0: //nhwc:
		addr += x * fm->stride_x + c;

If a user provides a base offset large enough to prevent unsigned wrap-around,
the maximum address evaluated in feat_matrix_length() simply remains the
initial base offset at coordinate (0,0,0), completely hiding the bounds of
the access.

If an attacker submits a job setting a large hardware stride, and a second
job omits the stride command, could the verifier use the subtractive -1 stride
to pass bounds checks while the hardware performs out-of-bounds DMA using the
retained large stride from the previous job?

>  
> +/*
> + * Bound the region a feature matrix touches over the coordinate box
> + * [0, x] x [0, y] x [0, c].

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720192044.19557-1-doruk@0sec.ai?part=1