|WARNING| [14/14] gso: fix overflow when writing to very small buffers

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-14

# DPDK Patch Review

## PATCH 01/14: vhost/crypto: fix TOCTOU OOB write in finalize path

### Errors

**TOCTOU / Use-After-Revalidation (Error)**
Line 1712: `desc_idx = vc_req->desc_idx;`
Line 1745: `vq->used->ring[desc_idx].id = desc_idx;`

The patch eliminates the re-read of `desc_idx` from guest memory at finalization time,
which is correct. However, the new bounds check at line 1733:
```c
if (unlikely(desc_idx >= vq->size)) {
    VC_LOG_ERR("Invalid descriptor index %u", desc_idx);
    return NULL;
}
```
validates against `vq->size`, but the `desc_idx` value in `vc_req->desc_idx`
was captured at fetch time when the ring was a different size.
A malicious guest could:
1. Set up a small ring (e.g., size 256)
2. Submit a request with `desc_idx = 200`
3. Enlarge the ring to size 512 between fetch and finalization
4. The bounds check passes (200 < 512), but the `used->ring` array is still sized for 256

The correct fix requires validating `desc_idx` at **capture time** against the ring size at that moment,
and storing that validated index. The finalization check should be against the saved ring size, not the current `vq->size`.

**Missing Validation at Fetch Time (Error)**
Lines 2054-2057 (first fetch loop) and 2088-2091 (second fetch loop):
```c
if (unlikely(desc_idx >= vq->size))
    break;
```
The `desc_idx` read from `vq->avail->ring[used_idx]` is validated against `vq->size`,
but this check is after the read. The TOCTOU issue exists here too:
`desc_idx` should be captured once, validated once against a stable ring size,
and the validated value passed to `vhost_crypto_process_one_req()`.

The patch converts the third parameter of `vhost_crypto_process_one_req()` from `used_idx` to `desc_idx`,
which is the right direction, but the validation is still racy because `vq->size` can change.

**Recommendation:**
- Capture `vq->size` in a local variable at the start of the fetch loop.
- Validate `desc_idx` against that captured size.
- Store the validated `desc_idx` in `vc_req->desc_idx`.
- At finalization, validate against the size stored at fetch time, not the current `vq->size`.

---

## PATCH 02/14: vhost: fix missing memory size validation before mmap

### Clean

The `fstat()` check at lines 1388-1398 prevents the mmap-larger-than-file issue.
The logic is correct: `mmap_size` is computed at line 1361,
validated against the file's `st_size` at line 1394,
and only then passed to `mmap()` at line 1402.

---

## PATCH 03/14: vhost: fix double-fetch vulnerability in control queue

### Errors

**Incorrect Overflow Detection (Error)**
Lines 180-183:
```c
if (copied + desc_len > data_len) {
    VHOST_CONFIG_LOG(dev->ifname, ERR, "Descriptor length overflow detected");
    goto free_err;
}
```
This check is placed **after** the `memcpy()` at line 185,
so the overflow has already occurred before the check runs.
The check must be moved to **before** line 185.

Additionally, `desc_len` at line 177 is re-read from `descs[desc_idx].len` (guest-controlled memory)
after being used to compute `data_len` at the top of the function.
The correct pattern is:
1. Capture `desc_len` at line 177 in a local variable.
2. **Before** the `memcpy()`, check `if (copied + desc_len > data_len)`.
3. Perform the `memcpy()` only if the check passes.

**Recommendation:**
Move the overflow check to before the `memcpy()` at line 185.

---

## PATCH 04/14: vhost: fix log base address mapping offset

### Errors

**Incorrect `munmap()` Size Calculation (Error)**
Line 299:
```c
munmap((void *)(uintptr_t)dev->log_addr,
       dev->log_size + dev->log_base - dev->log_addr);
```
Line 2743:
```c
munmap((void *)(uintptr_t)dev->log_addr,
       dev->log_size + dev->log_base - dev->log_addr);
```

After the patch, `dev->log_addr` is the base of the mmap
(the value returned by `mmap()` at line 2730),
and `dev->log_base = dev->log_addr + off` (line 2747).
The size passed to `mmap()` is `size + off` (line 2730).
The `munmap()` size should therefore be `size + off`,
which equals `dev->log_size + off`.

The expression `dev->log_size + dev->log_base - dev->log_addr`
expands to `dev->log_size + (dev->log_addr + off) - dev->log_addr = dev->log_size + off`,
which is correct.

However, this is unnecessarily convoluted. A clearer fix would store the mmap size
in a field (e.g., `dev->log_mmap_size`) at mmap time and use that in `munmap()`.

Retracting the Error -- the arithmetic is correct, just opaque.

### Clean

The double-application of offset is fixed by mapping at file offset 0 and adding the offset to the result.

---

## PATCH 05/14: vhost: fix VDUSE stale vring pointers after IOTLB invalidation

### Clean

The patch moves `is_vring_iotlb()` to `vhost.c` and adds the missing vring invalidation
to the `VDUSE_UPDATE_IOTLB` handler, mirroring the logic already present in `vhost_user.c`.
The invalidation is protected by `vq->access_lock` (line 467),
matching the locking discipline in the vhost-user path.

---

## PATCH 06/14: vhost: fix header copy to multiple descriptors

### Errors

**Missing `nr_vec` Bounds Check Before Increment (Error)**
Line 1049:
```c
if (unlikely(idx + 1 >= nr_vec))
    return -1;

buf_vec++;
idx++;
```

The check is `idx + 1 >= nr_vec`, which prevents `idx` from reaching `nr_vec - 1`.
However, the subsequent `buf_vec++` and `idx++` advance the index to `idx + 1`,
which is then used to access `buf_vec[idx]` (now `buf_vec[idx+1]` in terms of the original array)
at line 1037 on the next loop iteration.

