Check size of config space (#29)

* Config space is only required to be 4 byte aligned.

* Check size of config space where possible.

* Return error rather than panicking if config space too small.

* Check alignment of config space too.

src/blk.rs

@@ -28,10 +28,12 @@ impl<H: Hal, T: Transport> VirtIOBlk<H, T> {
         });
 
         // read configuration space
-        let config = transport.config_space().cast::<BlkConfig>();
+        let config = transport.config_space::<BlkConfig>()?;
         info!("config: {:?}", config);
         // Safe because config is a valid pointer to the device configuration space.
-        let capacity = unsafe { volread!(config, capacity) };
+        let capacity = unsafe {
+            volread!(config, capacity_low) as u64 | (volread!(config, capacity_high) as u64) << 32
+        };
         info!("found a block device of size {}KB", capacity / 2);
 
         let queue = VirtQueue::new(&mut transport, 0, 16)?;
@@ -192,7 +194,8 @@ impl<H: Hal, T: Transport> VirtIOBlk<H, T> {
 #[repr(C)]
 struct BlkConfig {
     /// Number of 512 Bytes sectors
-    capacity: Volatile<u64>,
+    capacity_low: Volatile<u32>,
+    capacity_high: Volatile<u32>,
     size_max: Volatile<u32>,
     seg_max: Volatile<u32>,
     cylinders: Volatile<u16>,

src/console.rs

@@ -31,7 +31,7 @@ impl<H: Hal, T: Transport> VirtIOConsole<'_, H, T> {
             let supported_features = Features::empty();
             (features & supported_features).bits()
         });
-        let config_space = transport.config_space().cast::<Config>();
+        let config_space = transport.config_space::<Config>()?;
         unsafe {
             let columns = volread!(config_space, cols);
             let rows = volread!(config_space, rows);
@@ -171,10 +171,10 @@ mod tests {
             device_type: DeviceType::Console,
             max_queue_size: 2,
             device_features: 0,
-            config_space: NonNull::from(&mut config_space).cast(),
+            config_space: NonNull::from(&mut config_space),
             state: state.clone(),
         };
-        let mut console = VirtIOConsole::<FakeHal, FakeTransport>::new(transport).unwrap();
+        let mut console = VirtIOConsole::<FakeHal, FakeTransport<Config>>::new(transport).unwrap();
 
         // Nothing is available to receive.
         assert_eq!(console.recv(false).unwrap(), None);

src/gpu.rs

@@ -43,7 +43,7 @@ impl<H: Hal, T: Transport> VirtIOGpu<'_, H, T> {
         });
 
         // read configuration space
-        let config_space = transport.config_space().cast::<Config>();
+        let config_space = transport.config_space::<Config>()?;
         unsafe {
             let events_read = volread!(config_space, events_read);
             let num_scanouts = volread!(config_space, num_scanouts);

src/input.rs

@@ -3,6 +3,7 @@ use crate::transport::Transport;
 use crate::volatile::{volread, volwrite, ReadOnly, WriteOnly};
 use alloc::boxed::Box;
 use bitflags::*;
+use core::ptr::NonNull;
 use log::*;
 
 /// Virtual human interface devices such as keyboards, mice and tablets.
@@ -15,6 +16,7 @@ pub struct VirtIOInput<H: Hal, T: Transport> {
     event_queue: VirtQueue<H>,
     status_queue: VirtQueue<H>,
     event_buf: Box<[InputEvent; 32]>,
+    config: NonNull<Config>,
 }
 
 impl<H: Hal, T: Transport> VirtIOInput<H, T> {
@@ -29,6 +31,8 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
             (features & supported_features).bits()
         });
 
+        let config = transport.config_space::<Config>()?;
+
         let mut event_queue = VirtQueue::new(&mut transport, QUEUE_EVENT, QUEUE_SIZE as u16)?;
         let status_queue = VirtQueue::new(&mut transport, QUEUE_STATUS, QUEUE_SIZE as u16)?;
         for (i, event) in event_buf.as_mut().iter_mut().enumerate() {
@@ -43,6 +47,7 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
             event_queue,
             status_queue,
             event_buf,
+            config,
         })
     }
 
@@ -75,15 +80,14 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
         subsel: u8,
         out: &mut [u8],
     ) -> u8 {
-        let config = self.transport.config_space().cast::<Config>();
         let size;
         let data;
         // Safe because config points to a valid MMIO region for the config space.
         unsafe {
-            volwrite!(config, select, select as u8);
-            volwrite!(config, subsel, subsel);
-            size = volread!(config, size);
-            data = volread!(config, data);
+            volwrite!(self.config, select, select as u8);
+            volwrite!(self.config, subsel, subsel);
+            size = volread!(self.config, size);
+            data = volread!(self.config, data);
         }
         out[..size as usize].copy_from_slice(&data[..size as usize]);
         size

src/lib.rs

@@ -54,6 +54,10 @@ pub enum Error {
     DmaError,
     /// I/O Error
     IoError,
+    /// The config space advertised by the device is smaller than the driver expected.
+    ConfigSpaceTooSmall,
+    /// The device doesn't have any config space, but the driver expects some.
+    ConfigSpaceMissing,
 }
 
 /// Align `size` up to a page.

src/net.rs

@@ -31,7 +31,7 @@ impl<H: Hal, T: Transport> VirtIONet<H, T> {
             (features & supported_features).bits()
         });
         // read configuration space
