From 2a5e090217e4aecd47f3e94f42614e25590f18ec Mon Sep 17 00:00:00 2001 From: German Maglione Date: Fri, 29 Oct 2021 19:07:56 +0200 Subject: [PATCH] Moves specification-required checks to setters Moves the mandatory checks, according to the virtio specification, from Queue::is_valid() to the corresponding Queue setters. closes: #94 Signed-off-by: German Maglione --- crates/virtio-device/src/mmio.rs | 34 +++-- crates/virtio-device/src/virtio_config.rs | 5 +- crates/virtio-queue/src/lib.rs | 162 ++++++++++++++-------- 3 files changed, 125 insertions(+), 76 deletions(-) diff --git a/crates/virtio-device/src/mmio.rs b/crates/virtio-device/src/mmio.rs index ee024c84..0faca651 100644 --- a/crates/virtio-device/src/mmio.rs +++ b/crates/virtio-device/src/mmio.rs @@ -9,11 +9,11 @@ use std::convert::TryInto; use std::sync::atomic::Ordering; -use log::warn; +use log::{error, warn}; use vm_memory::GuestAddressSpace; use crate::{status, WithDriverSelect}; -use virtio_queue::Queue; +use virtio_queue::{Error, Queue}; // Required by the Virtio MMIO device register layout at offset 0 from base. Turns out this // is actually the ASCII sequence for "virt" (in little endian ordering). @@ -35,11 +35,14 @@ fn update_queue_field(device: &mut D, f: F) where M: GuestAddressSpace, D: WithDriverSelect + ?Sized, - F: FnOnce(&mut Queue), + F: FnOnce(&mut Queue) -> Result<(), Error>, { if device.check_device_status(status::FEATURES_OK, status::DRIVER_OK | status::FAILED) { if let Some(queue) = device.selected_queue_mut() { - f(queue); + if let Err(e) = f(queue) { + device.set_device_status(status::FAILED); + error!("invalid queue update: {}", e); + } } else { warn!("update invalid virtio queue"); } @@ -150,7 +153,10 @@ pub trait VirtioMmioDevice: WithDriverSelect { // for now). 0x30 => self.set_queue_select(v as u16), 0x38 => update_queue_field(self, |q| q.set_size(v as u16)), - 0x44 => update_queue_field(self, |q| q.set_ready(v == 1)), + 0x44 => update_queue_field(self, |q| { + q.set_ready(v == 1); + Ok(()) + }), 0x50 => self.queue_notify(v), 0x64 => { if self.check_device_status(status::DRIVER_OK, 0) { @@ -272,22 +278,22 @@ mod tests { assert_eq!(d.last_queue_notify, 2); assert_eq!(d.cfg.queues[0].state.desc_table.0, 0); - d.write(0x80, &1u32.to_le_bytes()); - assert_eq!(d.cfg.queues[0].state.desc_table.0, 1); + d.write(0x80, &16u32.to_le_bytes()); + assert_eq!(d.cfg.queues[0].state.desc_table.0, 16); d.write(0x84, &2u32.to_le_bytes()); - assert_eq!(d.cfg.queues[0].state.desc_table.0, (2 << 32) + 1); + assert_eq!(d.cfg.queues[0].state.desc_table.0, (2 << 32) + 16); assert_eq!(d.cfg.queues[0].state.avail_ring.0, 0); - d.write(0x90, &1u32.to_le_bytes()); - assert_eq!(d.cfg.queues[0].state.avail_ring.0, 1); + d.write(0x90, &2u32.to_le_bytes()); + assert_eq!(d.cfg.queues[0].state.avail_ring.0, 2); d.write(0x94, &2u32.to_le_bytes()); - assert_eq!(d.cfg.queues[0].state.avail_ring.0, (2 << 32) + 1); + assert_eq!(d.cfg.queues[0].state.avail_ring.0, (2 << 32) + 2); assert_eq!(d.cfg.queues[0].state.used_ring.0, 0); - d.write(0xa0, &1u32.to_le_bytes()); - assert_eq!(d.cfg.queues[0].state.used_ring.0, 1); + d.write(0xa0, &4u32.to_le_bytes()); + assert_eq!(d.cfg.queues[0].state.used_ring.0, 4); d.write(0xa4, &2u32.to_le_bytes()); - assert_eq!(d.cfg.queues[0].state.used_ring.0, (2 << 32) + 1); + assert_eq!(d.cfg.queues[0].state.used_ring.0, (2 << 32) + 4); // Let's select a non-existent queue. d.write(0x30, &1u32.to_le_bytes()); diff --git a/crates/virtio-device/src/virtio_config.rs b/crates/virtio-device/src/virtio_config.rs index a6b5575a..ff31c8af 100644 --- a/crates/virtio-device/src/virtio_config.rs +++ b/crates/virtio-device/src/virtio_config.rs @@ -251,8 +251,9 @@ pub(crate) mod tests { impl Dummy { pub fn new(device_type: u32, features: u64, config_space: Vec) -> Self { - let mem = - Arc::new(GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x1000_0000)]).unwrap()); + let mem = Arc::new( + GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x10_0000_0000)]).unwrap(), + ); let queue = Queue::new(mem, 256); let cfg = VirtioConfig::new(features, vec![queue], config_space); diff --git a/crates/virtio-queue/src/lib.rs b/crates/virtio-queue/src/lib.rs index c2b45471..2a8391f9 100644 --- a/crates/virtio-queue/src/lib.rs +++ b/crates/virtio-queue/src/lib.rs @@ -54,6 +54,14 @@ pub enum Error { InvalidChain, /// Invalid descriptor index. InvalidDescriptorIndex, + /// Invalid queue size. + InvalidQueueSize, + /// Invalid descriptor table address alignment + InvalidDescriptorTableAddressAlignment, + /// Invalid available ring address alignment + InvalidAvailableRingAddressAlignment, + /// Invalid Used ring address alignment + InvalidUsedRingAddressAlignment, } impl Display for Error { @@ -66,6 +74,16 @@ impl Display for Error { InvalidIndirectDescriptor => write!(f, "invalid indirect descriptor"), InvalidIndirectDescriptorTable => write!(f, "invalid indirect descriptor table"), InvalidDescriptorIndex => write!(f, "invalid descriptor index"), + InvalidQueueSize => write!(f, "invalid queue size"), + InvalidDescriptorTableAddressAlignment => { + write!(f, "invalid descriptor table address alignment") + } + InvalidAvailableRingAddressAlignment => { + write!(f, "invalid available ring address alignment") + } + InvalidUsedRingAddressAlignment => { + write!(f, "invalid used ring address alignment") + } } } } @@ -467,7 +485,7 @@ pub trait QueueStateT { /// /// The `size` should power of two and less than or equal to value reported by `max_size()`, /// otherwise it will panic. - fn set_size(&mut self, size: u16); + fn set_size(&mut self, size: u16) -> Result<(), Error>; /// Check whether the queue is ready to be processed. fn ready(&self) -> bool; @@ -479,19 +497,19 @@ pub trait QueueStateT { /// /// The descriptor table address is 64-bit, the corresponding part will be updated if 'low' /// and/or `high` is valid. - fn set_desc_table_address(&mut self, low: Option, high: Option); + fn set_desc_table_address(&mut self, low: Option, high: Option) -> Result<(), Error>; /// Set available ring address for the queue. /// /// The available ring address is 64-bit, the corresponding part will be updated if 'low' /// and/or `high` is valid. - fn set_avail_ring_address(&mut self, low: Option, high: Option); + fn set_avail_ring_address(&mut self, low: Option, high: Option) -> Result<(), Error>; /// Set used ring address for the queue. /// /// The used ring address is 64-bit, the corresponding part will be updated if 'low' /// and/or `high` is valid. - fn set_used_ring_address(&mut self, low: Option, high: Option); + fn set_used_ring_address(&mut self, low: Option, high: Option) -> Result<(), Error>; /// Enable/disable the VIRTIO_F_RING_EVENT_IDX feature for interrupt coalescing. fn set_event_idx(&mut self, enabled: bool); @@ -667,10 +685,6 @@ impl QueueStateT for QueueState { if !self.ready { error!("attempt to use virtio queue that is not marked ready"); false - } else if self.size > self.max_size || self.size == 0 || (self.size & (self.size - 1)) != 0 - { - error!("virtio queue with invalid size: {}", self.size); - false } else if desc_table .checked_add(desc_table_size) .map_or(true, |v| !mem.address_in_range(v)) @@ -701,15 +715,6 @@ impl QueueStateT for QueueState { used_ring_size ); false - } else if desc_table.mask(0xf) != 0 { - error!("virtio queue descriptor table breaks alignment contraints"); - false - } else if avail_ring.mask(0x1) != 0 { - error!("virtio queue available ring breaks alignment contraints"); - false - } else if used_ring.mask(0x3) != 0 { - error!("virtio queue used ring breaks alignment contraints"); - false } else { true } @@ -739,8 +744,14 @@ impl QueueStateT for QueueState { min(self.size, self.max_size) } - fn set_size(&mut self, size: u16) { + fn set_size(&mut self, size: u16) -> Result<(), Error> { + if size > self.max_size() || size == 0 || (size & (size - 1)) != 0 { + error!("virtio queue with invalid size: {}", size); + return Err(Error::InvalidQueueSize); + } + self.size = size; + Ok(()) } fn ready(&self) -> bool { @@ -751,25 +762,46 @@ impl QueueStateT for QueueState { self.ready = ready; } - fn set_desc_table_address(&mut self, low: Option, high: Option) { + fn set_desc_table_address(&mut self, low: Option, high: Option) -> Result<(), Error> { let low = low.unwrap_or(self.desc_table.0 as u32) as u64; let high = high.unwrap_or((self.desc_table.0 >> 32) as u32) as u64; - self.desc_table = GuestAddress((high << 32) | low); + let desc_table = GuestAddress((high << 32) | low); + if desc_table.mask(0xf) != 0 { + error!("virtio queue descriptor table breaks alignment constraints"); + return Err(Error::InvalidDescriptorTableAddressAlignment); + } + + self.desc_table = desc_table; + Ok(()) } - fn set_avail_ring_address(&mut self, low: Option, high: Option) { + fn set_avail_ring_address(&mut self, low: Option, high: Option) -> Result<(), Error> { let low = low.unwrap_or(self.avail_ring.0 as u32) as u64; let high = high.unwrap_or((self.avail_ring.0 >> 32) as u32) as u64; - self.avail_ring = GuestAddress((high << 32) | low); + let avail_ring = GuestAddress((high << 32) | low); + if avail_ring.mask(0x1) != 0 { + error!("virtio queue available ring breaks alignment constraints"); + return Err(Error::InvalidAvailableRingAddressAlignment); + } + + self.avail_ring = avail_ring; + Ok(()) } - fn set_used_ring_address(&mut self, low: Option, high: Option) { + fn set_used_ring_address(&mut self, low: Option, high: Option) -> Result<(), Error> { let low = low.unwrap_or(self.used_ring.0 as u32) as u64; let high = high.unwrap_or((self.used_ring.0 >> 32) as u32) as u64; - self.used_ring = GuestAddress((high << 32) | low); + let used_ring = GuestAddress((high << 32) | low); + if used_ring.mask(0x3) != 0 { + error!("virtio queue used ring breaks alignment constraints"); + return Err(Error::InvalidUsedRingAddressAlignment); + } + + self.used_ring = used_ring; + Ok(()) } fn set_event_idx(&mut self, enabled: bool) { @@ -917,7 +949,7 @@ impl QueueStateT for QueueStateSync { self.state.lock().unwrap().actual_size() } - fn set_size(&mut self, size: u16) { + fn set_size(&mut self, size: u16) -> Result<(), Error> { self.state.lock().unwrap().set_size(size) } @@ -929,16 +961,16 @@ impl QueueStateT for QueueStateSync { self.state.lock().unwrap().set_ready(ready) } - fn set_desc_table_address(&mut self, low: Option, high: Option) { - self.state.lock().unwrap().set_desc_table_address(low, high); + fn set_desc_table_address(&mut self, low: Option, high: Option) -> Result<(), Error> { + self.state.lock().unwrap().set_desc_table_address(low, high) } - fn set_avail_ring_address(&mut self, low: Option, high: Option) { - self.state.lock().unwrap().set_avail_ring_address(low, high); + fn set_avail_ring_address(&mut self, low: Option, high: Option) -> Result<(), Error> { + self.state.lock().unwrap().set_avail_ring_address(low, high) } - fn set_used_ring_address(&mut self, low: Option, high: Option) { - self.state.lock().unwrap().set_used_ring_address(low, high); + fn set_used_ring_address(&mut self, low: Option, high: Option) -> Result<(), Error> { + self.state.lock().unwrap().set_used_ring_address(low, high) } fn set_event_idx(&mut self, enabled: bool) { @@ -1026,7 +1058,7 @@ impl> Queue { /// /// The `size` should power of two and less than or equal to value reported by `max_size()`, /// otherwise it will panic. - pub fn set_size(&mut self, size: u16) { + pub fn set_size(&mut self, size: u16) -> Result<(), Error> { self.state.set_size(size) } @@ -1044,23 +1076,35 @@ impl> Queue { /// /// The descriptor table address is 64-bit, the corresponding part will be updated if 'low' /// and/or `high` is valid. - pub fn set_desc_table_address(&mut self, low: Option, high: Option) { - self.state.set_desc_table_address(low, high); + pub fn set_desc_table_address( + &mut self, + low: Option, + high: Option, + ) -> Result<(), Error> { + self.state.set_desc_table_address(low, high) } /// Set available ring address for the queue. /// /// The available ring address is 64-bit, the corresponding part will be updated if 'low' /// and/or `high` is valid. - pub fn set_avail_ring_address(&mut self, low: Option, high: Option) { - self.state.set_avail_ring_address(low, high); + pub fn set_avail_ring_address( + &mut self, + low: Option, + high: Option, + ) -> Result<(), Error> { + self.state.set_avail_ring_address(low, high) } /// Set used ring address for the queue. /// /// The used ring address is 64-bit, the corresponding part will be updated if 'low' /// and/or `high` is valid. - pub fn set_used_ring_address(&mut self, low: Option, high: Option) { + pub fn set_used_ring_address( + &mut self, + low: Option, + high: Option, + ) -> Result<(), Error> { self.state.set_used_ring_address(low, high) } @@ -1292,38 +1336,36 @@ mod tests { assert!(!q.is_valid()); q.state.ready = true; - // or when size > max_size - q.state.size = q.state.max_size << 1; - assert!(!q.is_valid()); - q.state.size = q.state.max_size; + // shouldn't be allowed to set a size > max_size + assert!(q.set_size(q.state.max_size << 1).is_err()); - // or when size is 0 - q.state.size = 0; - assert!(!q.is_valid()); - q.state.size = q.state.max_size; - - // or when size is not a power of 2 - q.state.size = 11; - assert!(!q.is_valid()); - q.state.size = q.state.max_size; + // or set the size to 0 + assert!(q.set_size(0).is_err()); - // or if the various addresses are off + // or set a size which is not a power of 2 + assert!(q.set_size(11).is_err()); - q.state.desc_table = GuestAddress(0xffff_ffff); - assert!(!q.is_valid()); - q.state.desc_table = GuestAddress(0x1001); + // shouldn't be allowed to set a value that breaks the alignment constraint + assert!(q.set_desc_table_address(Some(0x100f), None).is_err()); + // should be allowed to set an out of bounds value + assert!(q.set_desc_table_address(Some(0xffff_fff0), None).is_ok()); + // but shouldn't be valid assert!(!q.is_valid()); q.state.desc_table = vq.desc_table_addr(); - q.state.avail_ring = GuestAddress(0xffff_ffff); - assert!(!q.is_valid()); - q.state.avail_ring = GuestAddress(0x1001); + // shouldn't be allowed to set a value that breaks the alignment constraint + assert!(q.set_avail_ring_address(Some(0x1001), None).is_err()); + // should be allowed to set an out of bounds value + assert!(q.set_avail_ring_address(Some(0xffff_fffe), None).is_ok()); + // but shouldn't be valid assert!(!q.is_valid()); q.state.avail_ring = vq.avail_addr(); - q.state.used_ring = GuestAddress(0xffff_ffff); - assert!(!q.is_valid()); - q.state.used_ring = GuestAddress(0x1001); + // shouldn't be allowed to set a value that breaks the alignment constraint + assert!(q.set_used_ring_address(Some(0x1003), None).is_err()); + // should be allowed to set an out of bounds value + assert!(q.set_used_ring_address(Some(0xffff_fffc), None).is_ok()); + // but shouldn't be valid assert!(!q.is_valid()); q.state.used_ring = vq.used_addr();