virtio: five descriptor walk sites across four devices, each with a different subset of the bounds

[cspinetta]

Problem

Two independent descriptor-chain walkers read guest memory in this tree. SplitVirtqueue::pop_avail (src/devices/virtqueue.rs) serves the userspace vsock device. Virtio9pDevice::process_queue (src/devices/virtio_9p.rs:503) walks its own chain instead: it reads the available ring, follows next indices through the descriptor table, splits readable from device-writable descriptors, and writes the used ring by hand.

Both implement the same virtio split-queue contract over the same untrusted input — a guest chooses the queue size, the three ring base addresses, and every descriptor field the walk reads. Every bound one walker applies, the other needs too, and nothing enforces that. The two drifted, and each ended up with guards the other lacked:

Guard	SplitVirtqueue::pop_avail	Virtio9pDevice::process_queue
Queue size of 0 refused	missing — divided by zero on the first kick	present
next index at or past the queue size	missing — read outside the descriptor table	present
Chain length bounded by the queue size	present	missing — a self-referential next looped forever
Ring base plus offset arithmetic checked	missing — panicked under overflow-checks	missing — same, through unchecked_add

Four of the seven defects #169 fixed are rows of that table. Both walkers now carry all four guards and share virtqueue::ring_addr for the arithmetic, so there is no live defect here. The duplication that let them diverge is still in place.

Proposed change

Give Virtio9pDevice a SplitVirtqueue and delete its private walk. The device keeps the 9P-specific work at the caller level: splitting chain.descriptors on VRING_DESC_F_WRITE, capping assembled request bytes at the negotiated msize, and writing the reply back into the writable descriptors.

What stands in the way

SplitVirtqueue::new takes a kick fd and a call fd, and signal_guest writes to them. The 9P device has no eventfds — it raises a polled interrupt_status bit instead. Either the fds become optional, or the device passes -1 the way the fuzz harness does; the first is the honest shape.

Queue state lives in different places. Virtio9pDevice holds QueueState plus its own avail_idx / used_idx fields, driven from mmio_write; SplitVirtqueue holds last_avail_idx / last_used_idx and takes its geometry at construction. The MMIO register handlers have to build or update the queue rather than write struct fields.

Error propagation differs. process_queue returns crate::Result and propagates a guest-memory read failure; pop_avail returns None and drops the reason. A caller that wants the errno keeps needing it.

Done when

• src/devices/virtio_9p.rs contains no descriptor-table read of its own, and grep unchecked_add src/devices/ is empty.
• The existing 9P transport tests still pass: process_queue_caps_descriptor_reads_at_msize and process_queue_stops_at_an_out_of_table_descriptor_index.
• cargo +nightly fuzz run nine_p_transport runs clean, and e2e_mount passes on Linux (virtio-9p is the transport it exercises).

Follow-up from #169. Related to #155.

[cspinetta]

Reopened: this closed as completed when #169 merged, but that PR bounded the walks rather than merging them. Nothing has been consolidated — SplitVirtqueue still has exactly one consumer, and grep -c 'let mut next = head_idx;' src/devices/*.rs still finds a private walk in virtio_9p, virtio_blk, and two in virtio_net.

Three PRs since then changed what this issue should say.

The count is five walk sites, not four

virtio_net has two — process_tx_queue and write_frames_to_rx_ring — and they diverged from each other, not only from the rest. The RX walk ended when the frame was consumed, which reads as a bound until you notice a descriptor with len == 0 or addr == 0 advances nothing, so a cycle among those never reaches it. The TX walk had no length bound at all.

The guard matrix, as it stood before #172

Guard	SplitVirtqueue	virtio_9p	virtio_blk	virtio_net
Queue size of 0 refused	missing	present	present	present
next at or past the queue size	missing	present	present	present
Chain length bounded	present	missing	present (32)	missing
Ring arithmetic checked	missing	missing	missing	missing

Every walker was missing something a sibling had. That is the argument for this issue, and it is empirical rather than stylistic: four of the seven defects #155 found are cells of this table, and #172 found two more in the walker nobody had checked.

What the fixes revealed about the shape of the merge

The consumers differ, the walk does not. virtio_blk wants header/data/status; virtio_9p wants readable bytes concatenated plus a list of writable descriptors; virtio_net wants the frame bytes; virtio_vsock_userspace wants a 44-byte header plus payload. All four are different readings of the same traversal.

Returning Vec<VirtqDesc> would regress virtio_net. Its TX path reads directly into the packet's tail to avoid an allocation and a memcpy per descriptor segment. An iterator-shaped API lets the caller read into its own buffer and keeps that; a Vec return does not. Changing pop_avail to yield an iterator is worth doing as an isolated change first, against its single current consumer.

Signalling is not part of walking. SplitVirtqueue holds a kick fd and a call fd; the other three raise a polled interrupt_status bit. Whatever shape the shared walker takes, signalling stays with the device.

Validating the rings once beats checking each element. #172 moved virtio_net to resolving all three rings at full size before either walk reads anything — one range lookup per kick instead of one per descriptor, and it covers the used.idx write and every available-ring read, which per-element checks did not. This is the shape the shared walker should have.

Order of work

1. Fuzz the virtio-net and virtio-blk device parsers #180 first — fuzz targets for virtio_net and virtio_blk. Merging walkers is rewriting a parser of attacker-controlled input, and today nothing would catch a mistake in those two.
2. Change pop_avail to yield an iterator, against its one consumer.
3. Migrate one device per PR, in ascending risk: virtio_blk, virtio_9p, virtio_net. During each migration, run both walkers on the same input and assert they produce identical chains — a temporary differential check that turns "did I break it" into a test, and that costs little because the fuzzing infrastructure already exists.

Still open, and deliberately not folded in

virtio_9p and virtio_blk reach guest memory through vm_memory's unchecked_add in places (a plain + despite the name); virtio_net and SplitVirtqueue were moved to a checked helper. Adopting virtio-queue from rust-vmm instead of writing a unified walker is worth its own evaluation, but doing both at once bundles a dependency migration with a refactor — and the snapshot format serialises queue state, so that migration is not free.