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

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

[Doruk Tan Ozturk]

The command-stream verifier records region_size[], the maximum byte
offset the NPU will touch in each buffer object, and the submit path
rejects a job whose region_size exceeds the bound BO
(region_size[i] > gem->size). feat_matrix_length() computed that maximum
by evaluating the touched address at a single presumed-worst corner
(x = width, y = height, c = depth). That is the true maximum only when
the address is monotonic in x/y/c and the feature matrix occupies a
single tile. Neither always holds:

 - Storage modes 0 and 1 split the feature matrix into up to four tiles,
   each with an independent, user-supplied base (base[0..3]) into the
   same region. feat_matrix_length() is evaluated once, so only the tile
   containing the (max x, max y) corner is bounded; a large offset placed
   in any other tile base is never accounted for.

 - In the nhcwb16 packed format the channel term
   (c / 16) * stride_c + (c & 0xf) * element_size is not monotonic in c
   when stride_c is small, so the largest offset can lie at an interior
   channel rather than at c = depth.

An unprivileged process able to open the accelerator node can therefore
craft a cmdstream whose real maximum output offset exceeds the computed
region_size, pass the submit check with an undersized output buffer, and
have the NPU DMA-write past the end of that buffer (up to ~1 MiB out of
bounds in the tiling case).

Evaluate the extent at every corner that can bound the populated tiles
and at the packed-channel peak, take the maximum, and reject the stream
if any reachable tile base is left unset. Well-formed single-tile,
monotonic streams are unchanged: the computed size is identical.

Found by 0sec automated security-research tooling (https://0sec.ai).
Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Assisted-by: 0sec:multi-model
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
 drivers/accel/ethosu/ethosu_gem.c | 73 +++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 8 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 3401883e207f..dfc499d7fb0c 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;
 }
 
-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)
 {
 	u32 element_size, storage = fm->precision >> 14;
 	int tile = 0;
 	u64 addr;
 
-	if (fm->region < 0)
-		return U64_MAX;
-
 	switch (storage) {
 	case 0:
 		if (x >= fm->width0 + 1) {
@@ -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;
 }
 
+/*
+ * Bound the region a feature matrix touches over the coordinate box
+ * [0, x] x [0, y] x [0, c].
+ *
+ * The address is separable and, as the strides are unsigned 40-bit quantities,
+ * non-decreasing in x and y, so their maxima sit at the box edge and, for the
+ * tiled storage modes, just below each split (width0, height[]) where an
+ * independent tile base takes over. Only nhcwb16 packing is non-monotonic in c:
+ * crossing a 16-lane group adds stride_c but resets the in-group term, so the
+ * peak may sit at the top of the last full group instead of at c. Evaluating
+ * every such extremal corner bounds all up-to-four tiles, whereas a single
+ * presumed-max corner undercounts the other tiles and the channel peak.
+ */
+static u64 feat_matrix_length(struct ethosu_validated_cmdstream_info *info,
+			      struct feat_matrix *fm,
+			      u32 x, u32 y, u32 c)
+{
+	u32 storage = fm->precision >> 14;
+	u32 xs[2], ys[3], cs[2];
+	int nx = 0, ny = 0, nc = 0, i, j, k;
+	u64 max_addr = 0;
+
+	if (fm->region < 0)
+		return U64_MAX;
+
+	xs[nx++] = x;
+	if (storage == 0)
+		xs[nx++] = min_t(u32, x, fm->width0);
+
+	ys[ny++] = y;
+	if (storage == 0 || storage == 1) {
+		ys[ny++] = min_t(u32, y, fm->height[0]);
+		ys[ny++] = min_t(u32, y, fm->height[1]);
+	}
+
+	cs[nc++] = c;
+	if (((fm->precision >> 6) & 0x3) == 1 && c >= 16)
+		cs[nc++] = (c & ~0xfu) - 1;
+
+	for (i = 0; i < nx; i++) {
+		for (j = 0; j < ny; j++) {
+			for (k = 0; k < nc; k++) {
+				u64 addr = feat_matrix_addr(fm, xs[i], ys[j], cs[k]);
+
+				if (addr == U64_MAX)
+					return U64_MAX;
+				max_addr = max(max_addr, addr);
+			}
+		}
+	}
+
+	info->region_size[fm->region] =
+		max(info->region_size[fm->region], max_addr + 1);
+
+	return max_addr;
+}
+
 static int calc_sizes(struct drm_device *ddev,
 		      struct ethosu_validated_cmdstream_info *info,
 		      u16 op, struct cmd_state *st,
-- 
2.43.0

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

[sashiko-bot]

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