|WARNING| common/cnxk: fix mailbox message size underflow

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/168806

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-01

# DPDK Patch Review

## Summary
This patch addresses a security vulnerability in the VF mailbox message handling code where a malicious VF could cause integer underflow in message size calculation, leading to buffer overrun and potential PF crash.

---

## Errors

### 1. Integer Underflow Check is Incomplete
**File:** `drivers/common/cnxk/roc_dev.c`  
**Line:** `size = mbox->rx_start + msg->next_msgoff - offset;`

The patch adds validation AFTER the potentially underflowing calculation has already occurred. The size variable should not be assigned an underflowed value at all.

**Why it matters:** If `offset > (mbox->rx_start + msg->next_msgoff)`, the unsigned arithmetic wraps to a large positive value before the check. While the validation catches the huge value, it's cleaner to validate the inputs before the calculation.

**Suggested fix:**
```c
/* Validate next_msgoff before computing size */
if (msg->next_msgoff < sizeof(struct mbox_msghdr) ||
    msg->next_msgoff > MBOX_MSG_REQ_SIZE_MAX + sizeof(struct mbox_msghdr)) {
	plt_err("VF%d: invalid next_msgoff %u", vf, msg->next_msgoff);
	break;
}

size = mbox->rx_start + msg->next_msgoff - offset;

/* Additional bounds check on computed size */
if (size < sizeof(struct mbox_msghdr) ||
    (size - sizeof(struct mbox_msghdr)) > MBOX_MSG_REQ_SIZE_MAX) {
	plt_err("VF%d: invalid computed msg size %zu", vf, size);
	break;
}
```

Alternatively, compute size with explicit underflow protection:
```c
if (msg->next_msgoff <= offset - mbox->rx_start ||
    msg->next_msgoff > MBOX_MSG_REQ_SIZE_MAX + sizeof(struct mbox_msghdr)) {
	plt_err("VF%d: invalid next_msgoff %u", vf, msg->next_msgoff);
	break;
}
size = mbox->rx_start + msg->next_msgoff - offset;
```

---

## Warnings

### 1. Missing Variable Type Analysis
The variable `size` is declared as `size_t` but the comment says "underflowed size". Verify that `mbox->rx_start`, `msg->next_msgoff`, and `offset` are all unsigned types so the underflow behavior is well-defined (wraps to large value) rather than undefined behavior.

**Action:** Confirm these are unsigned integers. If any are signed, the arithmetic could produce undefined behavior before the check.

### 2. Incomplete Security Comment
The comment states "an underflowed size cannot be used as the copy length below" but doesn't explain what copy operation this protects. 

**Suggested improvement:**
```c
/*
 * next_msgoff is VF-controlled. Validate it before computing
 * the message size to prevent integer underflow that would
 * cause mbox_process() to copy beyond the mailbox region.
 */
```

### 3. Error Path Behavior on Invalid Message
The code uses `break` on validation failure, which exits the loop processing messages from this VF. Verify this is the intended behavior - should the mailbox be marked as corrupted, or should a response be sent to the VF indicating rejection?

**Action:** Confirm that silently stopping message processing is the correct response to a malicious VF, or if additional state cleanup/notification is required.

---

## Info

### 1. Consider Additional Offset Validation
The code validates the computed `size` but doesn't directly validate `offset`. If `offset` itself is corrupted or manipulated, it could point outside the mailbox region even if `size` passes validation.

**Suggested additional check:**
```c
if (offset < mbox->rx_start || offset >= mbox->rx_end) {
	plt_err("VF%d: message offset out of range", vf);
	break;
}
```

### 2. `MBOX_MSG_REQ_SIZE_MAX` Definition
Verify that `MBOX_MSG_REQ_SIZE_MAX` is defined and appropriate for this validation context. The constant should exclude the header size since the check is `(size - sizeof(struct mbox_msghdr)) > MBOX_MSG_REQ_SIZE_MAX`.

---

## Style Observations

 Explicit comparison used (`size < sizeof(...)` not `!size`)  
 Multi-line comment uses proper C style  
 Error message includes VF identifier for debugging  
 Code indentation appears correct (tabs for indent)

---

## Validation Checklist Status

- [x] Addresses a real correctness bug (security vulnerability)
- [x] Code compiles independently (single file change)
- [x] Proper error handling pattern (break on invalid input)
- [x] Cc: stable at dpdk.org present (backport candidate)
- [x] Fixes: tag present with commit reference
- [ ] **Needs verification:** Validation occurs before underflow, not after
- [ ] **Needs verification:** All related variables are unsigned types
- [ ] **Needs verification:** Error path behavior is complete