From d2ffe9b66e8de395ca259446ede5170c08758400 Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Mon, 9 Nov 2026 14:14:17 -0800 Subject: [PATCH 1/3] fix(virtio): reject misplaced indirect descriptors in split queues An indirect descriptor can only be the last descriptor of a chain in the descriptor table: it cannot have VIRTQ_DESC_F_NEXT set or be in an indirect table (VirtIO spec 1.4, Sec 2.7.5.3.1). SplitQueue::get_avail() asserted the latter, so a guest could panic the device worker with an indirect descriptor in an indirect table. Return an error in both cases instead. Fixes: bd56123919ee ("feat(virtio): implement split queue operations") Signed-off-by: Changyuan Lyu --- alioth/src/virtio/queue/split.rs | 10 +++++- alioth/src/virtio/queue/split_test.rs | 45 +++++++++++++++++++++++++++ alioth/src/virtio/virtio.rs | 2 ++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/alioth/src/virtio/queue/split.rs b/alioth/src/virtio/queue/split.rs index e525c469..f3fa60ad 100644 --- a/alioth/src/virtio/queue/split.rs +++ b/alioth/src/virtio/queue/split.rs @@ -195,12 +195,20 @@ impl<'m> VirtQueue<'m> for SplitQueue<'m> { let desc = self.get_desc(id)?; let flag = DescFlag::from_bits_retain(desc.flag); if flag.contains(DescFlag::INDIRECT) { + // An indirect descriptor ends a chain in the descriptor + // table (VirtIO spec 1.4, Sec 2.7.5.3.1). + if flag.contains(DescFlag::NEXT) { + return error::UnexpectedIndirectDesc.fail(); + } let mut id = 0; loop { let addr = desc.addr + id as u64 * size_of::() as u64; let desc: Desc = ram.read_t(addr)?; let flag = DescFlag::from_bits_retain(desc.flag); - assert!(!flag.contains(DescFlag::INDIRECT)); + // An indirect table cannot refer to another one. + if flag.contains(DescFlag::INDIRECT) { + return error::UnexpectedIndirectDesc.fail(); + } if flag.contains(DescFlag::WRITE) { writable.push((desc.addr, desc.len as u64)); } else { diff --git a/alioth/src/virtio/queue/split_test.rs b/alioth/src/virtio/queue/split_test.rs index 1b608073..5ad10341 100644 --- a/alioth/src/virtio/queue/split_test.rs +++ b/alioth/src/virtio/queue/split_test.rs @@ -254,3 +254,48 @@ 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 { .. }) + ); +} diff --git a/alioth/src/virtio/virtio.rs b/alioth/src/virtio/virtio.rs index b908df65..b566d594 100644 --- a/alioth/src/virtio/virtio.rs +++ b/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}"))] From 1b997ad8a0f1db19cc33059c833d02d8e18e8523 Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Mon, 9 Nov 2026 14:16:25 -0800 Subject: [PATCH 2/3] fix(virtio): limit split descriptor chains to the queue size A driver cannot create a descriptor chain longer than the queue size, counting the descriptors in an indirect table (VirtIO spec 1.4, Sec 2.7.5.3.1). However, SplitQueue::get_avail() follows `next` without a limit, so with a loop in the descriptor table or in an indirect table, a guest could keep the device worker spinning and growing the buffer lists until the host runs out of memory. Fail a chain once it has more descriptors than the queue size. Also take only the descriptors within the length of an indirect table. Fixes: bd56123919ee ("feat(virtio): implement split queue operations") Signed-off-by: Changyuan Lyu --- alioth/src/virtio/queue/split.rs | 59 ++++++------ alioth/src/virtio/queue/split_test.rs | 133 ++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 27 deletions(-) diff --git a/alioth/src/virtio/queue/split.rs b/alioth/src/virtio/queue/split.rs index f3fa60ad..11a8931d 100644 --- a/alioth/src/virtio/queue/split.rs +++ b/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 { + if (id as u32) < indirect.len / size_of::() as u32 { + let addr = indirect.addr + id as u64 * size_of::() as u64; + Ok(ram.read_t(addr)?) + } else { + error::InvalidDescriptor { id }.fail() + } + } } impl<'m> SplitQueue<'m> { @@ -191,45 +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 = 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) { // An indirect descriptor ends a chain in the descriptor - // table (VirtIO spec 1.4, Sec 2.7.5.3.1). - if flag.contains(DescFlag::NEXT) { + // 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(); } - let mut id = 0; - loop { - let addr = desc.addr + id as u64 * size_of::() as u64; - let desc: Desc = ram.read_t(addr)?; - let flag = DescFlag::from_bits_retain(desc.flag); - // An indirect table cannot refer to another one. - if flag.contains(DescFlag::INDIRECT) { - return error::UnexpectedIndirectDesc.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 { - break; - } - } - } 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)?; diff --git a/alioth/src/virtio/queue/split_test.rs b/alioth/src/virtio/queue/split_test.rs index 5ad10341..ce181246 100644 --- a/alioth/src/virtio/queue/split_test.rs +++ b/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; @@ -299,3 +300,135 @@ fn unexpected_indirect_desc(#[case] flag: DescFlag, #[case] table_flag: DescFlag Err(Error::UnexpectedIndirectDesc { .. }) ); } + +/// Returns `n` readable descriptors linked in order. +fn linked_descs(n: u16) -> Vec { + (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::()) 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::() as u32; + add_chain(&q, &[desc(TABLE_ADDR, len, DescFlag::INDIRECT, 0)]); + assert_matches!( + q.get_avail(0, &ram), + Err(Error::InvalidDescriptor { id: 1, .. }) + ); +} From 70f98ffdc4f845587c71c40c6473ce505835657b Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Mon, 9 Nov 2026 14:18:07 -0800 Subject: [PATCH 3/3] fix(virtio): limit packed indirect tables to the queue size PackedQueue::get_avail() takes every descriptor in an indirect table, whose length comes from the guest, and accepts an indirect descriptor at each descriptor of a list. With a list of indirect descriptors referring to a large table, a guest could make the device worker spend CPU time and host memory far beyond the size of the guest memory. An indirect descriptor cannot be in a list (VirtIO spec 1.4, Sec 2.8.19), so reject it there. The same section lets the device decide the maximum length of a chain; like a split queue, limit an indirect table to the queue size. Fixes: f97eb722e6fa ("feat(virtio): support packed queue") Signed-off-by: Changyuan Lyu --- alioth/src/virtio/queue/packed.rs | 14 ++++- alioth/src/virtio/queue/packed_test.rs | 73 ++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/alioth/src/virtio/queue/packed.rs b/alioth/src/virtio/queue/packed.rs index 82210660..67a0fb80 100644 --- a/alioth/src/virtio/queue/packed.rs +++ b/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::()) { + // 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::(); + // 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::()) as u64; let desc: Desc = ram.read_t(addr)?; let flag = DescFlag::from_bits_retain(desc.flag.into_inner()); diff --git a/alioth/src/virtio/queue/packed_test.rs b/alioth/src/virtio/queue/packed_test.rs index 62c5a157..8f15a3d3 100644 --- a/alioth/src/virtio/queue/packed_test.rs +++ b/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::(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::() 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::() 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::() 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();