Skip to content

Commit c5ac056

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 1603209 commit c5ac056

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
@@ -341,10 +341,23 @@ void io_queue_pair::req_done()
341341
}
342342
}
343343
//
344-
// Read cid and release it
344+
// Read cid and release it. cqe.cid is device-supplied; a malicious
345+
// or buggy controller can return a cid whose row (cid / _qsize)
346+
// exceeds max_pending_levels, which would index _pending_bios[] out
347+
// of bounds. Validate before use (assert() is compiled out in
348+
// release builds, so it is not a real guard here).
345349
u16 cid = cqe.cid;
350+
if (cid_to_row(cid) >= max_pending_levels) {
351+
// Bogus completion id from the device; the CQ head was already
352+
// advanced above, so just skip this entry.
353+
continue;
354+
}
346355
auto pending_bio = _pending_bios[cid_to_row(cid)][cid_to_col(cid)].exchange(nullptr);
347-
assert(pending_bio);
356+
if (!pending_bio) {
357+
// No bio outstanding for this cid (duplicate / spurious
358+
// completion); ignore rather than dereference a null.
359+
continue;
360+
}
348361
//
349362
// Save for future re-use or free PRP list saved under bio_private if any
350363
if (pending_bio->bio_private) {

drivers/virtio-vring.cc

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

260+
// elem._id is written by the device/backend; a malicious or buggy
261+
// backend can return an id outside [0, _num), which would index
262+
// _cookie[] out of bounds (OOB read of a bogus cookie pointer that
263+
// is then dereferenced/freed, plus an OOB NULL write). Reject it.
264+
if (elem._id >= _num) {
265+
return nullptr;
266+
}
260267
cookie = _cookie[elem._id];
261268
_cookie[elem._id] = nullptr; //maybe use this array for the full size hdrs?
262269

0 commit comments

Comments
 (0)