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/ivpu: Add bounds checks for firmware log indices
Date: Thu, 04 Jun 2026 16:39:02 +1000
Message-ID: <review-patch1-20260529115842.135378-1-andrzej.kacprowski@linux.intel.com>
In-Reply-To: <20260529115842.135378-1-andrzej.kacprowski@linux.intel.com>
References: <20260529115842.135378-1-andrzej.kacprowski@linux.intel.com> <20260529115842.135378-1-andrzej.kacprowski@linux.intel.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

**Positive aspects:**
- Correctly identifies that `read_index` and `write_index` come from firmwa=
re via shared memory and must be validated
- Uses `READ_ONCE()` values that were already captured into local variables=
, avoiding TOCTOU issues
- Appropriately tagged with `Fixes:` and `Cc: stable`

**Issue 1: Asymmetric boundary checks (bug)**

```c
+	if (log_start >=3D data_size)
+		log_start =3D 0;
+	if (log_end > data_size)
+		log_end =3D data_size;
```

The checks use different comparison operators: `>=3D` for `log_start` and `=
>` for `log_end`. Looking at how these values are used downstream:

```c
if (log_end > log_start) {
    fw_log_print_lines(log_data + log_start, log_end - log_start, p);
} else {
    fw_log_print_lines(log_data + log_start, data_size - log_start, p);
    fw_log_print_lines(log_data, log_end, p);
}
```

If `log_end =3D=3D data_size`, then `log_data + log_end` would point one by=
te past the end of the data buffer. While `fw_log_print_lines` is passed `l=
og_end` as a size (not used as an index offset), the wrapping case calls `f=
w_log_print_lines(log_data + log_start, data_size - log_start, p)` which is=
 fine, and `fw_log_print_lines(log_data, log_end, p)` where `log_end =3D=3D=
 data_size` would read the entire buffer =E2=80=94 that's actually valid.

However, the asymmetry is still suspicious. The `read_index` and `write_ind=
ex` are documented in the header as offsets into the data area. A `write_in=
dex` equal to `data_size` would mean "one past the end," which is a valid s=
entinel for "buffer full" in a non-wrapping scenario, but it's an unusual v=
alue for a circular buffer. It would be cleaner and more defensive to use t=
he same check (`>=3D`) for both, clamping `log_end` to 0 as well, since an =
index pointing past the valid data area is equally invalid for both. If `lo=
g_end =3D=3D data_size` is intentionally valid, a comment explaining why wo=
uld be warranted.

**Suggestion:** Use `>=3D data_size` for both checks, or add a comment expl=
aining the intentional asymmetry.

**Issue 2: Silent clamping vs. reporting**

When out-of-bounds indices are encountered from firmware, the patch silentl=
y clamps them. This hides a firmware bug or corruption event. A `drm_dbg()`=
 or `ivpu_dbg()` call when clamping would help with debugging firmware issu=
es without being too noisy:

```c
if (log_start >=3D data_size) {
    ivpu_dbg(vdev, FW_BOOT, "FW log \"%s\" read_index %u out of bounds (siz=
e %u)\n",
             log->name, log_start, data_size);
    log_start =3D 0;
}
```

This would require passing `vdev` into `fw_log_print_buffer()`, which the f=
unction doesn't currently receive. Not a blocker, but worth considering.

**Issue 3: `ivpu_fw_log_mark_read` has the same exposure**

In `ivpu_fw_log_mark_read()`:
```c
log->read_index =3D READ_ONCE(log->write_index);
```

This copies `write_index` directly into `read_index` without validation. If=
 `write_index` is corrupted, this propagates the bad value to `read_index`,=
 which will be read back later via `READ_ONCE(log->read_index)` in `fw_log_=
print_buffer()`. The new bounds check in `fw_log_print_buffer` will catch i=
t at that point, but it may be worth validating here too for defense in dep=
th. This is arguably outside the scope of this patch, however.

**Issue 4: Fixes tag correctness**

The `Fixes:` tag references `1fc1251149a7 ("accel/ivpu: Refactor functions =
in ivpu_fw_log.c")` with `Cc: stable # v6.18+`. The vulnerability predates =
the refactor =E2=80=94 the original code that reads firmware indices withou=
t validation existed before 1fc1251149a7. The Fixes tag should point to the=
 commit that originally introduced the unvalidated reads, not the refactori=
ng commit, unless the refactoring changed the code path in a way that made =
the old bounds checking no longer apply. This affects which stable kernels =
get the backport.

**Summary:** The patch addresses a legitimate security/robustness concern. =
The asymmetric boundary check (`>=3D` vs `>`) should be clarified or made c=
onsistent. Consider whether the `Fixes:` tag points to the right commit for=
 stable backport purposes.

---
Generated by Claude Code Patch Reviewer