Check whole virtqueue rings in guest memory

alioth/src/mem/mapped.rs

@@ -287,22 +287,27 @@ impl Ram {
     }
 
     pub fn get_ptr<T>(&self, gpa: u64) -> Result<*mut T, Error> {
-        let host_ref = self.get_partial_slice_mut(gpa, size_of::<T>() as u64)?;
-        let ptr = host_ref.as_mut_ptr();
-        if host_ref.len() != size_of::<T>() {
-            error::NotContinuous {
-                addr: gpa,
-                size: size_of::<T>() as u64,
-            }
-            .fail()
+        self.get_array_ptr(gpa, 1)
+    }
+
+    /// Returns a pointer to `len` consecutive values of type `T` at `gpa`.
+    ///
+    /// All the values must be in one contiguous host mapping, so that the
+    /// pointer stays valid when offset by up to `len - 1` elements.
+    pub fn get_array_ptr<T>(&self, gpa: u64, len: u64) -> Result<*mut T, Error> {
+        let size = len.saturating_mul(size_of::<T>() as u64);
+        let host_ref = self.get_partial_slice_mut(gpa, size)?;
+        let ptr = host_ref.as_mut_ptr() as *mut T;
+        if host_ref.len() as u64 != size {
+            error::NotContinuous { addr: gpa, size }.fail()
         } else if !ptr.is_aligned() {
             error::NotAligned {
                 addr: ptr as u64,
                 align: align_of::<T>(),
             }
             .fail()
         } else {
-            Ok(ptr as *mut T)
+            Ok(ptr)
         }
     }
 

alioth/src/mem/mapped_test.rs

@@ -20,6 +20,7 @@ use libc::{PROT_READ, PROT_WRITE};
 use zerocopy::{FromBytes, Immutable, IntoBytes};
 
 use super::{ArcMemPages, RamBus};
+use crate::mem::Error;
 
 #[derive(Debug, IntoBytes, FromBytes, Immutable, PartialEq, Eq)]
 #[repr(C)]
@@ -88,3 +89,47 @@ fn test_ram_bus_read() {
     drop(locked_bus);
     bus.remove(0x0).unwrap();
 }
+
+#[test]
+fn test_ram_get_array_ptr() {
+    let bus = RamBus::new();
+    let prot = PROT_READ | PROT_WRITE;
+    // Two adjacent guest pages, backed by separate host mappings.
+    for gpa in [0, PAGE_SIZE] {
+        let pages = ArcMemPages::from_anonymous(PAGE_SIZE as usize, Some(prot), None).unwrap();
+        bus.add(gpa, pages).unwrap();
+    }
+    let ram = bus.lock_layout();
+
+    let ptr = ram.get_array_ptr::<u64>(PAGE_SIZE - 16, 2).unwrap();
+    assert_eq!(ptr, ram.get_ptr::<u64>(PAGE_SIZE - 16).unwrap());
+
+    // The last element is in the other host mapping.
+    assert_matches!(
+        ram.get_array_ptr::<u64>(PAGE_SIZE - 8, 2),
+        Err(Error::NotContinuous { addr, size: 16, .. }) if addr == PAGE_SIZE - 8
+    );
+    // The last element is beyond the end of guest memory.
+    assert_matches!(
+        ram.get_array_ptr::<u64>(2 * PAGE_SIZE - 8, 2),
+        Err(Error::NotContinuous { .. })
+    );
+    assert_matches!(
+        ram.get_array_ptr::<u64>(2 * PAGE_SIZE, 1),
+        Err(Error::NotMapped { .. })
+    );
+    assert_matches!(
+        ram.get_array_ptr::<u64>(0, u64::MAX),
+        Err(Error::NotContinuous { .. })
+    );
+
+    // Pointers must be aligned for the type.
+    assert_matches!(
+        ram.get_array_ptr::<u64>(4, 2),
+        Err(Error::NotAligned { align: 8, .. })
+    );
+    assert_matches!(
+        ram.get_ptr::<u64>(4),
+        Err(Error::NotAligned { align: 8, .. })
+    );
+}

alioth/src/virtio/queue/packed.rs

@@ -23,6 +23,11 @@ use crate::mem::mapped::Ram;
 use crate::virtio::queue::{DescChain, DescFlag, QueueReg, VirtQueue};
 use crate::virtio::{Result, error};
 
+/// The maximum size of a packed virtqueue (VirtIO spec 1.4, Sec 2.8).
+///
+/// [`WrappedIndex`] has 15 bits for the offset into the ring.
+const PACKED_QUEUE_SIZE_MAX: u16 = 1 << 15;
+
 #[repr(C, align(16))]
 #[derive(Debug, Default, FromBytes, IntoBytes)]
 struct Desc {
@@ -108,13 +113,15 @@ impl<'m> PackedQueue<'m> {
         if !reg.enabled.load(Ordering::Acquire) {
             return Ok(None);
         }
-        if size == 0 {
+        if size == 0 || size > PACKED_QUEUE_SIZE_MAX {
             return error::InvalidQueueSize { size }.fail();
         }
         let notification: *mut DescEvent = ram.get_ptr(device)?;
         Ok(Some(PackedQueue {
             size,
-            desc: ram.get_ptr(desc)?,
+            // The queue accesses descriptors up to the queue size, so the
+            // whole ring must be in guest memory.
+            desc: ram.get_array_ptr(desc, size as u64)?,
             enable_event_idx: event_idx,
             notification,
             interrupt: ram.get_ptr(driver)?,
@@ -178,6 +185,12 @@ impl<'m> VirtQueue<'m> for PackedQueue<'m> {
             if !flag.contains(DescFlag::NEXT) {
                 break desc.id;
             }
+            // A chain takes consecutive descriptors, so it cannot be longer
+            // than the ring. Stopping there also keeps the offset of the
+            // next chain within the ring.
+            if delta == self.size {
+                return error::DescChainTooLong { size: self.size }.fail();
+            }
             offset = (offset + 1) % self.size;
         };
         Ok(Some(DescChain {

alioth/src/virtio/queue/packed_test.rs

@@ -13,16 +13,18 @@
 // limitations under the License.
 
 use std::collections::HashMap;
+use std::mem::size_of;
 use std::sync::atomic::{AtomicU16, Ordering};
 
 use assert_matches::assert_matches;
 use rstest::rstest;
 
+use crate::mem;
 use crate::virtio::Error;
-use crate::virtio::queue::packed::{DescEvent, EventFlag, PackedQueue, WrappedIndex};
+use crate::virtio::queue::packed::{Desc, DescEvent, EventFlag, PackedQueue, WrappedIndex};
 use crate::virtio::queue::tests::{GuestQueue, UsedDesc, VirtQueueGuest};
 use crate::virtio::queue::{DescFlag, VirtQueue};
-use crate::virtio::tests::{DATA_ADDR, QUEUE_SIZE, fixture_queues, fixture_ram_bus};
+use crate::virtio::tests::{DATA_ADDR, MEM_SIZE, QUEUE_SIZE, fixture_queues, fixture_ram_bus};
 
 const WRAP_COUNTER: u16 = 1 << 15;
 
@@ -147,16 +149,73 @@ fn disabled_queue() {
     assert_matches!(split_queue, Ok(None));
 }
 
+#[rstest]
+#[case(0, false)]
+#[case(1 << 15, true)]
+#[case((1 << 15) + 1, false)]
+#[case(u16::MAX, false)]
+fn queue_size(#[case] size: u16, #[case] valid: bool) {
+    let ram_bus = fixture_ram_bus();
+    let queues = fixture_queues(1);
+    let ram = ram_bus.lock_layout();
+    let reg = &queues[0];
+
+    reg.size.store(size, Ordering::Relaxed);
+    let packed_queue = PackedQueue::new(reg, &ram, false);
+    if valid {
+        assert_matches!(packed_queue, Ok(Some(_)));
+    } else {
+        assert_matches!(packed_queue, Err(Error::InvalidQueueSize { size: s, .. }) if s == size);
+    }
+}
+
+#[rstest]
+#[case(QUEUE_SIZE, true)]
+#[case(QUEUE_SIZE - 1, false)]
+#[case(1, false)]
+fn ring_at_end_of_memory(#[case] entries: u16, #[case] valid: bool) {
+    let ram_bus = fixture_ram_bus();
+    let queues = fixture_queues(1);
+    let ram = ram_bus.lock_layout();
+    let reg = &queues[0];
+
+    // Place the ring so that only its first `entries` descriptors are in
+    // guest memory.
+    let ring_size = entries as u64 * size_of::<Desc>() as u64;
+    reg.desc
+        .store(MEM_SIZE as u64 - ring_size, Ordering::Relaxed);
+
+    let packed_queue = PackedQueue::new(reg, &ram, false);
+    if valid {
+        assert_matches!(packed_queue, Ok(Some(_)));
+    } else {
+        assert_matches!(
+            packed_queue,
+            Err(Error::Memory { source, .. }) if matches!(*source, mem::Error::NotContinuous { .. })
+        );
+    }
+}
+
 #[test]
-fn invalid_queue_size() {
+fn chain_longer_than_ring() {
     let ram_bus = fixture_ram_bus();
     let queues = fixture_queues(1);
     let ram = ram_bus.lock_layout();
     let reg = &queues[0];
+    reg.size.store(4, Ordering::Relaxed);
+    let q = PackedQueue::new(reg, &ram, false).unwrap().unwrap();
 
-    reg.size.store(0, Ordering::Relaxed);
-    let split_queue = PackedQueue::new(reg, &ram, false);
-    assert_matches!(split_queue, Err(Error::InvalidQueueSize { size: 0, .. }));
+    // Every descriptor is available and links to the next one, so the chain
+    // never ends.
+    for offset in 0..4 {
+        let desc = unsafe { &*q.desc.offset(offset) };
+        let flag = DescFlag::AVAIL | DescFlag::NEXT;
+        desc.flag.store(flag.bits(), Ordering::Release);
+    }
+    assert_matches!(
+        q.get_avail(WrappedIndex::INIT, &ram),
+        Err(Error::DescChainTooLong { size: 4, .. })
+    );
 }
 
 #[test]

alioth/src/virtio/queue/split.rs

@@ -152,15 +152,17 @@ impl<'m> SplitQueue<'m> {
         let used_hdr = ram.get_ptr::<UsedHeader>(used)?;
         let avail_ring_gpa = avail + size_of::<AvailHeader>() as u64;
         let used_ring_gpa = used + size_of::<UsedHeader>() as u64;
+        // The queue accesses entries up to the queue size through the ring
+        // pointers, so the whole rings must be in guest memory.
         Ok(Some(SplitQueue {
             size: size as u16,
             avail_hdr: ram.get_ptr(avail)?,
-            avail_ring: ram.get_ptr(avail_ring_gpa)?,
+            avail_ring: ram.get_array_ptr(avail_ring_gpa, size)?,
             used_event,
             used_hdr,
-            used_ring: ram.get_ptr(used_ring_gpa)?,
+            used_ring: ram.get_array_ptr(used_ring_gpa, size)?,
             avail_event,
-            desc: ram.get_ptr(desc)?,
+            desc: ram.get_array_ptr(desc, size)?,
             _phantom: PhantomData,
         }))
     }

alioth/src/virtio/queue/split_test.rs

@@ -13,15 +13,18 @@
 // limitations under the License.
 
 use std::collections::HashMap;
+use std::mem::size_of;
 use std::sync::atomic::Ordering;
 
 use assert_matches::assert_matches;
+use rstest::rstest;
 
+use crate::mem;
 use crate::virtio::Error;
 use crate::virtio::queue::VirtQueue;
-use crate::virtio::queue::split::{Desc, DescFlag, SplitQueue};
+use crate::virtio::queue::split::{AvailHeader, Desc, DescFlag, SplitQueue, UsedElem, UsedHeader};
 use crate::virtio::queue::tests::{GuestQueue, UsedDesc, VirtQueueGuest};
-use crate::virtio::tests::{DATA_ADDR, fixture_queues, fixture_ram_bus};
+use crate::virtio::tests::{DATA_ADDR, MEM_SIZE, QUEUE_SIZE, fixture_queues, fixture_ram_bus};
 
 impl<'m> VirtQueueGuest<'m> for SplitQueue<'m> {
     fn add_desc(
@@ -123,6 +126,63 @@ fn invalid_queue_size() {
     assert_matches!(split_queue, Err(Error::InvalidQueueSize { size: 3, .. }));
 }
 
+#[derive(Debug, Clone, Copy)]
+enum Ring {
+    Desc,
+    Avail,
+    Used,
+}
+
+#[rstest]
+#[case(Ring::Desc, QUEUE_SIZE, true)]
+#[case(Ring::Desc, QUEUE_SIZE - 1, false)]
+#[case(Ring::Desc, 1, false)]
+#[case(Ring::Avail, QUEUE_SIZE, true)]
+#[case(Ring::Avail, QUEUE_SIZE - 1, false)]
+#[case(Ring::Avail, 1, false)]
+#[case(Ring::Used, QUEUE_SIZE, true)]
+#[case(Ring::Used, QUEUE_SIZE - 1, false)]
+#[case(Ring::Used, 1, false)]
+fn ring_at_end_of_memory(#[case] ring: Ring, #[case] entries: u16, #[case] valid: bool) {
+    let ram_bus = fixture_ram_bus();
+    let queues = fixture_queues(1);
+    let ram = ram_bus.lock_layout();
+    let reg = &queues[0];
+
+    // Place the ring so that only its first `entries` entries are in guest
+    // memory.
+    let end = MEM_SIZE as u64;
+    let entries = entries as u64;
+    let (field, gpa) = match ring {
+        Ring::Desc => (&reg.desc, end - entries * size_of::<Desc>() as u64),
+        Ring::Avail => {
+            let ring_size = entries * size_of::<u16>() as u64;
+            (
+                &reg.driver,
+                end - ring_size - size_of::<AvailHeader>() as u64,
+            )
+        }
+        Ring::Used => {
+            let ring_size = entries * size_of::<UsedElem>() as u64;
+            (
+                &reg.device,
+                end - ring_size - size_of::<UsedHeader>() as u64,
+            )
+        }
+    };
+    field.store(gpa, Ordering::Relaxed);
+
+    let split_queue = SplitQueue::new(reg, &ram, false);
+    if valid {
+        assert_matches!(split_queue, Ok(Some(_)));
+    } else {
+        assert_matches!(
+            split_queue,
+            Err(Error::Memory { source, .. }) if matches!(*source, mem::Error::NotContinuous { .. })
+        );
+    }
+}
+
 #[test]
 fn enabled_queue() {
     let ram_bus = fixture_ram_bus();

alioth/src/virtio/virtio.rs

@@ -67,6 +67,8 @@ pub enum Error {
     WorkerThread { error: std::io::Error },
     #[snafu(display("Invalid descriptor id {id}"))]
     InvalidDescriptor { id: u16 },
+    #[snafu(display("Descriptor chain is longer than the queue size {size}"))]
+    DescChainTooLong { size: u16 },
     #[snafu(display("Invalid queue index {index}"))]
     InvalidQueueIndex { index: u16 },
     #[snafu(display("Invalid queue size {size}"))]

alioth/src/virtio/virtio_test.rs

@@ -23,7 +23,7 @@ use crate::virtio::queue::{QUEUE_SIZE_MAX, QueueReg};
 use crate::virtio::{DevStatus, IrqSender, Result};
 
 pub const QUEUE_SIZE: u16 = QUEUE_SIZE_MAX;
-const MEM_SIZE: usize = 2 << 20;
+pub const MEM_SIZE: usize = 2 << 20;
 pub const DATA_ADDR: u64 = 0x0;
 const QUEUE_START: u64 = 1 << 20;