[PATCH] accel/ivpu: Add bounds checks for firmware log indices

[PATCH] accel/ivpu: Add bounds checks for firmware log indices

[Andrzej Kacprowski]

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>
---
 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);
-- 
2.43.0

Re: [PATCH] accel/ivpu: Add bounds checks for firmware log indices

[Wachowski, Karol]

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);

Re: [PATCH] accel/ivpu: Add bounds checks for firmware log indices

[Wachowski, Karol]

On 29-May-26 14:06, Wachowski, Karol wrote:
> 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>
> 

Applied to drm-misc-fixes.

>> ---
>>   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);
> 
> 

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

[Claude Code Review Bot]

Overall Series Review

Subject: accel/ivpu: Add bounds checks for firmware log indices
Author: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
Patches: 3
Reviewed: 2026-06-04T16:39:02.376357

---

This is a single-patch series that adds bounds checking to firmware log indices (`read_index` and `write_index`) in the IVPU (Intel VPU accelerator) driver. The firmware writes these indices and the host reads them via `READ_ONCE()`, so they are an untrusted input boundary that should be validated. The fix is targeted at a real bug class — out-of-bounds buffer access from malicious or corrupted firmware data — and is appropriately tagged for stable backport.

The patch is **mostly correct** but has an **asymmetry in the boundary checks** that appears to be a bug, and there are some additional considerations worth discussing.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

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