From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: accel/ethosu: fix arithmetic issues in dma_length()
Date: Mon, 25 May 2026 17:08:44 +1000
Message-ID: <review-patch3-20260524103710.47397-1-meatuni001@gmail.com>
In-Reply-To: <20260524103710.47397-1-meatuni001@gmail.com>
References: <20260524051659.70654-1-meatuni001@gmail.com> <20260524103710.47397-1-meatuni001@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the final version and the one that should be applied. It is a **wel=
l-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 +=3D dma->stride[0];
```
This prevents `len` (u64) from wrapping when a negative stride is added. Th=
e `(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_o=
verflow`.

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

4. **Caller validation** (correct):
```c
 			srclen =3D dma_length(info, &st.dma, &st.dma.src);
 			dstlen =3D dma_length(info, &st.dma, &st.dma.dst);
+			if (srclen =3D=3D U64_MAX || dstlen =3D=3D 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 +=3D 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-b=
it stride still cannot overflow u64. So this is safe in practice, but a com=
ment or a `check_add_overflow` would make the reasoning explicit.

- **Commit message quality** is good in v3 =E2=80=94 it avoids making unpro=
ven security claims and factually describes the arithmetic hazards. The cha=
ngelog is clean.

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

**Verdict:** v3 looks correct and ready to apply. The identified similar is=
sues in `feat_matrix_length()` and `calc_sizes()` (unchecked `addr + 1`, `b=
ase + 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