virtio-queue: Error out on invalid available ring index

src/devices/src/virtio/block/device.rs

@@ -1477,7 +1477,6 @@ pub(crate) mod tests {
             add_flush_requests_batch(&mut block, &mem, &vq, 1);
             simulate_queue_event(&mut block, Some(false));
             assert_eq!(block.is_io_engine_throttled, true);
-            assert_eq!(block.queues[0].len(&mem), 1);
 
             simulate_async_completion_event(&mut block, true);
             assert_eq!(block.is_io_engine_throttled, false);

src/devices/src/virtio/queue.rs

@@ -49,7 +49,7 @@ impl fmt::Display for QueueError {
     }
 }
 
-/// A virtio descriptor constraints with C representive.
+/// A virtio descriptor constraints with C representative.
 #[repr(C)]
 #[derive(Default, Clone, Copy)]
 struct Descriptor {
@@ -104,9 +104,9 @@ impl<'a> DescriptorChain<'a> {
         // These reads can't fail unless Guest memory is hopelessly broken.
         let desc = match mem.read_obj::<Descriptor>(desc_head) {
             Ok(ret) => ret,
-            Err(_) => {
+            Err(err) => {
                 // TODO log address
-                error!("Failed to read from memory");
+                error!("Failed to read virtio descriptor from memory: {}", err);
                 return None;
             }
         };
@@ -289,7 +289,7 @@ impl Queue {
     }
 
     /// Returns the number of yet-to-be-popped descriptor chains in the avail ring.
-    pub fn len(&self, mem: &GuestMemoryMmap) -> u16 {
+    fn len(&self, mem: &GuestMemoryMmap) -> u16 {
         (self.avail_idx(mem) - self.next_avail).0
     }
 
@@ -300,7 +300,21 @@ impl Queue {
 
     /// Pop the first available descriptor chain from the avail ring.
     pub fn pop<'a, 'b>(&'a mut self, mem: &'b GuestMemoryMmap) -> Option<DescriptorChain<'b>> {
-        if self.len(mem) == 0 {
+        let len = self.len(mem);
+        // The number of descriptor chain heads to process should always
+        // be smaller or equal to the queue size, as the driver should
+        // never ask the VMM to process a available ring entry more than
+        // once. Checking and reporting such incorrect driver behavior
+        // can prevent potential hanging and Denial-of-Service from
+        // happening on the VMM side.
+        if len > self.actual_size() {
+            // We are choosing to interrupt execution since this could be a potential malicious
+            // driver scenario. This way we also eliminate the risk of repeatedly
+            // logging and potentially clogging the microVM through the log system.
+            panic!("The number of available virtio descriptors is greater than queue size!");
+        }
+
+        if len == 0 {
             return None;
         }
 
@@ -425,7 +439,7 @@ impl Queue {
         // via `self.is_valid()`, so it's safe to unwrap and use unchecked offsets here.
         // Note: the `MmioTransport` code ensures that queue addresses cannot be changed by the
         // guest       after device activation, so we can be certain that no change has
-        // occured since       the last `self.is_valid()` check.
+        // occurred since the last `self.is_valid()` check.
         let addr = self.avail_ring.unchecked_add(2);
         Wrapping(mem.read_obj::<u16>(addr).unwrap())
     }
@@ -461,7 +475,17 @@ impl Queue {
             return true;
         }
 
-        if self.len(mem) != 0 {
+        let len = self.len(mem);
+        if len != 0 {
+            // The number of descriptor chain heads to process should always
+            // be smaller or equal to the queue size.
+            if len > self.actual_size() {
+                // We are choosing to interrupt execution since this could be a potential malicious
+                // driver scenario. This way we also eliminate the risk of
+                // repeatedly logging and potentially clogging the microVM through
+                // the log system.
+                panic!("The number of available virtio descriptors is greater than queue size!");
+            }
             return false;
         }
 
@@ -741,6 +765,92 @@ pub(crate) mod tests {
         assert_eq!(q.avail_event(m), 2);
     }
 
+    #[test]
+    #[should_panic(
+        expected = "The number of available virtio descriptors is greater than queue size!"
+    )]
+    fn test_invalid_avail_idx_no_notification() {
+        // This test ensures constructing a descriptor chain succeeds
+        // with valid available ring indexes while it produces an error with invalid
+        // indexes.
+        // No notification suppression enabled.
+        let m = &create_anon_guest_memory(&[(GuestAddress(0), 0x6000)], false).unwrap();
+
+        // We set up a queue of size 4.
+        let vq = VirtQueue::new(GuestAddress(0), m, 4);
+        let mut q = vq.create_queue();
+
+        for j in 0..4 {
+            vq.dtable[j].set(
+                0x1000 * (j + 1) as u64,
+                0x1000,
+                VIRTQ_DESC_F_NEXT,
+                (j + 1) as u16,
+            );
+        }
+
+        // Create 2 descriptor chains.
+        // the chains are (0, 1) and (2, 3)
+        vq.dtable[1].flags.set(0);
+        vq.dtable[3].flags.set(0);
+        vq.avail.ring[0].set(0);
+        vq.avail.ring[1].set(2);
+        vq.avail.idx.set(2);
+
+        // We've just set up two chains.
+        assert_eq!(q.len(m), 2);
+
+        // We process the first descriptor.
+        let d = q.pop(m).unwrap().next_descriptor();
+        assert!(matches!(d, Some(x) if !x.has_next()));
+        // We confuse the device and set the available index as being 6.
+        vq.avail.idx.set(6);
+
+        // We've actually just popped a descriptor so 6 - 1 = 5.
+        assert_eq!(q.len(m), 5);
+
+        // However, since the apparent length set by the driver is more than the queue size,
+        // we would be running the risk of going through some descriptors more than once.
+        // As such, we expect to panic.
+        q.pop(m);
+    }
+
+    #[test]
+    #[should_panic(
+        expected = "The number of available virtio descriptors is greater than queue size!"
+    )]
+    fn test_invalid_avail_idx_with_notification() {
+        // This test ensures constructing a descriptor chain succeeds
+        // with valid available ring indexes while it produces an error with invalid
+        // indexes.
+        // Notification suppression is enabled.
+        let m = &create_anon_guest_memory(&[(GuestAddress(0), 0x6000)], false).unwrap();
+
+        // We set up a queue of size 4.
+        let vq = VirtQueue::new(GuestAddress(0), m, 4);
+        let mut q = vq.create_queue();
+
+        q.uses_notif_suppression = true;
+
+        // Create 1 descriptor chain of 4.
+        for j in 0..4 {
+            vq.dtable[j].set(
+                0x1000 * (j + 1) as u64,
+                0x1000,
+                VIRTQ_DESC_F_NEXT,
+                (j + 1) as u16,
+            );
+        }
+        // We need to clear the VIRTQ_DESC_F_NEXT for the last descriptor.
+        vq.dtable[3].flags.set(0);
+        vq.avail.ring[0].set(0);
+
+        // driver sets available index to suspicious value.
+        vq.avail.idx.set(6);
+
+        q.pop_or_enable_notification(m);
+    }
+
     #[test]
     fn test_add_used() {
         let m = &create_anon_guest_memory(&[(GuestAddress(0), 0x10000)], false).unwrap();

tests/framework/gitlint_rules.py

@@ -4,9 +4,6 @@
 
 from gitlint.rules import CommitRule, RuleViolation
 
-# Too few public methods (1/2) (too-few-public-methods)
-# pylint: disable=R0903
-
 
 class EndsSigned(CommitRule):
     """Checks commit message body formatting.
@@ -131,13 +128,13 @@ def rtn(stmt, i):
                 break
 
             return rtn(
-                (f"Non '{co_auth}' or '{sig}' string found " f"following 1st '{sig}'"),
+                f"Non '{co_auth}' or '{sig}' string found " f"following 1st '{sig}'",
                 i,
             )
 
         # Checks lines following co-author are only additional co-authors.
         for i, line in message_iter:
-            if not line.startswith(co_auth):
+            if line and not line.startswith(co_auth):
                 return rtn(
                     f"Non '{co_auth}' string found after 1st '{co_auth}'",
                     i,