diff --git a/crates/virtio-queue/src/lib.rs b/crates/virtio-queue/src/lib.rs index 2d4ed5d4..0e27935b 100644 --- a/crates/virtio-queue/src/lib.rs +++ b/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"), } } } diff --git a/crates/virtio-queue/src/queue.rs b/crates/virtio-queue/src/queue.rs index 03197d62..eaefa2cf 100644 --- a/crates/virtio-queue/src/queue.rs +++ b/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 = 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); + } }