|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

**Use of guest-controlled descriptor index without bounds check in fetch loops:**

```c
uint16_t desc_idx = vq->avail->ring[used_idx];
```

The descriptor index `desc_idx` is read from guest-writable memory (`vq->avail->ring[used_idx]`) and then used to access `vq->desc[desc_idx]` without first validating that `desc_idx < vq->size`. This can cause an out-of-bounds read of the descriptor table. The patch adds bounds checks *after* the descriptor is already used to initialize `head`:

```c
if (unlikely(desc_idx >= vq->size))
    break;

head = &vq->desc[desc_idx];
```

The check must occur *before* the descriptor table access. The correct pattern is:

```c
uint16_t desc_idx = vq->avail->ring[used_idx];

if (unlikely(desc_idx >= vq->size))
    break;

struct vring_desc *head = &vq->desc[desc_idx];
```

This applies to both occurrences in the patch (lines starting at `@@ -2044,16 +2048,20` and `@@ -2074,9 +2082,13`).

**Assignment that shadows the intended variable:**

```c
-	uint16_t used_idx, desc_idx;
+	uint16_t desc_idx;
```

The patch removes `used_idx` from the local variable list in `vhost_crypto_finalize_one_request()`, but the comment and subsequent line:

```c
vq = vc_req->vq;
-	used_idx = vc_req->desc_idx;
+	desc_idx = vc_req->desc_idx;
```

indicate that the patch intends to use the saved `desc_idx` from `vc_req`. However, later the code writes:

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

The removed line `desc_idx = vq->avail->ring[used_idx];` is the TOCTOU vulnerability the patch claims to fix. But the second line in the old code, `vq->avail->ring[desc_idx]`, uses the *new* `desc_idx` value as an index into `vq->avail->ring`, which is nonsensical (an avail ring index being used to index the avail ring itself). This suggests the original code was already wrong, and the patch's fix--using the saved `vc_req->desc_idx` directly--is correct. However, the patch does not explain this pre-existing bug, and the commit message focuses only on the TOCTOU issue, not the index-of-index mistake.

---

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

### Errors

None. The logic is correct: `fstat()` retrieves the file size, and the check `mmap_size > (uint64_t)file_stat.st_size` catches the malicious oversized mapping before `mmap()` is called.

---

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

### Errors

None. The patch adds a cumulative byte counter `copied` and verifies `copied + desc_len <= data_len` before each `memcpy()`, preventing the heap overflow from a TOCTOU descriptor length change.

---

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

### Errors

None. The patch correctly maps the full region (`size + off`) at file offset 0, matching the pattern in `vhost_user_mmap_region()`, and updates `munmap()` calls to use the actual mapped size.

---

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

### Errors

None. The patch adds the missing `is_vring_iotlb()` check and `vring_invalidate()` call in the VDUSE_UPDATE_IOTLB handler, mirroring the vhost-user VHOST_IOTLB_INVALIDATE logic.

---

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

### Errors

**Missing check that return value is used:**

```c
-static __rte_noinline void
+static __rte_noinline int
 copy_vnet_hdr_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
-		struct buf_vector *buf_vec,
-		struct virtio_net_hdr_mrg_rxbuf *hdr)
+		struct buf_vector *buf_vec, uint16_t nr_vec,
+		struct virtio_net_hdr_mrg_rxbuf *hdr,
+		uint32_t *vec_idx, uint32_t *buf_offset)
```

The function now returns `int` and can return `-1` on error. The call site in `mbuf_to_desc()` checks the return value:

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

However, the other call site (the `else` branch) does not check the return value:

```c
+		hdr = (struct virtio_net_hdr_mrg_rxbuf *)(uintptr_t)buf_addr;
+		virtio_enqueue_offload(m, &hdr->hdr);
+		if (rxvq_is_mergeable(dev))
+			ASSIGN_UNLESS_EQUAL(hdr->num_buffers, num_buffers);
+
+		PRINT_PACKET(dev, (uintptr_t)buf_addr, dev->vhost_hlen, 0);
+		vhost_log_cache_write_iova(dev, vq, buf_iova, dev->vhost_hlen);
```

This path writes the header in-place to `buf_addr`, which the patch removed from the `copy_vnet_hdr_to_desc()` call. But the in-place write is to the *first* descriptor's buffer, which is guaranteed to be large enough (the check `unlikely(buf_len < dev->vhost_hlen)` at the top of the function branches to the `if` case when the first descriptor is too small). So no error is possible here, and the missing return value check is benign--but the code would be clearer if `copy_vnet_hdr_to_desc()` were not called at all in the `else` branch. The patch does not call `copy_vnet_hdr_to_desc()` in the `else` branch, so this is not an error. On re-reading, the `else` branch logic is correct: it writes the header directly (no `copy_vnet_hdr_to_desc()` call), so no return value is ignored.

