Fix soundness issues with MMIO and shared memory (#18)

* First cut at sound volatile MMIO.

* Use new volatile module for blk too.

* Backwards compatibility for old rustc in example.

* Document volatile module.

* Use volatile module for remaining devices.

* UsedRing is only read by the driver.

* No need to volatile for queue.

The queue is just shared memory, not MMIO memory, so there is no need to
use volatile reads and writes.

* Add fence before reading used ring and after writing available ring.

Author: qwandor

Cargo.toml

@@ -8,6 +8,5 @@ description = "VirtIO guest drivers."
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-volatile = "0.3"
 log = "0.4"
 bitflags = "1.3"

src/blk.rs

@@ -1,22 +1,22 @@
 use super::*;
 use crate::queue::VirtQueue;
 use crate::transport::Transport;
+use crate::volatile::{volread, Volatile};
 use bitflags::*;
 use core::hint::spin_loop;
 use log::*;
-use volatile::Volatile;
 
 /// The virtio block device is a simple virtual block device (ie. disk).
 ///
 /// Read and write requests (and other exotic requests) are placed in the queue,
 /// and serviced (probably out of order) by the device except where noted.
-pub struct VirtIOBlk<'a, H: Hal, T: Transport> {
+pub struct VirtIOBlk<H: Hal, T: Transport> {
     transport: T,
-    queue: VirtQueue<'a, H>,
+    queue: VirtQueue<H>,
     capacity: usize,
 }
 
-impl<H: Hal, T: Transport> VirtIOBlk<'_, H, T> {
+impl<H: Hal, T: Transport> VirtIOBlk<H, T> {
     /// Create a new VirtIO-Blk driver.
     pub fn new(mut transport: T) -> Result<Self> {
         transport.begin_init(|features| {
@@ -28,21 +28,19 @@ impl<H: Hal, T: Transport> VirtIOBlk<'_, H, T> {
         });
 
         // read configuration space
-        let config_space = transport.config_space().cast::<BlkConfig>();
-        let config = unsafe { config_space.as_ref() };
+        let config = transport.config_space().cast::<BlkConfig>();
         info!("config: {:?}", config);
-        info!(
-            "found a block device of size {}KB",
-            config.capacity.read() / 2
-        );
+        // Safe because config is a valid pointer to the device configuration space.
+        let capacity = unsafe { volread!(config, capacity) };
+        info!("found a block device of size {}KB", capacity / 2);
 
         let queue = VirtQueue::new(&mut transport, 0, 16)?;
         transport.finish_init();
 
         Ok(VirtIOBlk {
             transport,
             queue,
-            capacity: config.capacity.read() as usize,
+            capacity: capacity as usize,
         })
     }
 

src/console.rs

@@ -1,10 +1,10 @@
 use super::*;
 use crate::queue::VirtQueue;
 use crate::transport::Transport;
+use crate::volatile::{ReadOnly, WriteOnly};
 use bitflags::*;
 use core::{fmt, hint::spin_loop};
 use log::*;
-use volatile::{ReadOnly, WriteOnly};
 
 const QUEUE_RECEIVEQ_PORT_0: usize = 0;
 const QUEUE_TRANSMITQ_PORT_0: usize = 1;
@@ -14,8 +14,8 @@ const QUEUE_SIZE: u16 = 2;
 /// Emergency and cols/rows unimplemented.
 pub struct VirtIOConsole<'a, H: Hal, T: Transport> {
     transport: T,
-    receiveq: VirtQueue<'a, H>,
-    transmitq: VirtQueue<'a, H>,
+    receiveq: VirtQueue<H>,
+    transmitq: VirtQueue<H>,
     queue_buf_dma: DMA<H>,
     queue_buf_rx: &'a mut [u8],
     cursor: usize,
@@ -161,7 +161,7 @@ mod tests {
             cols: ReadOnly::new(0),
             rows: ReadOnly::new(0),
             max_nr_ports: ReadOnly::new(0),
-            emerg_wr: WriteOnly::new(0),
+            emerg_wr: WriteOnly::default(),
         };
         let state = Arc::new(Mutex::new(State {
             status: DeviceStatus::empty(),

src/gpu.rs

@@ -1,10 +1,10 @@
 use super::*;
 use crate::queue::VirtQueue;
 use crate::transport::Transport;
+use crate::volatile::{ReadOnly, Volatile, WriteOnly};
 use bitflags::*;
 use core::{fmt, hint::spin_loop};
 use log::*;
-use volatile::{ReadOnly, Volatile, WriteOnly};
 
 /// A virtio based graphics adapter.
 ///
@@ -21,9 +21,9 @@ pub struct VirtIOGpu<'a, H: Hal, T: Transport> {
     /// DMA area of cursor image buffer.
     cursor_buffer_dma: Option<DMA<H>>,
     /// Queue for sending control commands.
-    control_queue: VirtQueue<'a, H>,
+    control_queue: VirtQueue<H>,
     /// Queue for sending cursor commands.
-    cursor_queue: VirtQueue<'a, H>,
+    cursor_queue: VirtQueue<H>,
     /// Queue buffer DMA
     queue_buf_dma: DMA<H>,
     /// Send buffer for queue.

src/input.rs

@@ -1,23 +1,23 @@
 use super::*;
 use crate::transport::Transport;
+use crate::volatile::{volread, volwrite, ReadOnly, WriteOnly};
 use alloc::boxed::Box;
 use bitflags::*;
 use log::*;
-use volatile::{ReadOnly, WriteOnly};
 
 /// Virtual human interface devices such as keyboards, mice and tablets.
 ///
 /// An instance of the virtio device represents one such input device.
 /// Device behavior mirrors that of the evdev layer in Linux,
 /// making pass-through implementations on top of evdev easy.
-pub struct VirtIOInput<'a, H: Hal, T: Transport> {
+pub struct VirtIOInput<H: Hal, T: Transport> {
     transport: T,
-    event_queue: VirtQueue<'a, H>,
-    status_queue: VirtQueue<'a, H>,
+    event_queue: VirtQueue<H>,
+    status_queue: VirtQueue<H>,
     event_buf: Box<[InputEvent; 32]>,
 }
 
-impl<H: Hal, T: Transport> VirtIOInput<'_, H, T> {
+impl<H: Hal, T: Transport> VirtIOInput<H, T> {
     /// Create a new VirtIO-Input driver.
     pub fn new(mut transport: T) -> Result<Self> {
         let mut event_buf = Box::new([InputEvent::default(); QUEUE_SIZE]);
@@ -75,12 +75,16 @@ impl<H: Hal, T: Transport> VirtIOInput<'_, H, T> {
         subsel: u8,
         out: &mut [u8],
     ) -> u8 {
-        let mut config_space = self.transport.config_space().cast::<Config>();
-        let config = unsafe { config_space.as_mut() };
-        config.select.write(select as u8);
-        config.subsel.write(subsel);
-        let size = config.size.read();
-        let data = config.data.read();
+        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);
+        }
         out[..size as usize].copy_from_slice(&data[..size as usize]);
         size
     }

src/lib.rs

@@ -15,6 +15,7 @@ mod input;
 mod net;
 mod queue;
 mod transport;
+mod volatile;
 
 pub use self::blk::{BlkResp, RespStatus, VirtIOBlk};
 pub use self::console::VirtIOConsole;

src/net.rs

@@ -2,10 +2,10 @@ use core::mem::{size_of, MaybeUninit};
 
 use super::*;
 use crate::transport::Transport;
+use crate::volatile::{volread, ReadOnly, Volatile};
 use bitflags::*;
 use core::hint::spin_loop;
 use log::*;
-use volatile::{ReadOnly, Volatile};
 
 /// The virtio network device is a virtual ethernet card.
 ///
@@ -14,14 +14,14 @@ use volatile::{ReadOnly, Volatile};
 /// Empty buffers are placed in one virtqueue for receiving packets, and
 /// outgoing packets are enqueued into another for transmission in that order.
 /// A third command queue is used to control advanced filtering features.
-pub struct VirtIONet<'a, H: Hal, T: Transport> {
+pub struct VirtIONet<H: Hal, T: Transport> {
     transport: T,
     mac: EthernetAddress,
-    recv_queue: VirtQueue<'a, H>,
-    send_queue: VirtQueue<'a, H>,
+    recv_queue: VirtQueue<H>,
+    send_queue: VirtQueue<H>,
 }
 
-impl<H: Hal, T: Transport> VirtIONet<'_, H, T> {
+impl<H: Hal, T: Transport> VirtIONet<H, T> {
     /// Create a new VirtIO-Net driver.
     pub fn new(mut transport: T) -> Result<Self> {
         transport.begin_init(|features| {
@@ -31,10 +31,13 @@ impl<H: Hal, T: Transport> VirtIONet<'_, H, T> {
             (features & supported_features).bits()
         });
         // read configuration space
-        let config_space = transport.config_space().cast::<Config>();
-        let config = unsafe { config_space.as_ref() };
-        let mac = config.mac.read();
-        debug!("Got MAC={:?}, status={:?}", mac, config.status.read());
+        let config = transport.config_space().cast::<Config>();
+        let mac;
+        // Safe because config points to a valid MMIO region for the config space.
+        unsafe {
+            mac = volread!(config, mac);
+            debug!("Got MAC={:?}, status={:?}", mac, volread!(config, status));
+        }
 
         let queue_num = 2; // for simplicity
         let recv_queue = VirtQueue::new(&mut transport, QUEUE_RECEIVE, queue_num)?;