|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 Series Review

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

### Errors

**Use of `desc_idx` from request vs from virtqueue:**
The patch replaces reading `vq->avail->ring[used_idx]` in the finalize function with `vc_req->desc_idx`, which is correct for eliminating the TOCTOU vulnerability. However, the line:

```c
vq->used->ring[desc_idx].id = desc_idx;
```

should be:

```c
vq->used->ring[desc_idx].id = vq->avail->ring[desc_idx];
```

The `id` field in `vq->used->ring[desc_idx]` should contain the descriptor index that was originally in `vq->avail->ring[desc_idx]`, not `desc_idx` itself. In the original code, `used_idx` was the index into the available ring, and `desc_idx = vq->avail->ring[used_idx]`. The patch changes `used_idx` to `desc_idx` but then writes `desc_idx` to both the index and the id field. The id field should still come from the available ring at that descriptor index, or the saved value from request creation time should be stored separately.

This corrupts the used ring by writing the wrong id value, breaking the virtio protocol contract with the guest.

**Bounds check placement in fetch loops:**
The bounds checks added in the fetch loops check `desc_idx >= vq->size` after reading `desc_idx = vq->avail->ring[used_idx]` but before dereferencing `&vq->desc[desc_idx]`. This is correct and prevents the immediate out-of-bounds access. However, the check does not prevent the malicious guest from passing a valid `desc_idx` at fetch time and then modifying it in the available ring before finalization. The core fix (using the saved `vc_req->desc_idx`) is correct, but the patch should clarify that the fetch-time bounds checks are a defense-in-depth measure, not the primary fix.

---

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

### Warnings

**fstat error handling:**
The patch adds `fstat()` call and checks for `-1` return, which is correct. However, the error log uses `strerror(errno)`, and if `fstat()` fails, `errno` may not be set reliably on all platforms. It is safer to log the return value and avoid assuming `errno` is valid. This is a minor point but worth noting for robustness.

**Type consistency for st_size:**
`file_stat.st_size` is `off_t`, which is signed. The comparison casts it to `uint64_t`. If `st_size` is negative (malformed file), the cast produces a large unsigned value and the check may not behave as intended. Add a check that `file_stat.st_size >= 0` before the cast, or cast to `int64_t` and check signedness explicitly.

---

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

### Errors

**Incomplete bounds check:**
The patch adds a cumulative `copied` counter and checks `if (copied + desc_len > data_len)` before each `memcpy()`. This is correct for preventing overflow beyond the allocated buffer size. However, the check is placed inside the descriptor traversal loop, and `desc_len` is read from `descs[desc_idx].len`, which is in guest-controlled memory. The `desc_len` variable is not captured in a local before the comparison, so a TOCTOU still exists between the bounds check and the memcpy:

```c
desc_len = descs[desc_idx].len;
// Guest can modify descs[desc_idx].len here
if (copied + desc_len > data_len) { ... }
memcpy(ctrl_req, (void *)(uintptr_t)desc_addr, desc_len);
```

The `memcpy()` re-reads `desc_len` from the descriptor (implicitly, since `desc_len` is just an alias for the descriptor field). The fix must capture `desc_len` into a local variable before the check and use that local in the memcpy.

**Corrected pattern:**

```c
uint32_t desc_len_local = descs[desc_idx].len;
if (copied + desc_len_local > data_len) {
    VHOST_CONFIG_LOG(...);
    goto free_err;
}
memcpy(ctrl_req, (void *)(uintptr_t)desc_addr, desc_len_local);
ctrl_req += desc_len_local;
copied += desc_len_local;
```

Without this, the TOCTOU is not fully closed.

---

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

### Warnings

**munmap size calculation in cleanup path:**
The patch changes the `munmap()` call to:

```c
munmap((void *)(uintptr_t)dev->log_addr, dev->log_size + dev->log_base - dev->log_addr);
```

This assumes `dev->log_base >= dev->log_addr`. If `log_base` is less than `log_addr` due to corruption or a logic error elsewhere, the subtraction underflows and produces a huge size. Add a sanity check that `dev->log_base >= dev->log_addr` before the munmap, or at least document that this invariant is guaranteed by the mmap logic.

---

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

### Clean

The patch correctly mirrors the vhost-user IOTLB invalidation logic by calling `is_vring_iotlb()` and `vring_invalidate()` in the VDUSE handler. The move of `is_vring_iotlb()` to `vhost.c` for shared use is appropriate. The locking (write lock on `vq->access_lock`) is correct.

---

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

### Errors

**Return value not checked:**
The new function `copy_vnet_hdr_to_desc()` returns `int` (0 on success, -1 on error), but the return value is not checked at one call site:

```c
if (copy_vnet_hdr_to_desc(dev, vq, buf_vec, nr_vec,
        &tmp_hdr, &vec_idx, &buf_offset) < 0)
    return -1;
```

This is correct. However, the second call site (where `hdr` is not `&tmp_hdr`) in the else branch does not call `copy_vnet_hdr_to_desc()` at all -- it directly writes the header and logs/validates. The patch should either make both paths use the same function for consistency, or document why the direct-write path is safe. As written, the direct-write path assumes the header fits in the first descriptor and does not check `buf_len >= dev->vhost_hlen`. The original code also did not check this, so it is pre-existing, but the patch introduces a bounds-checked function without applying it uniformly.

**Suggested fix:** Apply `copy_vnet_hdr_to_desc()` to both branches, or add a comment explaining why the else branch does not need it.

---

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

### Clean

