[PATCH] accel/ethosu: fix integer overflow in dma_length()

[PATCH] accel/ethosu: fix integer overflow in dma_length()

[Muhammad Bilal]

dma_length() computes the total DMA transfer length as:

    len = ((len + stride[0]) * size0 + stride[1]) * size1

where len and stride[] are 64-bit values derived from user-supplied
40-bit command stream fields, and size0/size1 are user-supplied u16
values.

The final multiplication by size1 (up to 65535) on an intermediate
result that can already be ~2^55 easily exceeds 2^64, wrapping the
u64 result to a small positive value.

This wrapped value is then stored in info->region_size[] and compared
against gem->size in ethosu_job.c:

    if (cmd_info->region_size[i] > gem->size)
        return -EOVERFLOW;

A userspace caller can craft stride and size values so that the
calculated length wraps to zero or a small value, passing this check
while the hardware executes a DMA transfer with the original large
strides, accessing memory far outside the GEM buffer.

Fix by replacing the unchecked multiplications with
check_mul_overflow(), returning U64_MAX on overflow. The callers
of dma_length() already treat U64_MAX as an error sentinel.

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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 5a02285a4986..1f132611a6ce 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -2,6 +2,7 @@
 /* Copyright 2025 Arm, Ltd. */
 
 #include <linux/err.h>
+#include <linux/overflow.h>
 #include <linux/slab.h>
 
 #include <drm/ethosu_accel.h>
