[PATCH] accel/ethosu: reject DMA commands with uninitialized length

[PATCH] accel/ethosu: reject DMA commands with uninitialized length

[Muhammad Bilal]

cmd_state_init() initializes the command state with memset(0xff),
leaving dma->len at U64_MAX to signal missing setup. The only setter
is NPU_SET_DMA0_LEN; if userspace omits this command and issues
NPU_OP_DMA_START, dma->len remains U64_MAX.

In dma_length(), a positive stride added to U64_MAX wraps to a small
value. With size0 == 1, check_mul_overflow() does not trigger and
dma_length() returns 0 instead of U64_MAX. The caller's U64_MAX check
then passes, region_size[] stays 0, and the bounds check in
ethosu_job.c is bypassed, allowing hardware to execute DMA with stale
physical addresses.

Fix by checking for U64_MAX at the start of dma_length() before any
arithmetic, consistent with the sentinel value used throughout the
driver to detect uninitialized fields.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/accel/ethosu/ethosu_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 8e95539da98f..3401883e207f 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -164,6 +164,9 @@ static u64 dma_length(struct ethosu_validated_cmdstream_info *info,
 	s8 mode = dma_st->mode;
 	u64 len = dma->len;
 
+	if (len == U64_MAX)
+		return U64_MAX;
+
 	if (mode >= 1) {
 		if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len)
 			return U64_MAX;
-- 
2.53.0

Claude review: accel/ethosu: reject DMA commands with uninitialized length

[Claude Code Review Bot]

Overall Series Review

Subject: accel/ethosu: reject DMA commands with uninitialized length
Author: Muhammad Bilal <meatuni001@gmail.com>
Patches: 1
Reviewed: 2026-05-25T17:05:58.467707

---

This is a single-patch fix for the Arm Ethos-U NPU accelerator driver's command stream validation. The patch addresses a real issue: when `dma->len` is left at its sentinel value (`U64_MAX` from the `memset(0xff)` initialization) and the DMA mode involves stride arithmetic, integer wrapping can cause `dma_length()` to return a small value instead of `U64_MAX`, potentially bypassing bounds checks downstream in `ethosu_job.c`.

The fix is correct in principle — early-returning `U64_MAX` before any arithmetic prevents the wrapping. However, there are accuracy issues in the commit message, and the fix is incomplete since the `NPU_OP_DMA_START` handler doesn't actually validate the return value.

**Verdict: The fix is a valid hardening improvement but the commit message overstates the exploitability, and the DMA path has a deeper missing-validation issue that this patch does not address.**

---
Generated by Claude Code Patch Reviewer

Claude review: accel/ethosu: reject DMA commands with uninitialized length

[Claude Code Review Bot]

Patch Review

**The fix itself is correct and well-placed:**

```c
+	if (len == U64_MAX)
+		return U64_MAX;
```

Adding this check at line 166 (before the stride arithmetic at lines 167-172) prevents `U64_MAX` from wrapping through addition and multiplication, which is the right approach. It's consistent with the sentinel pattern used elsewhere in the driver (e.g., `feat_matrix_length()` checks `fm->base[tile] == U64_MAX` at line 213).

**Commit message inaccuracies:**

1. The message says *"The caller's U64_MAX check then passes"* — but looking at the `NPU_OP_DMA_START` handler (lines 395-403), there is **no** U64_MAX check on the return value of `dma_length()`. The values `srclen`/`dstlen` are computed and logged but never validated. The only downstream protection is the `region_size` bounds check in `ethosu_job.c:453`, which compares against the GEM buffer size.

2. The message says *"region_size[] stays 0"* — this is actually correct only when `dma->region` is `-1` (the `0xff` sentinel for `s8`), because the `dma->region >= 0` guard at line 174 prevents the `region_size` update. But in that case the DMA wouldn't reference any region at all. If userspace sets a valid `dma->region` but omits `NPU_SET_DMA0_LEN`, the wrapped small value *would* be written to `region_size`, which is the more interesting attack vector — and this patch does correctly fix that case.

**Incomplete fix — no validation at the DMA caller:**

The `NPU_OP_DMA_START` handler at line 395 does not check whether `dma_length()` returned `U64_MAX`. Compare with how `feat_matrix_length()` return values are checked in `calc_sizes()`. After this patch, `dma_length()` correctly returns `U64_MAX` for uninitialized lengths, but nothing in the DMA path actually acts on that sentinel. The `region_size` write is skipped (due to `region >= 0` guard), and execution continues normally. A more complete fix would add:

```c
case NPU_OP_DMA_START:
    srclen = dma_length(info, &st.dma, &st.dma.src);
    dstlen = dma_length(info, &st.dma, &st.dma.dst);
+   if (srclen == U64_MAX || dstlen == U64_MAX)
+       return -EINVAL;
```

**Minor style note:** The patch applies cleanly against the diff context shown, and the 3-line addition is minimal and non-invasive. The `Fixes:` tag and `Cc: stable` are appropriate if the maintainers agree this is a security-relevant fix.

**Summary:** The arithmetic hardening in `dma_length()` is correct and worth having, but the commit message's exploitation narrative doesn't fully match the code. The author should also consider adding explicit U64_MAX validation in the `NPU_OP_DMA_START` handler to make the defense complete, similar to how other operation handlers validate their length computations.

---
Generated by Claude Code Patch Reviewer