public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2] accel/ivpu: Fix signed integer truncation in IPC receive
       [not found] <b464b589-2d28-4617-baf0-eefbe14e170a@linux.intel.com>
@ 2026-06-01 16:16 ` Andrzej Kacprowski
  2026-06-04  3:53   ` Claude review: " Claude Code Review Bot
  2026-06-04  3:53   ` Claude Code Review Bot
  0 siblings, 2 replies; 5+ messages in thread
From: Andrzej Kacprowski @ 2026-06-01 16:16 UTC (permalink / raw)
  To: dri-devel
  Cc: oded.gabbay, jeff.hugo, lizhi.hou, karol.wachowski,
	dawid.osuchowski, david.laight.linux, Andrzej Kacprowski, stable

Fix potential buffer overflow where firmware-supplied data_size is cast
to signed int before being used in min_t(). Large unsigned values
(>= 0x80000000) become negative, causing unsigned wraparound and
oversized memcpy operations that can overflow the stack buffer.

Change min_t(int, ...) to min() as both values are unsigned and can be
handled by min() without explicit cast.

Fixes: 3b434a3445ff ("accel/ivpu: Use threaded IRQ to handle JOB done messages")
Cc: <stable@vger.kernel.org> # v6.12+
Signed-off-by: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
---
Changes in v2:
- Replaced min_t() with min()

 drivers/accel/ivpu/ivpu_ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/accel/ivpu/ivpu_ipc.c b/drivers/accel/ivpu/ivpu_ipc.c
index f47df092bb0d..9347f05a2b79 100644
--- a/drivers/accel/ivpu/ivpu_ipc.c
+++ b/drivers/accel/ivpu/ivpu_ipc.c
@@ -276,7 +276,7 @@ int ivpu_ipc_receive(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons,
 	if (ipc_buf)
 		memcpy(ipc_buf, rx_msg->ipc_hdr, sizeof(*ipc_buf));
 	if (rx_msg->jsm_msg) {
-		u32 size = min_t(int, rx_msg->ipc_hdr->data_size, sizeof(*jsm_msg));
+		u32 size = min(rx_msg->ipc_hdr->data_size, sizeof(*jsm_msg));
 
 		if (rx_msg->jsm_msg->result != VPU_JSM_STATUS_SUCCESS) {
 			ivpu_err(vdev, "IPC resp result error: %d\n", rx_msg->jsm_msg->result);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Claude review: accel/ivpu: Fix signed integer truncation in IPC receive
  2026-06-01 16:16 ` [PATCH v2] accel/ivpu: Fix signed integer truncation in IPC receive Andrzej Kacprowski
@ 2026-06-04  3:53   ` Claude Code Review Bot
  2026-06-04  3:53   ` Claude Code Review Bot
  1 sibling, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-06-04  3:53 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: accel/ivpu: Fix signed integer truncation in IPC receive
Author: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
Patches: 1
Reviewed: 2026-06-04T13:53:03.415757

---

This is a single-patch fix for a real integer truncation bug in the Intel VPU (IVPU) IPC receive path. The issue is well-identified: `min_t(int, ...)` casts a `u32` value (`data_size`) to `signed int`, which means values >= 0x80000000 become negative. When that negative value is then used as a `u32` (the type of `size`), it wraps to a very large unsigned value, leading to an oversized `memcpy` at line 291 that could overflow a stack buffer.

The fix is correct and minimal — replacing `min_t(int, ...)` with `min()` — since both operands (`data_size` is `u32`, `sizeof(...)` is `size_t`) are unsigned, making `min()` safe and appropriate here. The Fixes tag, stable backport annotation, and commit message are all well-formed.

**Verdict: This patch looks correct and ready to merge.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: accel/ivpu: Fix signed integer truncation in IPC receive
  2026-06-01 16:16 ` [PATCH v2] accel/ivpu: Fix signed integer truncation in IPC receive Andrzej Kacprowski
  2026-06-04  3:53   ` Claude review: " Claude Code Review Bot
@ 2026-06-04  3:53   ` Claude Code Review Bot
  1 sibling, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-06-04  3:53 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**The bug is real and clearly explained.** Looking at the existing code:

```c
u32 size = min_t(int, rx_msg->ipc_hdr->data_size, sizeof(*jsm_msg));
```

`rx_msg->ipc_hdr->data_size` is `u32` (confirmed in `ivpu_ipc.h:38`), and `sizeof(*jsm_msg)` is `size_t`. The `min_t(int, ...)` cast truncates both to signed `int`. If firmware-supplied `data_size` is >= 0x80000000, the cast produces a negative value, which "wins" the min comparison, then gets assigned back to `u32 size` as a huge value, and the subsequent `memcpy(jsm_msg, rx_msg->jsm_msg, size)` at line 291 overflows.

**The fix is correct:**

```c
u32 size = min(rx_msg->ipc_hdr->data_size, sizeof(*jsm_msg));
```

- `data_size` is `u32`, `sizeof(...)` is `size_t` (unsigned). The kernel's `min()` macro handles type promotion correctly for unsigned types — `u32` will promote to `size_t` in the comparison, which is safe.
- The result is capped to `sizeof(*jsm_msg)` which fits in `u32`, so the assignment is also safe.

**Minor observations (not blocking):**

- The `sizeof(*jsm_msg)` operand has type `size_t`, while `data_size` has type `u32`. On 64-bit kernels these differ in width. The kernel's `min()` macro (using `__types_ok()`) permits this comparison when both are unsigned and one promotes safely. This works correctly, but if the build bot flags a type mismatch warning (some older kernel versions were stricter), `min_t(u32, ...)` would be the fallback. This should be fine on current kernels.

- The commit message and changelog are clean. The `Fixes:` tag references the correct commit that introduced `min_t(int, ...)`, and the `Cc: stable` annotation with `# v6.12+` is appropriate.