The condition should be `idx >= nr_vec - 1` (or equivalently `idx + 1 >= nr_vec` before increment)
to ensure `buf_vec[idx]` is valid after the increment. The current code is correct.

Retracting.

### Clean

The temporary buffer approach and early-exit on overflow are correct.

---

## PATCH 07/14: vhost/crypto: catch incorrect request length

### Clean

Widening the addition operands to `uint64_t` before the sum prevents the 32-bit overflow.
The casts at lines 1185, 1187, and 1189 are placed on the first operand of each addition,
which is sufficient to promote the entire expression to 64-bit.

---

## PATCH 08/14: vhost/crypto: drop crypto session cache

### Clean

Removing the cache eliminates the race where a session is closed on one thread
while another thread uses the cached pointer. Each request now performs a hash lookup,
which is the safe approach.

---

## PATCH 09/14: vhost/crypto: fix write back chains termination

### Errors

**Recursive Free Without Tail-Call Optimization Risk (Warning)**
Line 890:
```c
static void
free_wb_data(struct vhost_crypto_writeback_data *wb_data,
        struct rte_mempool *mp)
{
    if (wb_data->next != NULL)
        free_wb_data(wb_data->next, mp);

    wb_data->next = NULL;
    rte_mempool_put(mp, wb_data);
}
```

The recursion depth is bounded by the number of descriptors in a single request,
which is bounded by `max_n_descs` (passed to `prepare_write_back_data()`).
However, long chains could still exhaust the stack in a resource-constrained environment.

This is not a correctness bug per se (the recursion terminates correctly),
but converting to an iterative free would be more robust.

Severity: Warning (not Error -- the code is correct, just suboptimal).

---

## PATCH 10/14: vhost: check number of queues for VDPA

### Clean

The `queue_num` check at line 3312 prevents out-of-bounds vring access.
The multiplication by 2 accounts for the Rx/Tx queue pairs.

---

## PATCH 11/14: vhost: fix out-of-bounds read in control virtqueue handler

### Errors

**`data_len` Does Not Include Header (Error)**
Line 227:
```c
if (data_len < sizeof(*ctrl_req) + sizeof(queue_pairs)) {
```

At line 138, `data_len` is computed as the sum of **readable** descriptor lengths
(those without `VRING_DESC_F_WRITE`). This is the payload size,
not the total request size. The `ctrl_req` structure is already allocated
and the header bytes (`ctrl_req->class` and `ctrl_req->command`) are accessible,
so `data_len` represents the bytes available in `ctrl_req->command_data` and beyond.

The check should be:
```c
if (data_len < sizeof(queue_pairs)) {
```
because `data_len` does not include the 2-byte header.

Alternatively, if `data_len` is intended to include the header,
the computation at line 138 must be adjusted to add `sizeof(struct virtio_net_ctrl_hdr)`.

**Recommendation:**
Clarify whether `data_len` includes the header. If not, the check should only validate
that `command_data` has enough bytes for the expected payload.

---

## PATCH 12/14: vhost: fix inflight descriptor table overflow

### Clean

The bounds checks at lines 1152, 1202, 1264, 1311, 1398, and 1405
validate indices against `desc_num`, preventing out-of-bounds access
when the inflight table size is smaller than the virtqueue size.

---

## PATCH 13/14: vhost: fix indirect descriptor table double fetch

### Errors

**Missing Leak Fix in Packed Path (Error)**
Line 941:
```c
if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id,
                descs[i].addr, dlen,
                perm))) {
    free_ind_table(idescs);
    return -1;
}
```

The `free_ind_table()` call is correctly added here.
However, the loop at line 933 has another error path at line 937:
```c
dlen = descs[i].len;
if (unlikely(dlen == 0 || dlen > remaining)) {
    free_ind_table(idescs);  // MISSING
    return -1;
}
```

The `free_ind_table(idescs)` call is missing before the `return -1` at line 937.
This is a memory leak on the dlen validation failure path.

**Recommendation:**
Add `free_ind_table(idescs);` before `return -1;` at line 937.

---

## PATCH 14/14: gso: fix overflow when writing to very small buffers

### Errors

**Redundant Header Size Check (Warning)**
Line 65:
```c
if (unlikely(pkt_hdr_offset > pkt->data_len || pkt_hdr_offset > direct_tailroom))
    return -EINVAL;
```

The `pkt_hdr_offset > pkt->data_len` check is redundant with the existing
`hdr_offset >= pkt->pkt_len` checks in the per-protocol segment functions
(gso_tcp4.c line 58, gso_tunnel_tcp4.c line 80, etc.).
However, adding it here is harmless and makes the precondition explicit.

Severity: Info (not an error, just a note that the check is duplicated).

---

## Summary

### Errors (7 findings)
1. **PATCH 01**: TOCTOU in bounds check -- `desc_idx` validated against current `vq->size`, not the size at capture time
2. **PATCH 01**: Fetch-time validation still racy -- `vq->size` can change between read and check
3. **PATCH 03**: Overflow check **after** memcpy -- check must be moved before line 185
4. **PATCH 11**: `data_len` validation incorrect -- does not account for whether header is included in `data_len`
5. **PATCH 13**: Memory leak on `dlen` validation failure in packed indirect path (line 937)

### Warnings (1 finding)
6. **PATCH 09**: Recursive free could exhaust stack on long chains -- iterative free preferred

### Info (1 finding)
7. **PATCH 14**: Redundant header size check at line 65 (duplicates per-protocol checks)

Review-Result: ERROR