Limit virtqueue descriptor chains to the queue size

alioth/src/virtio/queue/packed.rs

@@ -166,7 +166,19 @@ impl<'m> VirtQueue<'m> for PackedQueue<'m> {
             let desc = unsafe { self.desc.offset(offset as isize).read_volatile() };
             let flag = DescFlag::from_bits_retain(desc.flag.into_inner());
             if flag.contains(DescFlag::INDIRECT) {
-                for i in 0..(desc.len as usize / size_of::<Desc>()) {
+                // An indirect descriptor cannot be in a list (VirtIO spec
+                // 1.4, Sec 2.8.19).
+                if delta > 0 || flag.contains(DescFlag::NEXT) {
+                    return error::UnexpectedIndirectDesc.fail();
+                }
+                let num = desc.len as usize / size_of::<Desc>();
+                // A driver cannot create a chain longer than allowed by the
+                // device (VirtIO spec 1.4, Sec 2.8.19). Like a split queue,
+                // allow up to the queue size.
+                if num > self.size as usize {
+                    return error::DescChainTooLong { size: self.size }.fail();
+                }
+                for i in 0..num {
                     let addr = desc.addr + (i * size_of::<Desc>()) as u64;
                     let desc: Desc = ram.read_t(addr)?;
                     let flag = DescFlag::from_bits_retain(desc.flag.into_inner());

alioth/src/virtio/queue/packed_test.rs

@@ -20,6 +20,7 @@ use assert_matches::assert_matches;
 use rstest::rstest;
 
 use crate::mem;
+use crate::mem::mapped::Ram;
 use crate::virtio::Error;
 use crate::virtio::queue::packed::{Desc, DescEvent, EventFlag, PackedQueue, WrappedIndex};
 use crate::virtio::queue::tests::{GuestQueue, UsedDesc, VirtQueueGuest};
@@ -218,6 +219,78 @@ fn chain_longer_than_ring() {
     );
 }
 
+const TABLE_ADDR: u64 = DATA_ADDR + 0x1000;
+
+/// Makes descriptor `offset` of the ring available.
+fn set_avail_desc(q: &PackedQueue, offset: u16, addr: u64, len: u32, flag: DescFlag) {
+    let desc = unsafe { &mut *q.desc.offset(offset as isize) };
+    desc.addr = addr;
+    desc.len = len;
+    desc.flag
+        .store((flag | DescFlag::AVAIL).bits(), Ordering::Release);
+}
+
+/// Writes an indirect table of `num` readable descriptors.
+fn write_indirect_table(ram: &Ram, num: u16) {
+    let table = ram.get_array_ptr::<Desc>(TABLE_ADDR, num as u64).unwrap();
+    for i in 0..num as usize {
+        let desc = Desc {
+            addr: DATA_ADDR,
+            len: 1,
+            ..Default::default()
+        };
+        unsafe { table.add(i).write(desc) };
+    }
+}
+
+#[rstest]
+#[case::with_next(DescFlag::INDIRECT | DescFlag::NEXT, DescFlag::empty())]
+#[case::after_next(DescFlag::NEXT, DescFlag::INDIRECT)]
+fn unexpected_indirect_desc(#[case] flag_0: DescFlag, #[case] flag_1: DescFlag) {
+    let ram_bus = fixture_ram_bus();
+    let queues = fixture_queues(1);
+    let ram = ram_bus.lock_layout();
+    let q = PackedQueue::new(&queues[0], &ram, false).unwrap().unwrap();
+
+    write_indirect_table(&ram, 1);
+    // Descriptors 0 and 1 form a list, which has an indirect descriptor.
+    for (offset, flag) in [(0, flag_0), (1, flag_1)] {
+        let (addr, len) = if flag.contains(DescFlag::INDIRECT) {
+            (TABLE_ADDR, size_of::<Desc>() as u32)
+        } else {
+            (DATA_ADDR, 1)
+        };
+        set_avail_desc(&q, offset, addr, len, flag);
+    }
+    assert_matches!(
+        q.get_avail(WrappedIndex::INIT, &ram),
+        Err(Error::UnexpectedIndirectDesc { .. })
+    );
+}
+
+#[rstest]
+#[case(4, true)]
+#[case(5, false)]
+#[case(u32::MAX / size_of::<Desc>() as u32, false)]
+fn indirect_table_len(#[case] num: u32, #[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(4, Ordering::Relaxed);
+    let q = PackedQueue::new(reg, &ram, false).unwrap().unwrap();
+
+    write_indirect_table(&ram, 5);
+    let len = num * size_of::<Desc>() as u32;
+    set_avail_desc(&q, 0, TABLE_ADDR, len, DescFlag::INDIRECT);
+    let chain = q.get_avail(WrappedIndex::INIT, &ram);
+    if valid {
+        assert_matches!(chain, Ok(Some(c)) if c.readable.len() == num as usize);
+    } else {
+        assert_matches!(chain, Err(Error::DescChainTooLong { size: 4, .. }));
+    }
+}
+
 #[test]
 fn enabled_queue() {
     let ram_bus = fixture_ram_bus();

alioth/src/virtio/queue/split.rs

@@ -120,6 +120,16 @@ impl SplitQueue<'_> {
             error::InvalidDescriptor { id }.fail()
         }
     }
+
+    /// Returns descriptor `id` in the table that `indirect` refers to.
+    fn get_indirect_desc(ram: &Ram, indirect: &Desc, id: u16) -> Result<Desc> {
+        if (id as u32) < indirect.len / size_of::<Desc>() as u32 {
+            let addr = indirect.addr + id as u64 * size_of::<Desc>() as u64;
+            Ok(ram.read_t(addr)?)
+        } else {
+            error::InvalidDescriptor { id }.fail()
+        }
+    }
 }
 
 impl<'m> SplitQueue<'m> {
@@ -191,37 +201,40 @@ impl<'m> VirtQueue<'m> for SplitQueue<'m> {
                 .read_volatile()
         };
         let mut id = head_id;
