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

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

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