Claude review: accel/ivpu: Add bounds checks for firmware log indices

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Positive aspects:**
- Correctly identifies that `read_index` and `write_index` come from firmware 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 >= data_size)
+		log_start = 0;
+	if (log_end > data_size)
+		log_end = data_size;
```

The checks use different comparison operators: `>=` 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 == data_size`, then `log_data + log_end` would point one byte past the end of the data buffer. While `fw_log_print_lines` is passed `log_end` as a size (not used as an index offset), the wrapping case calls `fw_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 == data_size` would read the entire buffer — that's actually valid.

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

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

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

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

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

This would require passing `vdev` into `fw_log_print_buffer()`, which the function 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 = 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 it at that point, but it may be worth validating here too for defense in depth. 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 — the original code that reads firmware indices without validation existed before 1fc1251149a7. The Fixes tag should point to the commit that originally introduced the unvalidated reads, not the refactoring 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 (`>=` vs `>`) should be clarified or made consistent. Consider whether the `Fixes:` tag points to the right commit for stable backport purposes.

---
Generated by Claude Code Patch Reviewer