No error.

---

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

### Errors

None. The patch widens the sum to `uint64_t` before the comparison, preventing the 32-bit overflow that would allow malicious length values to pass validation.

---

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

### Errors

None. The cache is removed, eliminating the race condition and use-after-invalidation issues.

---

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

### Errors

None. The patch adds an `init_wb_data()` callback to the mempool constructor that sets `wb_data->next = NULL`, ensuring that chains are always properly terminated even if allocation fails partway through.

---

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

### Errors

None. The patch validates `vring_idx` against the driver's advertised queue count, preventing out-of-bounds accesses.

---

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

### Errors

None. The patch propagates `data_len` through `virtio_net_ctrl_elem` and validates the minimum required size (4 bytes) before dereferencing `command_data`.

---

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

### Errors

None. The patch adds bounds checks against `vq->inflight_split->desc_num` and `inflight_info->desc_num` in all inflight descriptor API functions, preventing out-of-bounds access when the inflight table size and virtqueue size are desynchronized.

---

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

### Errors

None. The patch captures the descriptor length once in a local variable (`desc_len`) and uses that for all subsequent operations, eliminating the TOCTOU vulnerability. The memory leak fix (`free_ind_table(idescs)` on the `map_one_desc()` error path) is also correct.

---

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

### Errors

**Incorrect check for header size vs segment size:**

```c
-	/* Don't process the packet without data */
+	/* Don't process the packet without data or hdr larger than seg size */
 	hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;
-	if (unlikely(hdr_offset >= pkt->pkt_len)) {
+	if (unlikely(hdr_offset >= pkt->pkt_len || hdr_offset >= gso_size))
 		return 0;
-	}
```

The check `hdr_offset >= gso_size` rejects packets where the header is larger than the GSO segment size. However, the GSO segment size is the *maximum* output segment size, not the buffer size. A packet with `hdr_offset == 60` and `gso_size == 1500` is valid: each output segment will be <= 1500 bytes. The check should be against the *direct buffer size* (the tailroom available in a freshly allocated mbuf), which is computed in `gso_do_segment()`:

```c
+	direct_buf_size = rte_pktmbuf_data_room_size(direct_pool);
+	if (unlikely(direct_buf_size <= (uint16_t)RTE_PKTMBUF_HEADROOM))
+		return -EINVAL;
+	direct_tailroom = direct_buf_size - RTE_PKTMBUF_HEADROOM;
+	if (unlikely(pkt_hdr_offset > pkt->data_len || pkt_hdr_offset > direct_tailroom))
+		return -EINVAL;
```

The check in `gso_do_segment()` is correct: `pkt_hdr_offset > direct_tailroom` ensures the header fits in the mbuf. But the added checks in `gso_tcp4_segment()`, `gso_tunnel_tcp4_segment()`, `gso_tunnel_udp4_segment()`, and `gso_udp4_segment()` compare `hdr_offset` against `gso_size`, which is too strict. A header of 100 bytes with a GSO size of 80 should fail (the header doesn't fit in a segment), but a header of 100 bytes with a GSO size of 1500 should not fail unless the mbuf buffer size is also < 100. The correct check is `hdr_offset > direct_tailroom`, but `direct_tailroom` is not available in these functions--only in `gso_do_segment()`.

The patch's approach is conservative (reject any packet where the header alone would exceed the segment size), which is safe but may reject valid packets when the mbuf pool has larger buffers. The vulnerability is prevented by the check in `gso_do_segment()`, so the additional checks in the segment functions are redundant and overly strict.

**Recommendation:** Remove the `hdr_offset >= gso_size` checks from the segment functions and rely solely on the `pkt_hdr_offset > direct_tailroom` check in `gso_do_segment()`. If a defense-in-depth check is desired, it should use `direct_tailroom`, not `gso_size`.

---

## Summary

**PATCH 01/14:** Descriptor index bounds check occurs *after* the descriptor is accessed (`head = &vq->desc[desc_idx]`), not before. This is a use-before-check bug (out-of-bounds read still possible).

**PATCH 14/14:** The `hdr_offset >= gso_size` checks in the segment functions are too strict (compare header size to segment size instead of buffer size) and may reject valid packets. The check in `gso_do_segment()` is correct and sufficient.

All other patches are correct.

---

Review-Result: ERROR