**Reviewed-by recommendation: This patch is correct and suitable for merging.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: accel/ivpu: Fix signed integer truncation in IPC receive
  2026-05-29 11:54 [PATCH] " Andrzej Kacprowski
@ 2026-06-04  6:40 ` Claude Code Review Bot
  2026-06-04  6:40 ` Claude Code Review Bot
  1 sibling, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-06-04  6:40 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: accel/ivpu: Fix signed integer truncation in IPC receive
Author: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
Patches: 2
Reviewed: 2026-06-04T16:40:23.158315

---

This is a single-patch fix for a signed integer truncation bug in the Intel VPU (IVPU) accelerator driver's IPC receive path. The fix is correct, minimal, and clearly explained. The `data_size` field is a `u32` sourced from firmware, and using `min_t(int, ...)` to compare it against `sizeof(*jsm_msg)` (also unsigned) is a genuine bug: any firmware-supplied value with bit 31 set would be interpreted as negative when cast to `int`, causing `min_t` to select the negative (now-huge-unsigned) value instead of the `sizeof`, leading to a stack buffer overflow via the subsequent `memcpy` at line 291.

The fix is straightforward and appropriate for stable backport. No concerns.

**Verdict: Patch looks good. Recommend acceptance.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: accel/ivpu: Fix signed integer truncation in IPC receive
  2026-05-29 11:54 [PATCH] " Andrzej Kacprowski
  2026-06-04  6:40 ` Claude review: " Claude Code Review Bot
@ 2026-06-04  6:40 ` Claude Code Review Bot
  1 sibling, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-06-04  6:40 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Good.**

The bug is real. Looking at the kernel tree (`ivpu_ipc.h:38`), `data_size` is declared as `u32`:

```c
struct ivpu_ipc_hdr {
    u32 data_addr;
    u32 data_size;
    ...
};
```

The original code at `ivpu_ipc.c:283`:
```c
u32 size = min_t(int, rx_msg->ipc_hdr->data_size, sizeof(*jsm_msg));
```

`min_t(int, ...)` casts both arguments to `int`. If `data_size >= 0x80000000`, the cast to `int` produces a negative value. Since `sizeof(*jsm_msg)` is positive, `min_t` picks the negative value. That negative `int` is then assigned to `u32 size`, wrapping to a very large unsigned value. The `memcpy` at line 291 then overflows `jsm_msg`:

```c
if (jsm_msg)
    memcpy(jsm_msg, rx_msg->jsm_msg, size);
```

The fix:
```c
u32 size = min_t(u32, rx_msg->ipc_hdr->data_size, sizeof(*jsm_msg));
```

This is correct — both `data_size` and `sizeof()` are naturally unsigned, so `u32` is the appropriate comparison type. The result is always clamped to at most `sizeof(*jsm_msg)`.

**Commit message:** Clear and accurate. The Fixes tag and stable Cc are appropriate. The `# v6.18+` annotation on the stable Cc indicates this only applies from v6.18 onwards, which matches the referenced commit.

**Minor observation (not a blocking concern):** The `data_size` value comes from firmware, which is a trust boundary. While this fix correctly prevents the overflow, there's no explicit validation that `data_size` is sane (e.g., not wildly larger than expected but still < 0x80000000). The `min_t(u32, ...)` fix handles this case too since it clamps to `sizeof(*jsm_msg)`, so this is a defense-in-depth comment rather than a problem with the patch.

Reviewed-by worthy as-is.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-06-04  6:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <b464b589-2d28-4617-baf0-eefbe14e170a@linux.intel.com>
2026-06-01 16:16 ` [PATCH v2] accel/ivpu: Fix signed integer truncation in IPC receive Andrzej Kacprowski
2026-06-04  3:53   ` Claude review: " Claude Code Review Bot
2026-06-04  3:53   ` Claude Code Review Bot
2026-05-29 11:54 [PATCH] " Andrzej Kacprowski
2026-06-04  6:40 ` Claude review: " Claude Code Review Bot
2026-06-04  6:40 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox