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()