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, .. }) + ); +} 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/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.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}"))] 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;