Skip to content

Commit 6fb5280

Browse files
committed
drivers: validate device-supplied completion ids (virtio used-ring, NVMe cid)
Two OOB accesses driven by a malicious or buggy device backend (relevant for untrusted/soft hypervisor backends; kernel memory corruption in a unikernel): 1. virtio used-ring (drivers/virtio-vring.cc): the used-element _id is written by the device and was used directly to index _cookie[] (allocated with _num entries): 'cookie = _cookie[elem._id]; _cookie[elem._id] = nullptr;'. A backend returning _id >= _num causes an OOB read of a bogus cookie pointer (later dereferenced/freed) plus an OOB NULL write past the array. Reject elem._id >= _num. 2. NVMe completion (drivers/nvme-queue.cc): the completion-queue-entry cid is device-supplied and indexed _pending_bios[cid/_qsize][cid%_qsize] where the row dimension is only max_pending_levels (4). A controller returning a cid with cid/_qsize >= 4 indexes _pending_bios[] out of bounds; the following assert(pending_bio) is compiled out in release builds, so it was not a real guard. Validate cid_to_row(cid) < max_pending_levels and that the slot is non-null (skip duplicate/spurious completions) before use. Verified: boot + virtio-blk I/O (tst-vblk) still pass.
1 parent 97463b2 commit 6fb5280

2 files changed

Lines changed: 22 additions & 2 deletions

File tree

drivers/nvme-queue.cc

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -342,10 +342,23 @@ void io_queue_pair::req_done()
342342
}
343343
}
344344
//
345-
// Read cid and release it
345+
// Read cid and release it. cqe.cid is device-supplied; a malicious
346+
// or buggy controller can return a cid whose row (cid / _qsize)
347+
// exceeds max_pending_levels, which would index _pending_bios[] out
348+
// of bounds. Validate before use (assert() is compiled out in
349+
// release builds, so it is not a real guard here).
346350
u16 cid = cqe.cid;
351+
if (cid_to_row(cid) >= max_pending_levels) {
352+
// Bogus completion id from the device; the CQ head was already
353+
// advanced above, so just skip this entry.
354+
continue;
355+
}
347356
auto pending_bio = _pending_bios[cid_to_row(cid)][cid_to_col(cid)].exchange(nullptr);
348-
assert(pending_bio);
357+
if (!pending_bio) {
358+
// No bio outstanding for this cid (duplicate / spurious
359+
// completion); ignore rather than dereference a null.
360+
continue;
361+
}
349362
//
350363
// Save for future re-use or free PRP list saved under bio_private if any
351364
if (pending_bio->bio_private) {

drivers/virtio-vring.cc

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,13 @@ namespace virtio {
271271
elem = _used->_used_elements[used_ptr];
272272
*len = elem._len;
273273

274+
// elem._id is written by the device/backend; a malicious or buggy
275+
// backend can return an id outside [0, _num), which would index
276+
// _cookie[] out of bounds (OOB read of a bogus cookie pointer that
277+
// is then dereferenced/freed, plus an OOB NULL write). Reject it.
278+
if (elem._id >= _num) {
279+
return nullptr;
280+
}
274281
cookie = _cookie[elem._id];
275282
_cookie[elem._id] = nullptr; //maybe use this array for the full size hdrs?
276283

0 commit comments

Comments
 (0)