From 147ab97c0a09d5fb25930abd413dcd358fd27545 Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Mon, 9 Nov 2026 11:18:59 -0800 Subject: [PATCH 1/3] fix(mem): check every element behind a guest memory pointer Ram::get_ptr() only checks that one value of the pointee type is backed by host memory, while callers like virtqueues use the pointer for an array of values. Its alignment check also tests the byte pointer before the cast, so it never fails. Add Ram::get_array_ptr(), which checks that all the values are in one contiguous host mapping, and that the pointer is aligned for the pointee type. Ram::get_ptr() becomes the case of a single value, which fixes its alignment check. Signed-off-by: Changyuan Lyu --- alioth/src/mem/mapped.rs | 23 +++++++++++------- alioth/src/mem/mapped_test.rs | 45 +++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/alioth/src/mem/mapped.rs b/alioth/src/mem/mapped.rs index 6e7f6894..c6fea98b 100644 --- a/alioth/src/mem/mapped.rs +++ b/alioth/src/mem/mapped.rs @@ -287,14 +287,19 @@ impl Ram { } pub fn get_ptr(&self, gpa: u64) -> Result<*mut T, Error> { - let host_ref = self.get_partial_slice_mut(gpa, size_of::() as u64)?; - let ptr = host_ref.as_mut_ptr(); - if host_ref.len() != size_of::() { - error::NotContinuous { - addr: gpa, - size: size_of::() 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(&self, gpa: u64, len: u64) -> Result<*mut T, Error> { + let size = len.saturating_mul(size_of::() 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, @@ -302,7 +307,7 @@ impl Ram { } .fail() } else { - Ok(ptr as *mut T) + Ok(ptr) } } diff --git a/alioth/src/mem/mapped_test.rs b/alioth/src/mem/mapped_test.rs index 0c9bfaa5..5bc7f2b5 100644 --- a/alioth/src/mem/mapped_test.rs +++ b/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::(PAGE_SIZE - 16, 2).unwrap(); + assert_eq!(ptr, ram.get_ptr::(PAGE_SIZE - 16).unwrap()); + + // The last element is in the other host mapping. + assert_matches!( + ram.get_array_ptr::(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::(2 * PAGE_SIZE - 8, 2), + Err(Error::NotContinuous { .. }) + ); + assert_matches!( + ram.get_array_ptr::(2 * PAGE_SIZE, 1), + Err(Error::NotMapped { .. }) + ); + assert_matches!( + ram.get_array_ptr::(0, u64::MAX), + Err(Error::NotContinuous { .. }) + ); + + // Pointers must be aligned for the type. + assert_matches!( + ram.get_array_ptr::(4, 2), + Err(Error::NotAligned { align: 8, .. }) + ); + assert_matches!( + ram.get_ptr::(4), + Err(Error::NotAligned { align: 8, .. }) + ); +} From eaee9f0540f4b9622ee2281811e47f53c09636df Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Mon, 9 Nov 2026 11:19:43 -0800 Subject: [PATCH 2/3] fix(virtio): check whole split virtqueue rings in guest memory SplitQueue::new() checks only the first entry of the descriptor table, the available ring and the used ring, but the queue accesses entries up to the queue size through the same pointers. A driver that places a ring at the end of guest memory, or across two memory regions, makes the worker read and write host memory beyond the guest memory mapping. Check that each ring is backed by one contiguous host mapping in full. Fixes: fc059ad17176 ("perf(virtio): lock memory layout while running") Signed-off-by: Changyuan Lyu --- alioth/src/virtio/queue/split.rs | 8 ++-- alioth/src/virtio/queue/split_test.rs | 64 ++++++++++++++++++++++++++- alioth/src/virtio/virtio_test.rs | 2 +- 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/alioth/src/virtio/queue/split.rs b/alioth/src/virtio/queue/split.rs index 87d2131d..e525c469 100644 --- a/alioth/src/virtio/queue/split.rs +++ b/alioth/src/virtio/queue/split.rs @@ -152,15 +152,17 @@ impl<'m> SplitQueue<'m> { let used_hdr = ram.get_ptr::(used)?; let avail_ring_gpa = avail + size_of::() as u64; let used_ring_gpa = used + size_of::() 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, })) } diff --git a/alioth/src/virtio/queue/split_test.rs b/alioth/src/virtio/queue/split_test.rs index 2d9a71cf..1b608073 100644 --- a/alioth/src/virtio/queue/split_test.rs +++ b/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 => (®.desc, end - entries * size_of::() as u64), + Ring::Avail => { + let ring_size = entries * size_of::() as u64; + ( + ®.driver, + end - ring_size - size_of::() as u64, + ) + } + Ring::Used => { + let ring_size = entries * size_of::() as u64; + ( + ®.device, + end - ring_size - size_of::() 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(); diff --git a/alioth/src/virtio/virtio_test.rs b/alioth/src/virtio/virtio_test.rs index 61babf41..fd2a34fa 100644 --- a/alioth/src/virtio/virtio_test.rs +++ b/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; From 474cc60bf36bb89a5226a4031774c3e47828a219 Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Mon, 9 Nov 2026 11:21:16 -0800 Subject: [PATCH 3/3] fix(virtio): check whole packed virtqueue rings in guest memory Like SplitQueue::new() did, PackedQueue::new() checks only the first descriptor of the ring, but the queue accesses descriptors up to the queue size. A driver that places the ring at the end of guest memory, or across two memory regions, makes the worker read and write host memory beyond the guest memory mapping. Check that the whole ring is backed by one contiguous host mapping, and reject queue sizes above 2^15, the maximum for packed virtqueues, which is also all that the 15-bit ring offset in WrappedIndex can address. Also stop at a descriptor chain as long as the ring, since a chain takes consecutive descriptors. Otherwise, a driver that sets VIRTQ_DESC_F_NEXT in every descriptor makes the worker loop forever, and a driver that changes the flags while the worker walks the chain can end it after more descriptors than the ring has, which moves the offset of the next chain beyond the ring. Fixes: f97eb722e6fa ("feat(virtio): support packed queue") Signed-off-by: Changyuan Lyu --- alioth/src/virtio/queue/packed.rs | 17 +++++- alioth/src/virtio/queue/packed_test.rs | 71 +++++++++++++++++++++++--- alioth/src/virtio/virtio.rs | 2 + 3 files changed, 82 insertions(+), 8 deletions(-) diff --git a/alioth/src/virtio/queue/packed.rs b/alioth/src/virtio/queue/packed.rs index c0578f4a..82210660 100644 --- a/alioth/src/virtio/queue/packed.rs +++ b/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 { diff --git a/alioth/src/virtio/queue/packed_test.rs b/alioth/src/virtio/queue/packed_test.rs index 4d88a391..62c5a157 100644 --- a/alioth/src/virtio/queue/packed_test.rs +++ b/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::() 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] diff --git a/alioth/src/virtio/virtio.rs b/alioth/src/virtio/virtio.rs index bec52587..b908df65 100644 --- a/alioth/src/virtio/virtio.rs +++ b/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}"))]