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: rust-vmm#94

Signed-off-by: German Maglione <[email protected]>

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<M, D, F>(device: &mut D, f: F)
 where
     M: GuestAddressSpace,
     D: WithDriverSelect<M> + ?Sized,
-    F: FnOnce(&mut Queue<M>),
+    F: FnOnce(&mut Queue<M>) -> 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<M: GuestAddressSpace>: WithDriverSelect<M> {
                     // 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());

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<u8>) -> 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);

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<M: GuestAddressSpace> {
     ///
     /// 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<M: GuestAddressSpace> {
     ///
     /// 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<u32>, high: Option<u32>);
+    fn set_desc_table_address(&mut self, low: Option<u32>, high: Option<u32>) -> 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<u32>, high: Option<u32>);
+    fn set_avail_ring_address(&mut self, low: Option<u32>, high: Option<u32>) -> 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<u32>, high: Option<u32>);
+    fn set_used_ring_address(&mut self, low: Option<u32>, high: Option<u32>) -> 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<M: GuestAddressSpace> QueueStateT<M> for QueueState<M> {
         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<M: GuestAddressSpace> QueueStateT<M> for QueueState<M> {
                 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<M: GuestAddressSpace> QueueStateT<M> for QueueState<M> {
         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<M: GuestAddressSpace> QueueStateT<M> for QueueState<M> {
         self.ready = ready;
     }
 
-    fn set_desc_table_address(&mut self, low: Option<u32>, high: Option<u32>) {
+    fn set_desc_table_address(&mut self, low: Option<u32>, high: Option<u32>) -> 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<u32>, high: Option<u32>) {
+    fn set_avail_ring_address(&mut self, low: Option<u32>, high: Option<u32>) -> 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<u32>, high: Option<u32>) {
+    fn set_used_ring_address(&mut self, low: Option<u32>, high: Option<u32>) -> 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<M: GuestAddressSpace> QueueStateT<M> for QueueStateSync<M> {
         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<M: GuestAddressSpace> QueueStateT<M> for QueueStateSync<M> {
         self.state.lock().unwrap().set_ready(ready)
     }
 
-    fn set_desc_table_address(&mut self, low: Option<u32>, high: Option<u32>) {
-        self.state.lock().unwrap().set_desc_table_address(low, high);
+    fn set_desc_table_address(&mut self, low: Option<u32>, high: Option<u32>) -> Result<(), Error> {
+        self.state.lock().unwrap().set_desc_table_address(low, high)
     }
 
-    fn set_avail_ring_address(&mut self, low: Option<u32>, high: Option<u32>) {
-        self.state.lock().unwrap().set_avail_ring_address(low, high);
+    fn set_avail_ring_address(&mut self, low: Option<u32>, high: Option<u32>) -> Result<(), Error> {
+        self.state.lock().unwrap().set_avail_ring_address(low, high)
     }
 
-    fn set_used_ring_address(&mut self, low: Option<u32>, high: Option<u32>) {
-        self.state.lock().unwrap().set_used_ring_address(low, high);
+    fn set_used_ring_address(&mut self, low: Option<u32>, high: Option<u32>) -> 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<M: GuestAddressSpace, S: QueueStateT<M>> Queue<M, S> {
     ///
     /// 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<M: GuestAddressSpace, S: QueueStateT<M>> Queue<M, S> {
     ///
     /// 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<u32>, high: Option<u32>) {
-        self.state.set_desc_table_address(low, high);
+    pub fn set_desc_table_address(
+        &mut self,
+        low: Option<u32>,
+        high: Option<u32>,
+    ) -> 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<u32>, high: Option<u32>) {
-        self.state.set_avail_ring_address(low, high);
+    pub fn set_avail_ring_address(
+        &mut self,
+        low: Option<u32>,
+        high: Option<u32>,
+    ) -> 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<u32>, high: Option<u32>) {
+    pub fn set_used_ring_address(
+        &mut self,
+        low: Option<u32>,
+        high: Option<u32>,
+    ) -> 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();