The patch widens the integer type in the overflow checks from `uint32_t` to `uint64_t`, preventing the overflow that would cause the checks to pass when they should fail. The casts to `uint64_t` on the left-hand side of the addition are correct and necessary.

---

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

### Clean

The patch removes the session cache variables (`cache_sym_session_id`, `cache_asym_session_id`, `cache_sym_session`, `cache_asym_session`) and always performs a hash lookup. This eliminates the race where the cached session could be invalidated by a guest close operation on another thread. The performance impact of the extra hash lookups is acceptable for a security fix.

---

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

### Clean

The patch adds `init_wb_data()` to initialize `wb_data->next = NULL` on allocation, and explicitly sets `->next = NULL` in the free paths. This ensures chains are properly terminated and prevents infinite loops and double frees. The use of `rte_mempool_obj_iter()` (implied by the `init_wb_data` callback in `rte_mempool_create`) is correct.

---

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

### Warnings

**Error path when `rte_vhost_driver_get_queue_num()` fails:**
The patch returns `-1` if `rte_vhost_driver_get_queue_num()` fails, which is correct. However, the function being checked is `vhost_user_check_and_alloc_queue_pair()`, and returning `-1` may not be sufficient to prevent the out-of-bounds access if the caller does not handle the error. Review the callers of this function to ensure they check the return value and do not proceed with an invalid `vring_idx`.

---

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

### Warnings

**data_len calculation and storage:**
The patch adds `data_len` to `struct virtio_net_ctrl_elem` and propagates it through the handler. The `data_len` is calculated in `virtio_net_ctrl_pop()` by summing descriptor lengths, but those lengths are read from guest memory and could overflow the allocation if the cumulative check is not also applied here. The patch in 03/14 adds the cumulative check, but 11/14 does not verify that the `data_len` stored in `ctrl_elem` is consistent with the allocated size. The two patches should be cross-referenced or merged to ensure the `data_len` is validated at allocation time as well.

---

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

### Clean

The patch adds bounds checks `if (unlikely(idx >= vq->inflight_split->desc_num))` in all paths that index into the inflight descriptor table. This prevents out-of-bounds access when the virtqueue size and inflight table size are desynchronized. The checks are correctly placed before the array access.

---

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

### Errors

**Descriptor length capture in fill_vec_buf_split:**
The patch captures `desc_len` in a local variable:

```c
uint64_t desc_len = vq->desc[idx].len;
```

However, `vq->desc[idx].len` is a `uint32_t` (the `len` field in `struct vring_desc`), so the local should be `uint32_t`, not `uint64_t`. The cast to `uint64_t` is harmless but inconsistent with the actual descriptor field size. Use `uint32_t desc_len` for clarity.

**Missing memory leak fix in fill_vec_buf_packed_indirect:**
The patch adds `free_ind_table(idescs)` on the `map_one_desc()` error path, which is correct and fixes a memory leak. However, the commit message describes this as "also fix a memory leak," implying it is secondary to the TOCTOU fix. This should be a separate line in the commit message under "Fixes:" to make it clear there are two bugs being fixed.

---

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

### Warnings

**Check for `direct_tailroom` underflow:**
The patch calculates:

```c
direct_tailroom = direct_buf_size - RTE_PKTMBUF_HEADROOM;
```

If `direct_buf_size <= RTE_PKTMBUF_HEADROOM`, the subtraction underflows (wraps to a large unsigned value). The earlier check `if (unlikely(direct_buf_size <= (uint16_t)RTE_PKTMBUF_HEADROOM))` catches this, but the check uses `<=` when it should use `<` to allow exactly `RTE_PKTMBUF_HEADROOM` bytes of headroom (leaving zero tailroom, which is still valid for an empty packet). Alternatively, clarify the intent: if zero tailroom is invalid, the check is correct; if zero tailroom is allowed, use `<`.

**Type of `hdr_offset` in comparisons:**
The new check `pkt_hdr_offset > direct_tailroom` compares a function parameter (assumed `uint16_t` based on usage) against `direct_tailroom` (also `uint16_t`). If `pkt_hdr_offset` is a different type (e.g., `uint32_t`), the comparison may not behave as intended. Verify that all callers pass a `uint16_t` or add an explicit cast/check.

---

## Summary

### Errors (must fix)
- **Patch 01:** Used ring id field corruption (`vq->used->ring[desc_idx].id = desc_idx` should use the saved avail ring value).
- **Patch 03:** TOCTOU not fully closed -- `desc_len` must be captured in a local variable before bounds check and memcpy.
- **Patch 06:** Inconsistent header bounds checking between `tmp_hdr` and direct-write paths.
- **Patch 13:** Descriptor length type should be `uint32_t`, not `uint64_t` (minor type inconsistency).

### Warnings (should fix)
- **Patch 02:** Check `file_stat.st_size >= 0` before casting to unsigned.
- **Patch 04:** Document or check invariant `dev->log_base >= dev->log_addr` before munmap size calculation.
- **Patch 10:** Verify callers of `vhost_user_check_and_alloc_queue_pair()` handle `-1` return correctly.
- **Patch 11:** Cross-reference or merge with Patch 03 to ensure `data_len` is validated at allocation time.
- **Patch 14:** Clarify zero-tailroom validity and check direct_tailroom calculation bounds.

### Info
- **Patch 02:** `fstat()` error logging assumes errno is valid; consider logging return value directly.
- **Patch 13:** Memory leak fix should be called out explicitly in commit message as a second bug.

---

Review-Result: ERROR