@@ -165,11 +166,13 @@ static u64 dma_length(struct ethosu_validated_cmdstream_info *info,
 
 	if (mode >= 1) {
 		len += dma->stride[0];
-		len *= dma_st->size0;
+		if (check_mul_overflow(len, (u64)dma_st->size0, &len))
+			return U64_MAX;
 	}
 	if (mode == 2) {
 		len += dma->stride[1];
-		len *= dma_st->size1;
+		if (check_mul_overflow(len, (u64)dma_st->size1, &len))
+			return U64_MAX;
 	}
 	if (dma->region >= 0)
 		info->region_size[dma->region] = max(info->region_size[dma->region],
-- 
2.53.0

[PATCH v2] accel/ethosu: fix integer overflow and underflow in dma_length()

[Muhammad Bilal]

dma_length() computes the total DMA transfer length as:

    len = ((len + stride[0]) * size0 + stride[1]) * size1

where len and stride[] are 64-bit values derived from user-supplied
40-bit command stream fields, and size0/size1 are user-supplied u16
values.

Two bugs exist:

1. Integer overflow: the final multiplication by size1 (up to 65535)
   on an intermediate result that can already be ~2^55 easily exceeds
   2^64, wrapping the u64 result to a small positive value. This
   wrapped value passes the region_size[i] <= gem->size check in
   ethosu_job.c while the hardware executes DMA with the original
   large strides, accessing memory outside the GEM buffer.

2. Negative stride underflow: stride[0] and stride[1] are signed
   64-bit values sign-extended from 40-bit user input, and can be
   negative. Adding a large negative stride to a small u64 len wraps
   to a huge value. With size0 or size1 == 1, check_mul_overflow()
   does not trigger, and len + offset can wrap back to a small value,
   bypassing the bounds check while the hardware accesses memory below
   the GEM buffer base.

3. Missing caller check: dma_length() returned U64_MAX on error but
   the caller only used the result for dev_dbg(), never checking for
   U64_MAX. This left info->region_size[] at 0, causing ethosu_job.c
   to skip the region entirely and allow hardware to run with stale
   physical addresses.

Fix by adding underflow checks before each stride addition, replacing
the unchecked multiplications with check_mul_overflow(), and adding
a U64_MAX check in the caller that returns -EINVAL.

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 | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 5a02285a4986..0383b7a6c3d3 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -2,6 +2,7 @@
 /* Copyright 2025 Arm, Ltd. */
 
 #include <linux/err.h>
+#include <linux/overflow.h>
 #include <linux/slab.h>
 
 #include <drm/ethosu_accel.h>
@@ -164,12 +165,18 @@ static u64 dma_length(struct ethosu_validated_cmdstream_info *info,
 	u64 len = dma->len;
 
 	if (mode >= 1) {
+		if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len)
+			return U64_MAX;
 		len += dma->stride[0];
-		len *= dma_st->size0;
+		if (check_mul_overflow(len, (u64)dma_st->size0, &len))
+			return U64_MAX;
 	}
 	if (mode == 2) {
+		if (dma->stride[1] < 0 && (u64)(-dma->stride[1]) > len)
+			return U64_MAX;
 		len += dma->stride[1];
-		len *= dma_st->size1;
+		if (check_mul_overflow(len, (u64)dma_st->size1, &len))
+			return U64_MAX;
 	}
 	if (dma->region >= 0)
 		info->region_size[dma->region] = max(info->region_size[dma->region],
@@ -397,6 +404,8 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 		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;
 
 			if (st.dma.dst.region >= 0)
 				info->output_region[st.dma.dst.region] = true;
-- 
2.53.0

[PATCH v3] accel/ethosu: fix arithmetic issues in dma_length()

[Muhammad Bilal]

dma_length() derives DMA region usage from command stream values and
updates region_size[]:

    len = ((len + stride[0]) * size0 + stride[1]) * size1
    region_size[region] = max(..., len + dma->offset)

Several arithmetic issues can corrupt the derived region size:

- signed stride values may underflow when added to len
- intermediate multiplications may overflow
- len + dma->offset may overflow during region_size updates
- dma_length() error returns were not validated by the caller

region_size[] is later used by ethosu_job.c to validate command stream
accesses against GEM buffer sizes. Arithmetic wraparound can therefore
under-report region usage and bypass the bounds validation.

Fix by validating signed additions, using overflow helpers for
multiplications and offset updates, and propagating dma_length()
failures to the caller.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---

v3:
- add check_add_overflow() for len + dma->offset
- validate dma_length() return value in caller
- rework commit message to avoid unproven claims

v2:
- add negative stride underflow checks before each addition
- replace unchecked multiplications with check_mul_overflow()
 drivers/accel/ethosu/ethosu_gem.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 5a02285a4986..8e95539da98f 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -2,6 +2,7 @@
 /* Copyright 2025 Arm, Ltd. */
 
 #include <linux/err.h>
+#include <linux/overflow.h>
 #include <linux/slab.h>
 
 #include <drm/ethosu_accel.h>
@@ -164,16 +165,26 @@ static u64 dma_length(struct ethosu_validated_cmdstream_info *info,
 	u64 len = dma->len;
 
 	if (mode >= 1) {
+		if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len)
+			return U64_MAX;
 		len += dma->stride[0];
-		len *= dma_st->size0;
+		if (check_mul_overflow(len, (u64)dma_st->size0, &len))
+			return U64_MAX;
 	}
 	if (mode == 2) {
+		if (dma->stride[1] < 0 && (u64)(-dma->stride[1]) > len)
+			return U64_MAX;
 		len += dma->stride[1];
-		len *= dma_st->size1;
+		if (check_mul_overflow(len, (u64)dma_st->size1, &len))
+			return U64_MAX;
+	}
+	if (dma->region >= 0) {
+		u64 end;
+
+		if (check_add_overflow(len, dma->offset, &end))
+			return U64_MAX;
+		info->region_size[dma->region] = max(info->region_size[dma->region], end);
 	}
-	if (dma->region >= 0)
-		info->region_size[dma->region] = max(info->region_size[dma->region],
-						     len + dma->offset);
 
 	return len;
 }
@@ -397,6 +408,8 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 		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;
 
 			if (st.dma.dst.region >= 0)
 				info->output_region[st.dma.dst.region] = true;
-- 
2.53.0

Claude review: accel/ethosu: fix arithmetic issues in dma_length()

[Claude Code Review Bot]

Patch Review

This is the final version and the one that should be applied. It is a **well-constructed fix** that addresses all identified issues.

**Changes reviewed:**

1. **Negative stride underflow checks** (correct):
```c
+		if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len)
+			return U64_MAX;
 		len += dma->stride[0];
```
This prevents `len` (u64) from wrapping when a negative stride is added. The `(u64)(-dma->stride[0])` cast is safe: strides are sign-extended from 40 bits, so `S64_MIN` cannot occur, and the kernel's `-fwrapv` semantics make the negation well-defined regardless.

2. **Multiplication overflow checks** (correct):
```c
+		if (check_mul_overflow(len, (u64)dma_st->size0, &len))
+			return U64_MAX;
```
Standard kernel pattern using `<linux/overflow.h>`. The `(u64)` cast on the `u16` operand ensures both operands match the result type for `check_mul_overflow`.

3. **`len + offset` overflow check** (correct, new in v3):
```c
+	if (dma->region >= 0) {
+		u64 end;
+
+		if (check_add_overflow(len, dma->offset, &end))
+			return U64_MAX;
+		info->region_size[dma->region] = max(info->region_size[dma->region], end);
+	}
```
Both `len` and `dma->offset` are `u64`, so `check_add_overflow` correctly detects wrapping. The local `end` variable avoids writing a potentially corrupted value into `region_size[]`.

4. **Caller validation** (correct):
```c
 			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;
```
This ensures that when `dma_length()` fails, the command stream is rejected rather than silently continuing with stale `region_size` values.

**Minor observations:**

- **Positive stride overflow is not checked** before `len += dma->stride[N]` when stride is positive. Given the 40-bit input constraint on both `len` and `stride`, the sum cannot exceed ~2^41 before the first multiply, and after the first multiply the maximum intermediate value (~2^57) plus a 40-bit stride still cannot overflow u64. So this is safe in practice, but a comment or a `check_add_overflow` would make the reasoning explicit.

- **Commit message quality** is good in v3 — it avoids making unproven security claims and factually describes the arithmetic hazards. The changelog is clean.

- **Fixes tag and stable CC** are appropriate since this is a security-relevant fix for a driver introduced in 5a5e9c0228e6.

**Verdict:** v3 looks correct and ready to apply. The identified similar issues in `feat_matrix_length()` and `calc_sizes()` (unchecked `addr + 1`, `base + length` additions) are pre-existing and out of scope for this patch, but would benefit from a follow-up.

---
Generated by Claude Code Patch Reviewer

Claude review: accel/ethosu: fix integer overflow and underflow in dma_length()

[Claude Code Review Bot]

Patch Review

This is v2 and is **superseded by v3**. It fixes the multiplication overflow, adds negative stride underflow checks, and adds the caller check.

**Improvements over v1:**

- Adds underflow guard: `if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len) return U64_MAX;`
- Adds caller validation: `if (srclen == U64_MAX || dstlen == U64_MAX) return -EINVAL;`

**Remaining issue:**

- Still missing `check_add_overflow` for the `len + dma->offset` computation in the `region_size` update. A crafted `offset` could wrap `len + dma->offset` to a small value, causing `region_size[]` to underreport the region usage.

**Minor nit:** The commit message lists "Two bugs exist:" but then enumerates items 1, 2, and 3. Should say "three issues" or similar.

---
Generated by Claude Code Patch Reviewer

Claude review: accel/ethosu: fix integer overflow in dma_length()

[Claude Code Review Bot]

Overall Series Review

Subject: accel/ethosu: fix integer overflow in dma_length()
Author: Muhammad Bilal <meatuni001@gmail.com>
Patches: 3
Reviewed: 2026-05-25T17:08:43.686509

---

This mbox contains three versions (v1, v2, v3) of the same patch fixing arithmetic issues in `dma_length()` in the Arm Ethos-U NPU accelerator driver. Only v3 should be considered for merging — v1 and v2 are superseded.

The core issue is real and security-relevant: user-supplied command stream values feed into unchecked arithmetic that computes `region_size[]`, which is later used as a bounds check against GEM buffer sizes. Integer wraparound can underreport the true DMA region usage, allowing the NPU hardware to access memory outside the allocated GEM buffer.

The v3 patch correctly addresses four arithmetic hazards: negative stride underflow, multiplication overflow, `len + offset` overflow, and the missing caller validation of the `U64_MAX` error sentinel. The fix is well-scoped and uses the standard kernel overflow helpers (`check_mul_overflow`, `check_add_overflow`).

One observation for a possible follow-up: the same `addr + 1` addition at `ethosu_gem.c:240` in `feat_matrix_length()`, and the `base + length` additions at lines 291–293 and 303–305 in `calc_sizes()`, have no overflow protection either. These are analogous to the `len + offset` overflow that v3 fixed in `dma_length()`.

---

---
Generated by Claude Code Patch Reviewer

Claude review: accel/ethosu: fix integer overflow in dma_length()

[Claude Code Review Bot]

Patch Review

This is v1 and is **superseded by v3**. It only addresses multiplication overflow:

```c
-		len *= dma_st->size0;
+		if (check_mul_overflow(len, (u64)dma_st->size0, &len))
+			return U64_MAX;
```

**Issues in v1:**

1. **Missing negative stride underflow check.** Before the multiplication, `len += dma->stride[0]` adds a signed `s64` to an unsigned `u64`. If `stride[0]` is a large negative value (sign-extended from 40-bit input), this wraps `len` to a huge value. With `size0 == 1`, `check_mul_overflow` does not trigger, and the result is a garbage length.

2. **Missing caller validation.** The commit message claims "the callers of dma_length() already treat U64_MAX as an error sentinel," but looking at the caller at the original line 397–400 (now line 407), `srclen`/`dstlen` were only used in a `dev_dbg()` format string — the return value was never checked for `U64_MAX`. This means even when `dma_length()` returns `U64_MAX`, the `info->region_size[]` entry would remain at 0 (never updated because the function returned early), and the region would be skipped entirely by `ethosu_job.c` at line 420/670, allowing stale physical addresses through. This claim in the commit message is **incorrect** for the `NPU_OP_DMA_START` case.

3. **Missing `len + dma->offset` overflow check** at the `region_size` update.

---
Generated by Claude Code Patch Reviewer