Skip to content

drivers: validate device-supplied completion ids (virtio used-ring _id, NVMe cid) - #1450

Open
gburd wants to merge 1 commit into
cloudius-systems:masterfrom
gburd:pr/sec-driver-backend
Open

drivers: validate device-supplied completion ids (virtio used-ring _id, NVMe cid)#1450
gburd wants to merge 1 commit into
cloudius-systems:masterfrom
gburd:pr/sec-driver-backend

Conversation

@gburd

@gburd gburd commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Security fix — OOB read/write driven by a malicious or buggy device backend. Relevant when the hypervisor/device backend is not fully trusted (soft backends, some cloud/nested setups); in a unikernel these corrupt kernel memory.

1. virtio used-ring elem._id unvalidated (drivers/virtio-vring.cc)

The used-element _id is written by the device and was indexed directly into _cookie[] (allocated with _num entries):

elem = _used->_used_elements[used_ptr];
cookie = _cookie[elem._id];        // OOB read if elem._id >= _num
_cookie[elem._id] = nullptr;       // OOB NULL write

A backend returning _id >= _num → OOB read of a bogus cookie pointer (subsequently dereferenced / deleted / passed to biodone) plus an OOB NULL write past the array. Fix: reject elem._id >= _num. Affects all virtio devices.

2. NVMe completion cid unvalidated (drivers/nvme-queue.cc)

u16 cid = cqe.cid;                                  // device-supplied
auto pending_bio = _pending_bios[cid_to_row(cid)][cid_to_col(cid)].exchange(nullptr);
assert(pending_bio);                                // compiled out in release

cid_to_row(cid) = cid / _qsize, but _pending_bios has only max_pending_levels (4) rows. A controller returning cid / _qsize >= 4 indexes out of bounds; the assert is a no-op in release builds. Fix: validate cid_to_row(cid) < max_pending_levels and that the slot is non-null (skip duplicate/spurious completions) before use.

Severity

High (CVSS ~7.4 each, AV:L via a malicious backend / kernel memory corruption). Both are in shipped master.

Verified: boot + virtio-blk I/O (tst-vblk) pass with the guards in place.

@gburd
gburd force-pushed the pr/sec-driver-backend branch 2 times, most recently from c5ac056 to 6fb5280 Compare July 17, 2026 11:43
@wkozaczuk
wkozaczuk requested a review from Copilot July 25, 2026 20:08
Copilot AI reviewed Jul 25, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens OSV’s paravirtualized storage drivers against malicious or buggy device backends by validating device-supplied completion identifiers before using them as array indices, preventing potential out-of-bounds memory access in the kernel.

Changes:

  • Add bounds checking for virtio used-ring elem._id before indexing the _cookie[] array.
  • Add bounds checking for NVMe completion cqe.cid before indexing the _pending_bios tracking matrix, and ignore spurious/duplicate completions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
drivers/virtio-vring.cc Adds validation of device-supplied used-ring _id before indexing cookies.
drivers/nvme-queue.cc Adds validation of device-supplied completion cid before indexing pending-bio tracking.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread drivers/virtio-vring.cc Outdated
Comment thread drivers/nvme-queue.cc
Comment thread drivers/virtio-vring.cc Outdated
@gburd
drivers: validate device-supplied completion ids (virtio used-ring, N…
68078e5 
…VMe cid)

A malicious or buggy device backend can write a completion identifier that
the driver then uses as an array index. Harden the paravirtualized storage
drivers so such an id cannot cause out-of-bounds access or wedge the queue.

virtio-vring:
 - get_buf_elem() rejects a used-ring elem._id outside [0, _num) that would
   otherwise index _cookie[] out of bounds. Rather than return nullptr
   (which would stop the caller's drain loop without advancing the used
   head, stalling the queue forever on the same bad entry), it advances
   past the bogus entry and keeps draining.
 - get_buf_gc() now bounds-checks elem._id and every _desc[]._next chain
   link before indexing the descriptor table, and stops draining on a
   bad id instead of walking off the end.

nvme-queue:
 - The completion path validates cqe.cid before use: it must map to a row
   within max_pending_levels AND to a row that was actually allocated
   (rows are allocated lazily, so an in-range-but-unallocated row would
   dereference a null _pending_bios[row]). A bogus cid is skipped (the CQ
   head was already advanced). Note assert() is compiled out in release
   builds, so an explicit runtime check is required here.

Signed-off-by: Greg Burd <greg@burd.me>
@gburd
gburd force-pushed the pr/sec-driver-backend branch from 6fb5280 to 68078e5 Compare July 29, 2026 10:19
@gburd

gburd commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks Copilot, these are good catches. Addressed in the current tip (68078e5, rebased onto master):

  • get_buf_gc() reads device-supplied elem._id and indexes _desc[] unchecked (OOB in the GC path): get_buf_gc() now bounds-checks elem._id and every _desc[]._next chain link against [0, _num) before dereferencing, and stops draining on a bad id instead of walking off the descriptor table. This is the same class of bug the original guard fixed in get_buf_elem(), now closed in both paths.
  • get_buf_elem() returning nullptr on a bad id stalls the queue (DoS): right, returning nullptr stopped the caller's drain loop without advancing _used_ring_host_head, so the same bogus used entry sat at the head forever. It now advances past the bad entry and keeps draining, so a malicious backend forfeits that descriptor slot but cannot wedge completions.
  • NVMe cid in an in-range but lazily-unallocated row: right, the row-index bound was not enough because rows above 0 are allocated on demand. The completion path now also checks _pending_bios[row] is non-null before indexing, so an in-range-but-unallocated row is treated as a bogus completion and skipped (assert() is compiled out in release builds, so this is an explicit runtime check).

Needs a build to confirm it compiles; queued.

@gburd

gburd commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Build-validated on x86_64: builds clean (virtio-vring.cc, nvme.cc, nvme-queue.cc). Hot-path smoke: ZFS root mounted off both virtio-blk and NVMe, tst-fallocate 9/9 on each, on 1- and 4-CPU boots. The get_buf_gc/get_buf_elem drain-loop restructure handles normal completion traffic with no queue stall or regression on either backend. (The crafted-malicious-id path needs a hostile backend harness to exercise directly; the guards are in place and normal draining is confirmed green.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gburd