* Re: [PATCH] accel/ivpu: Add bounds checks for firmware log indices
2026-05-29 11:58 [PATCH] accel/ivpu: Add bounds checks for firmware log indices Andrzej Kacprowski
@ 2026-05-29 12:06 ` Wachowski, Karol
2026-06-02 5:49 ` Wachowski, Karol
2026-06-04 6:39 ` Claude review: " Claude Code Review Bot
2026-06-04 6:39 ` Claude Code Review Bot
2 siblings, 1 reply; 5+ messages in thread
From: Wachowski, Karol @ 2026-05-29 12:06 UTC (permalink / raw)
To: Andrzej Kacprowski, dri-devel
Cc: oded.gabbay, jeff.hugo, lizhi.hou, dawid.osuchowski, stable
On 29-May-26 13:58, Andrzej Kacprowski wrote:
> Add validation that read and write indices in the firmware log buffer
> are within valid bounds (< data_size) before using them. If
> out-of-bounds indices are encountered (from firmware), clamp them to
> safe values instead of proceeding with invalid offsets.
>
> This prevents potential out-of-bounds buffer access when firmware
> supplies invalid log indices.
>
> Fixes: 1fc1251149a7 ("accel/ivpu: Refactor functions in ivpu_fw_log.c")
> Cc: <stable@vger.kernel.org> # v6.18+
> Signed-off-by: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
Reviewed-by: Karol Wachowski <karol.wachowski@linux.intel.com>
> ---
> drivers/accel/ivpu/ivpu_fw_log.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/accel/ivpu/ivpu_fw_log.c b/drivers/accel/ivpu/ivpu_fw_log.c
> index 337c906b0210..275baf844b56 100644
> --- a/drivers/accel/ivpu/ivpu_fw_log.c
> +++ b/drivers/accel/ivpu/ivpu_fw_log.c
> @@ -98,6 +98,11 @@ static void fw_log_print_buffer(struct vpu_tracing_buffer_header *log, const cha
> u32 log_start = only_new_msgs ? READ_ONCE(log->read_index) : 0;
> u32 log_end = READ_ONCE(log->write_index);
>
> + if (log_start >= data_size)
> + log_start = 0;
> + if (log_end > data_size)
> + log_end = data_size;
> +
> if (log->wrap_count == log->read_wrap_count) {
> if (log_end <= log_start) {
> drm_printf(p, "==== %s \"%s\" log empty ====\n", prefix, log->name);
^ permalink raw reply [flat|nested] 5+ messages in thread* Claude review: accel/ivpu: Add bounds checks for firmware log indices
2026-05-29 11:58 [PATCH] accel/ivpu: Add bounds checks for firmware log indices Andrzej Kacprowski
2026-05-29 12:06 ` Wachowski, Karol
2026-06-04 6:39 ` Claude review: " Claude Code Review Bot
@ 2026-06-04 6:39 ` Claude Code Review Bot
2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 6:39 UTC (permalink / raw)
To: dri-devel-reviews
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
^ permalink raw reply [flat|nested] 5+ messages in thread