From 68078e50d8d426650b9181c380454d4a73ab3cfe Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Wed, 29 Jul 2026 06:16:56 -0400 Subject: [PATCH] drivers: validate device-supplied completion ids (virtio used-ring, NVMe 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 --- drivers/nvme-queue.cc | 21 +++++++++++++-- drivers/virtio-vring.cc | 57 +++++++++++++++++++++++++++++++---------- 2 files changed, 62 insertions(+), 16 deletions(-) diff --git a/drivers/nvme-queue.cc b/drivers/nvme-queue.cc index 7719c57ba..6da65a3e9 100644 --- a/drivers/nvme-queue.cc +++ b/drivers/nvme-queue.cc @@ -342,10 +342,27 @@ void io_queue_pair::req_done() } } // - // Read cid and release it + // Read cid and release it. cqe.cid is device-supplied; a malicious + // or buggy controller can return a cid whose row (cid / _qsize) + // exceeds max_pending_levels, which would index _pending_bios[] out + // of bounds. Validate before use (assert() is compiled out in + // release builds, so it is not a real guard here). u16 cid = cqe.cid; + if (cid_to_row(cid) >= max_pending_levels || + !_pending_bios[cid_to_row(cid)]) { + // Bogus completion id from the device: either the row index is + // out of range, or it names a row we never allocated (rows are + // allocated lazily, so an in-range-but-unallocated row would + // otherwise dereference a null _pending_bios[row]). The CQ head + // was already advanced above, so just skip this entry. + continue; + } auto pending_bio = _pending_bios[cid_to_row(cid)][cid_to_col(cid)].exchange(nullptr); - assert(pending_bio); + if (!pending_bio) { + // No bio outstanding for this cid (duplicate / spurious + // completion); ignore rather than dereference a null. + continue; + } // // Save for future re-use or free PRP list saved under bio_private if any if (pending_bio->bio_private) { diff --git a/drivers/virtio-vring.cc b/drivers/virtio-vring.cc index 3912ee464..2fc681bcf 100644 --- a/drivers/virtio-vring.cc +++ b/drivers/virtio-vring.cc @@ -233,11 +233,23 @@ namespace virtio { elem = _used->_used_elements[used_ptr]; int idx = elem._id; + // elem._id comes straight from the device/backend; a malicious + // or buggy backend can return an id outside [0, _num), which + // would index _desc[] out of bounds here (and again below when + // walking the chain). Stop draining on a bogus id rather than + // read/write past the descriptor table. + if ((unsigned)idx >= (unsigned)_num) { + break; + } + if (_desc[idx]._flags & vring_desc::VRING_DESC_F_INDIRECT) { free_phys_contiguous_aligned(mmu::phys_to_virt(_desc[idx]._paddr)); } else while (_desc[idx]._flags & vring_desc::VRING_DESC_F_NEXT) { idx = _desc[idx]._next; + if ((unsigned)idx >= (unsigned)_num) { + break; + } i++; } @@ -257,24 +269,41 @@ namespace virtio { vring_used_elem elem; void* cookie = nullptr; - // need to trim the free running counter w/ the array size - int used_ptr = _used_ring_host_head & (_num - 1); - u16 used_idx = _used->_idx.load(std::memory_order_acquire); - - trace_vring_get_buf_elem(this, _used_ring_host_head, - used_idx); + // Skip past any bogus completions the device/backend may have + // written, so a malicious id neither indexes _cookie[] out of + // bounds nor wedges the drain loop forever on the same bad entry. + for (;;) { + // need to trim the free running counter w/ the array size + int used_ptr = _used_ring_host_head & (_num - 1); + u16 used_idx = _used->_idx.load(std::memory_order_acquire); - if (_used_ring_host_head == used_idx) { - return nullptr; - } + trace_vring_get_buf_elem(this, _used_ring_host_head, + used_idx); - elem = _used->_used_elements[used_ptr]; - *len = elem._len; + if (_used_ring_host_head == used_idx) { + return nullptr; + } - cookie = _cookie[elem._id]; - _cookie[elem._id] = nullptr; //maybe use this array for the full size hdrs? + elem = _used->_used_elements[used_ptr]; - return cookie; + // elem._id is written by the device/backend; a malicious or + // buggy backend can return an id outside [0, _num), which would + // index _cookie[] out of bounds (OOB read of a bogus cookie + // pointer that is then dereferenced/freed, plus an OOB NULL + // write). Advance past the bad entry instead of returning it + // (returning nullptr here would stop the caller's drain loop + // without advancing the head, stalling the queue on the same + // bogus entry forever). + if (elem._id >= _num) { + _used_ring_host_head++; + continue; + } + + *len = elem._len; + cookie = _cookie[elem._id]; + _cookie[elem._id] = nullptr; //maybe use this array for the full size hdrs? + return cookie; + } } bool vring::avail_ring_not_empty()