+        // After an indirect descriptor, `id` indexes the table it refers to.
+        let mut indirect: Option<Desc> = None;
         loop {
-            let desc = self.get_desc(id)?;
+            let desc = match &indirect {
+                None => self.get_desc(id)?,
+                Some(indirect) => Self::get_indirect_desc(ram, indirect, id)?,
+            };
             let flag = DescFlag::from_bits_retain(desc.flag);
             if flag.contains(DescFlag::INDIRECT) {
-                let mut id = 0;
-                loop {
-                    let addr = desc.addr + id as u64 * size_of::<Desc>() as u64;
-                    let desc: Desc = ram.read_t(addr)?;
-                    let flag = DescFlag::from_bits_retain(desc.flag);
-                    assert!(!flag.contains(DescFlag::INDIRECT));
-                    if flag.contains(DescFlag::WRITE) {
-                        writable.push((desc.addr, desc.len as u64));
-                    } else {
-                        readable.push((desc.addr, desc.len as u64));
-                    }
-                    if flag.contains(DescFlag::NEXT) {
-                        id = desc.next;
-                    } else {
-                        break;
-                    }
+                // An indirect descriptor ends a chain in the descriptor
+                // table, and an indirect table cannot refer to another one
+                // (VirtIO spec 1.4, Sec 2.7.5.3.1).
+                if flag.contains(DescFlag::NEXT) || indirect.is_some() {
+                    return error::UnexpectedIndirectDesc.fail();
                 }
-            } else if flag.contains(DescFlag::WRITE) {
+                indirect = Some(desc);
+                id = 0;
+                continue;
+            }
+            // A chain cannot be longer than the queue size, counting the
+            // descriptors in an indirect table (VirtIO spec 1.4, Sec
+            // 2.7.5.3.1). This also catches loops in the chain.
+            if readable.len() + writable.len() >= self.size as usize {
+                return error::DescChainTooLong { size: self.size }.fail();
+            }
+            if flag.contains(DescFlag::WRITE) {
                 writable.push((desc.addr, desc.len as u64));
             } else {
                 readable.push((desc.addr, desc.len as u64));
             }
-            if flag.contains(DescFlag::NEXT) {
-                id = desc.next;
-            } else {
+            if !flag.contains(DescFlag::NEXT) {
                 break;
             }
+            id = desc.next;
         }
         let readable = ram.translate_iov(&readable)?;
         let writable = ram.translate_iov_mut(&writable)?;

alioth/src/virtio/queue/split_test.rs

@@ -18,6 +18,7 @@ use std::sync::atomic::Ordering;
 
 use assert_matches::assert_matches;
 use rstest::rstest;
+use zerocopy::IntoBytes;
 
 use crate::mem;
 use crate::virtio::Error;
@@ -254,3 +255,180 @@ fn event_idx_enabled() {
         12
     );
 }
+
+const TABLE_ADDR: u64 = DATA_ADDR + 0x1000;
+
+fn desc(addr: u64, len: u32, flag: DescFlag, next: u16) -> Desc {
+    Desc {
+        addr,
+        len,
+        flag: flag.bits(),
+        next,
+    }
+}
+
+/// Places `descs` at the start of the descriptor table and makes the chain
+/// starting at descriptor 0 available.
+fn add_chain(q: &SplitQueue, descs: &[Desc]) {
+    for (id, desc) in descs.iter().enumerate() {
+        *unsafe { &mut *q.desc.add(id) } = *desc;
+    }
+    *unsafe { &mut *q.avail_ring } = 0;
+    unsafe { &*q.avail_hdr }.idx.store(1, Ordering::Release);
+}
+
+#[rstest]
+#[case::with_next(DescFlag::INDIRECT | DescFlag::NEXT, DescFlag::empty())]
+#[case::in_indirect_table(DescFlag::INDIRECT, DescFlag::INDIRECT)]
+fn unexpected_indirect_desc(#[case] flag: DescFlag, #[case] table_flag: DescFlag) {
+    let ram_bus = fixture_ram_bus();
+    let queues = fixture_queues(1);
+    let ram = ram_bus.lock_layout();
+    let q = SplitQueue::new(&queues[0], &ram, false).unwrap().unwrap();
+
+    let table = [desc(DATA_ADDR, 1, table_flag, 0)];
+    ram.write_t(TABLE_ADDR, &table).unwrap();
+    add_chain(
+        &q,
+        &[
+            desc(TABLE_ADDR, size_of_val(&table) as u32, flag, 1),
+            desc(DATA_ADDR, 1, DescFlag::empty(), 0),
+        ],
+    );
+    assert_matches!(
+        q.get_avail(0, &ram),
+        Err(Error::UnexpectedIndirectDesc { .. })
+    );
+}
+
+/// Returns `n` readable descriptors linked in order.
+fn linked_descs(n: u16) -> Vec<Desc> {
+    (0..n)
+        .map(|id| {
+            let flag = if id + 1 < n {
+                DescFlag::NEXT
+            } else {
+                DescFlag::empty()
+            };
+            desc(DATA_ADDR, 1, flag, id + 1)
+        })
+        .collect()
+}
+
+#[test]
+fn indirect_chain() {
+    let ram_bus = fixture_ram_bus();
+    let queues = fixture_queues(1);
+    let ram = ram_bus.lock_layout();
+    let q = SplitQueue::new(&queues[0], &ram, false).unwrap().unwrap();
+
+    ram.write(DATA_ADDR, b"abcdefg").unwrap();
+    // A readable descriptor in the descriptor table, followed by an
+    // indirect table of a readable and a writable descriptor.
+    let table = [
+        desc(DATA_ADDR + 1, 2, DescFlag::NEXT, 1),
+        desc(DATA_ADDR + 3, 4, DescFlag::WRITE, 0),
+    ];
+    ram.write_t(TABLE_ADDR, &table).unwrap();
+    add_chain(
+        &q,
+        &[
+            desc(DATA_ADDR, 1, DescFlag::NEXT, 1),
+            desc(
+                TABLE_ADDR,
+                size_of_val(&table) as u32,
+                DescFlag::INDIRECT,
+                0,
+            ),
+        ],
+    );
+    let chain = q.get_avail(0, &ram).unwrap().unwrap();
+    assert_eq!(chain.id, 0);
+    assert_eq!(chain.readable.len(), 2);
+    assert_eq!(&*chain.readable[0], b"a");
+    assert_eq!(&*chain.readable[1], b"bc");
+    assert_eq!(chain.writable.len(), 1);
+    assert_eq!(&*chain.writable[0], b"defg");
+}
+
+#[rstest]
+#[case(4, 0, true)]
+#[case(2, 2, true)]
+#[case(0, 4, true)]
+#[case(2, 3, false)]
+#[case(0, 5, false)]
+fn chain_len(#[case] direct: u16, #[case] indirect: 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(4, Ordering::Relaxed);
+    let q = SplitQueue::new(reg, &ram, false).unwrap().unwrap();
+
+    // `direct` descriptors in the descriptor table, followed by an indirect
+    // descriptor referring to a table of `indirect` descriptors.
+    let mut descs = linked_descs(direct + (indirect > 0) as u16);
+    if indirect > 0 {
+        let table = linked_descs(indirect);
+        ram.write(TABLE_ADDR, table.as_bytes()).unwrap();
+        let len = (table.len() * size_of::<Desc>()) as u32;
+        *descs.last_mut().unwrap() = desc(TABLE_ADDR, len, DescFlag::INDIRECT, 0);
+    }
+    add_chain(&q, &descs);
+
+    let chain = q.get_avail(0, &ram);
+    if valid {
+        let len = (direct + indirect) as usize;
+        assert_matches!(chain, Ok(Some(c)) if c.readable.len() == len);
+    } else {
+        assert_matches!(chain, Err(Error::DescChainTooLong { size: 4, .. }));
+    }
+}
+
+#[rstest]
+#[case::in_desc_table(false)]
+#[case::in_indirect_table(true)]
+fn chain_with_loop(#[case] indirect: bool) {
+    let ram_bus = fixture_ram_bus();
+    let queues = fixture_queues(1);
+    let ram = ram_bus.lock_layout();
+    let q = SplitQueue::new(&queues[0], &ram, false).unwrap().unwrap();
+
+    // Descriptors 0 and 1 link to each other, so the chain never ends.
+    let descs = [
+        desc(DATA_ADDR, 1, DescFlag::NEXT, 1),
+        desc(DATA_ADDR, 1, DescFlag::NEXT, 0),
+    ];
+    if indirect {
+        ram.write_t(TABLE_ADDR, &descs).unwrap();
+        let len = size_of_val(&descs) as u32;
+        add_chain(&q, &[desc(TABLE_ADDR, len, DescFlag::INDIRECT, 0)]);
+    } else {
+        add_chain(&q, &descs);
+    }
+    assert_matches!(
+        q.get_avail(0, &ram),
+        Err(Error::DescChainTooLong {
+            size: QUEUE_SIZE,
+            ..
+        })
+    );
+}
+
+#[test]
+fn desc_out_of_indirect_table() {
+    let ram_bus = fixture_ram_bus();
+    let queues = fixture_queues(1);
+    let ram = ram_bus.lock_layout();
+    let q = SplitQueue::new(&queues[0], &ram, false).unwrap().unwrap();
+
+    // The indirect table has only descriptor 0, which links to descriptor 1.
+    let table = linked_descs(2);
+    ram.write(TABLE_ADDR, table.as_bytes()).unwrap();
+    let len = size_of::<Desc>() as u32;
+    add_chain(&q, &[desc(TABLE_ADDR, len, DescFlag::INDIRECT, 0)]);
+    assert_matches!(
+        q.get_avail(0, &ram),
+        Err(Error::InvalidDescriptor { id: 1, .. })
+    );
+}

alioth/src/virtio/virtio.rs

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