[PATCH] accel/ethosu: fix OOB write in ethosu_gem_cmdstream_copy_and_validate()

[PATCH] accel/ethosu: fix OOB write in ethosu_gem_cmdstream_copy_and_validate()

[Muhammad Bilal]

The command stream parsing loop increments the index variable a second
time when a 64-bit command word is encountered (bit 14 set), but does
not re-check the loop bound before writing the second word:

    for (i = 0; i < size / 4; i++) {
        bocmds[i] = cmds[0];
        if (cmd & 0x4000) {
            i++;
            bocmds[i] = cmds[1];   /* unchecked */
        }
    }

The buffer bocmds is backed by a DMA allocation of exactly size bytes
from drm_gem_dma_create(ddev, size), giving valid indices [0, size/4-1].

When i == size/4 - 1 on entry to an iteration and bit 14 of cmds[0] is
set, bocmds[size/4-1] is written in bounds, i is then incremented to
size/4, and bocmds[size/4] writes four bytes past the end of the
allocation.

Userspace controls both the buffer contents and the size argument via
the ioctl, making this a userspace-triggerable heap out-of-bounds write.

Fix by checking the incremented index against the buffer bound before
the second write and returning -EINVAL if the buffer is too small to
contain the extended command.

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

diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
index 7994e7073903..f526f4aedffd 100644
--- a/drivers/accel/ethosu/ethosu_gem.c
+++ b/drivers/accel/ethosu/ethosu_gem.c
@@ -387,6 +387,8 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
 				return -EFAULT;
 
 			i++;
+			if (i >= size / 4)
+				return -EINVAL;
 			bocmds[i] = cmds[1];
 			addr = cmd_to_addr(cmds);
 		}
-- 
2.53.0

Re: [PATCH] accel/ethosu: fix OOB write in ethosu_gem_cmdstream_copy_and_validate()

[Muhammad Bilal]

While reviewing the command stream parser further, I noticed that
weight[1..3] and scale[1] have their base and length parsed but no
corresponding WEIGHT1_REGION/SCALE1_REGION commands exist in the UAPI.
After cmd_state_init() memsets the state to 0xff, their .region field
stays 0xff and is never assigned, so calc_sizes() never updates
region_size[] with their extents.

The job submission in ethosu_job.c validates region_size[i] <= gem->size,
but since secondary weights never wrote into region_size[], a userspace
caller could supply large base+length values for weight[1..3] or scale[1]
that exceed the GEM buffer without the kernel catching it.

Does the hardware specification guarantee that weight[1..3] and scale[1]
are always sub-offsets within weight[0]'s region, or can they reference
memory independently? If the latter, should their extents be validated
against region_size[weight[0].region] in calc_sizes()?

Muhammad Bilal

Claude review: Re: [PATCH] accel/ethosu: fix OOB write in ethosu_gem_cmdstream_copy_and_validate()

[Claude Code Review Bot]

Overall Series Review

Subject: Re: [PATCH] accel/ethosu: fix OOB write in ethosu_gem_cmdstream_copy_and_validate()
Author: Muhammad Bilal <meatuni001@gmail.com>
Patches: 2
Reviewed: 2026-05-25T17:27:28.100190

---

This is a single-patch submission fixing a real out-of-bounds write vulnerability in the Arm Ethos-U NPU accelerator driver's command stream parser. The bug analysis in the commit message is correct and well-articulated: when the loop index `i` is at `size/4 - 1` and a 64-bit command is encountered (bit 14 set), the post-increment of `i` causes `bocmds[i]` to write one element past the DMA-allocated buffer. Since both `size` and buffer contents come from userspace via ioctl, this is indeed a userspace-triggerable heap OOB write — a legitimate security fix.

The fix itself is correct and minimal. The patch is appropriate for stable backporting.

**Verdict: The patch is correct and should be accepted, with one minor style suggestion.**

---
Generated by Claude Code Patch Reviewer

Claude review: Re: [PATCH] accel/ethosu: fix OOB write in ethosu_gem_cmdstream_copy_and_validate()

[Claude Code Review Bot]

Patch Review

**Commit message quality: Good.** The description clearly explains the vulnerable code path, the preconditions for triggering the bug, the impact (userspace-triggerable heap OOB write), and the fix. The Fixes tag and stable CC are appropriate.

**Correctness: The fix is correct.**

Looking at the source at `ethosu_gem.c:370-392`:

```c
for (i = 0; i < size / 4; i++) {
    ...
    if (get_user(cmds[0], ucmds++))
        return -EFAULT;

    bocmds[i] = cmds[0];
    ...
    if (cmd & 0x4000) {
        if (get_user(cmds[1], ucmds++))
            return -EFAULT;

        i++;
        bocmds[i] = cmds[1];   /* <-- OOB when i was size/4-1 */
        addr = cmd_to_addr(cmds);
    }
```

The patch adds:

```c
        i++;
+       if (i >= size / 4)
+           return -EINVAL;
        bocmds[i] = cmds[1];
```

This correctly bounds-checks the incremented index before the write. The `>=` comparison is correct since valid indices are `[0, size/4 - 1]`.

**Minor observations:**

1. **Error code choice:** `-EINVAL` is reasonable here — the command stream is malformed (a 64-bit command that doesn't fit in the declared buffer size). This is consistent with other validation failures in the same file.

2. **Potential style nit:** The existing code at line 386-387 checks `get_user()` with braces around the block, while this new bare `if`/`return` has no braces. Both are acceptable in kernel style for single-statement bodies, but some maintainers prefer braces when the surrounding code uses them. This is not a blocking concern.

3. **`size` alignment:** The function trusts that `size` is a multiple of 4 (using `size / 4` as the word count). If `size` were, say, 3, then `size / 4 == 0` and the loop body never executes, so there's no issue from this patch's perspective. The caller should validate this, but that's a pre-existing concern unrelated to this fix.

4. **No other 64-bit command paths:** I confirmed that this is the only place in the loop where `i` is incremented mid-iteration, so there are no other similar OOB paths to fix.

**Verdict: Correct fix. Recommend acceptance.**

---
Generated by Claude Code Patch Reviewer