-        let config = transport.config_space().cast::<Config>();
+        let config = transport.config_space::<Config>()?;
         let mac;
         // Safe because config points to a valid MMIO region for the config space.
         unsafe {

src/transport/fake.rs

@@ -1,23 +1,23 @@
 use super::{DeviceStatus, Transport};
 use crate::{
     queue::{fake_write_to_queue, Descriptor},
-    DeviceType, PhysAddr,
+    DeviceType, PhysAddr, Result,
 };
 use alloc::{sync::Arc, vec::Vec};
-use core::ptr::NonNull;
+use core::{any::TypeId, ptr::NonNull};
 use std::sync::Mutex;
 
 /// A fake implementation of [`Transport`] for unit tests.
 #[derive(Debug)]
-pub struct FakeTransport {
+pub struct FakeTransport<C: 'static> {
     pub device_type: DeviceType,
     pub max_queue_size: u32,
     pub device_features: u64,
-    pub config_space: NonNull<u64>,
+    pub config_space: NonNull<C>,
     pub state: Arc<Mutex<State>>,
 }
 
-impl Transport for FakeTransport {
+impl<C> Transport for FakeTransport<C> {
     fn device_type(&self) -> DeviceType {
         self.device_type
     }
@@ -74,8 +74,12 @@ impl Transport for FakeTransport {
         pending
     }
 
-    fn config_space(&self) -> NonNull<u64> {
-        self.config_space
+    fn config_space<T: 'static>(&self) -> Result<NonNull<T>> {
+        if TypeId::of::<T>() == TypeId::of::<C>() {
+            Ok(self.config_space.cast())
+        } else {
+            panic!("Unexpected config space type.");
+        }
     }
 }
 

src/transport/mmio.rs

@@ -3,12 +3,12 @@ use crate::{
     align_up,
     queue::Descriptor,
     volatile::{volread, volwrite, ReadOnly, Volatile, WriteOnly},
-    PhysAddr, PAGE_SIZE,
+    Error, PhysAddr, PAGE_SIZE,
 };
 use core::{
     convert::{TryFrom, TryInto},
     fmt::{self, Display, Formatter},
-    mem::size_of,
+    mem::{align_of, size_of},
     ptr::NonNull,
 };
 
@@ -442,7 +442,14 @@ impl Transport for MmioTransport {
         }
     }
 
-    fn config_space(&self) -> NonNull<u64> {
-        NonNull::new((self.header.as_ptr() as usize + CONFIG_SPACE_OFFSET) as _).unwrap()
+    fn config_space<T>(&self) -> Result<NonNull<T>, Error> {
+        if align_of::<T>() > 4 {
+            // Panic as this should only happen if the driver is written incorrectly.
+            panic!(
+                "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.",
+                align_of::<T>()
+            );
+        }
+        Ok(NonNull::new((self.header.as_ptr() as usize + CONFIG_SPACE_OFFSET) as _).unwrap())
     }
 }

src/transport/mod.rs

@@ -3,7 +3,7 @@ pub mod fake;
 pub mod mmio;
 pub mod pci;
 
-use crate::{PhysAddr, PAGE_SIZE};
+use crate::{PhysAddr, Result, PAGE_SIZE};
 use bitflags::bitflags;
 use core::ptr::NonNull;
 
@@ -74,7 +74,7 @@ pub trait Transport {
     }
 
     /// Gets the pointer to the config space.
-    fn config_space(&self) -> NonNull<u64>;
+    fn config_space<T: 'static>(&self) -> Result<NonNull<T>>;
 }
 
 bitflags! {

src/transport/pci.rs

@@ -9,6 +9,7 @@ use crate::{
     volatile::{
         volread, volwrite, ReadOnly, Volatile, VolatileReadable, VolatileWritable, WriteOnly,
     },
+    Error,
 };
 use core::{
     fmt::{self, Display, Formatter},
@@ -91,7 +92,7 @@ pub struct PciTransport {
     /// The ISR status register within some BAR.
     isr_status: NonNull<Volatile<u8>>,
     /// The VirtIO device-specific configuration within some BAR.
-    config_space: Option<NonNull<[u64]>>,
+    config_space: Option<NonNull<[u32]>>,
 }
 
 impl PciTransport {
@@ -300,15 +301,24 @@ impl Transport for PciTransport {
         isr_status & 0x3 != 0
     }
 
-    fn config_space(&self) -> NonNull<u64> {
-        // TODO: Check config_space_size
-        // TODO: Use NonNull::as_non_null_ptr once it is stable.
-        NonNull::new(
-            self.config_space
-                .expect("No VIRTIO_PCI_CAP_DEVICE_CFG capability.")
-                .as_ptr() as *mut u64,
-        )
-        .unwrap()
+    fn config_space<T>(&self) -> Result<NonNull<T>, Error> {
+        if let Some(config_space) = self.config_space {
+            if size_of::<T>() > config_space.len() * size_of::<u32>() {
+                Err(Error::ConfigSpaceTooSmall)
+            } else if align_of::<T>() > 4 {
+                // Panic as this should only happen if the driver is written incorrectly.
+                panic!(
+                    "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.",
+                    align_of::<T>()
+                );
+            } else {
+                // TODO: Use NonNull::as_non_null_ptr once it is stable.
+                let config_space_ptr = NonNull::new(config_space.as_ptr() as *mut u32).unwrap();
+                Ok(config_space_ptr.cast())
+            }
+        } else {
+            Err(Error::ConfigSpaceMissing)
+        }
     }
 }