don't allow processing descriptors for invalid q

This was a bug discovered during fuzzing. The impact is that a
request coming from a malicious driver can trigger iterating
over descriptor chains that come from previous requests.

Co-authored-by: Laura Loghin <[email protected]>
Signed-off-by: Andreea Florescu <[email protected]>

crates/virtio-queue/src/lib.rs

@@ -65,6 +65,8 @@ pub enum Error {
     InvalidUsedRingAlign,
     /// Invalid available ring index.
     InvalidAvailRingIndex,
+    /// The queue is not ready for operation.
+    QueueNotReady,
 }
 
 impl Display for Error {
@@ -95,6 +97,7 @@ impl Display for Error {
                 f,
                 "invalid available ring index (more descriptors to process than queue size)"
             ),
+            QueueNotReady => write!(f, "trying to process requests on a queue that's not ready"),
         }
     }
 }

crates/virtio-queue/src/queue.rs

@@ -611,6 +611,13 @@ impl QueueOwnedT for Queue {
         M: Deref,
         M::Target: GuestMemory,
     {
+        // We're checking here that a reset did not happen without re-initializing the queue.
+        // TODO: In the future we might want to also check that the other parameters in the
+        // queue are valid.
+        if !self.ready || self.avail_ring == GuestAddress(0) {
+            return Err(Error::QueueNotReady);
+        }
+
         self.avail_idx(mem.deref(), Ordering::Acquire)
             .map(move |idx| AvailIter::new(mem, idx, self))?
     }
@@ -1378,8 +1385,7 @@ mod tests {
         {
             // an invalid queue should return an iterator with no next
             q.ready = false;
-            let mut i = q.iter(m).unwrap();
-            assert!(i.next().is_none());
+            assert!(q.iter(m).is_err());
         }
 
         q.ready = true;
@@ -1500,4 +1506,57 @@ mod tests {
         assert_eq!(q.try_set_size(15).unwrap_err(), Error::InvalidSize);
         assert_eq!(q.size(), expected_val)
     }
+
+    #[test]
+    // This is a regression test for a fuzzing finding. If the driver requests a reset of the
+    // device, but then does not re-initializes the queue then a subsequent call to process
+    // a request should yield no descriptors to process. Before this fix we were processing
+    // descriptors that were added to the queue before, and we were ending up processing 255
+    // descriptors per chain.
+    fn test_regression_timeout_after_reset() {
+        // The input below was generated by libfuzzer and adapted for this test.
+        let m = &GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0x0), 0x10000)]).unwrap();
+        let vq = MockSplitQueue::new(m, 1024);
+
+        // This input below was generated by the fuzzer.
+        let descriptors: Vec<Descriptor> = vec![
+            Descriptor::new(21508325467, 0, 1, 4),
+            Descriptor::new(2097152, 4096, 3, 0),
+            Descriptor::new(18374686479672737792, 4294967295, 65535, 29),
+            Descriptor::new(76842670169653248, 1114115, 0, 0),
+            Descriptor::new(16, 983040, 126, 3),
+            Descriptor::new(897648164864, 0, 0, 0),
+            Descriptor::new(111669149722, 0, 0, 0),
+        ];
+        vq.build_multiple_desc_chains(&descriptors).unwrap();
+
+        let mut q: Queue = vq.create_queue().unwrap();
+
+        // Setting the queue to ready should not allow consuming descriptors after reset.
+        q.reset();
+        q.set_ready(true);
+        let mut counter = 0;
+        while let Some(mut desc_chain) = q.pop_descriptor_chain(m) {
+            // this empty loop is here to check that there are no side effects
+            // in terms of memory & execution time.
+            while desc_chain.next().is_some() {
+                counter += 1;
+            }
+        }
+        assert_eq!(counter, 0);
+
+        // Setting the avail_addr to valid should not allow consuming descriptors after reset.
+        q.reset();
+        q.set_avail_ring_address(Some(0x1000), None);
+        assert_eq!(q.avail_ring, GuestAddress(0x1000));
+        counter = 0;
+        while let Some(mut desc_chain) = q.pop_descriptor_chain(m) {
+            // this empty loop is here to check that there are no side effects
+            // in terms of memory & execution time.
+            while desc_chain.next().is_some() {
+                counter += 1;
+            }
+        }
+        assert_eq!(counter, 0);
+    }
 }