Allow HAL to validate MMIO ranges (#57)

* Don't use Hal::phys_to_virt for DMA ranges.

Instead, have dma_alloc return the virtual address alongside the
physical address.

* Let HAL check size of MMIO regions.

* Use NonNull<u8> for virtual addresses rather than VirtAddr.

* Remove VirtAddr type alias entirely.

* Pass fake descriptors as array pointer.

* Rename phys_to_virt to reflect that it is only used for MMIO.

examples/aarch64/src/hal.rs

@@ -4,7 +4,7 @@ use core::{
 };
 use lazy_static::lazy_static;
 use log::trace;
-use virtio_drivers::{BufferDirection, Hal, PhysAddr, VirtAddr, PAGE_SIZE};
+use virtio_drivers::{BufferDirection, Hal, PhysAddr, PAGE_SIZE};
 
 extern "C" {
     static dma_region: u8;
@@ -18,19 +18,20 @@ lazy_static! {
 pub struct HalImpl;
 
 impl Hal for HalImpl {
-    fn dma_alloc(pages: usize, _direction: BufferDirection) -> PhysAddr {
+    fn dma_alloc(pages: usize, _direction: BufferDirection) -> (PhysAddr, NonNull<u8>) {
         let paddr = DMA_PADDR.fetch_add(PAGE_SIZE * pages, Ordering::SeqCst);
         trace!("alloc DMA: paddr={:#x}, pages={}", paddr, pages);
-        paddr
+        let vaddr = NonNull::new(paddr as _).unwrap();
+        (paddr, vaddr)
     }
 
-    fn dma_dealloc(paddr: PhysAddr, pages: usize) -> i32 {
+    fn dma_dealloc(paddr: PhysAddr, _vaddr: NonNull<u8>, pages: usize) -> i32 {
         trace!("dealloc DMA: paddr={:#x}, pages={}", paddr, pages);
         0
     }
 
-    fn phys_to_virt(paddr: PhysAddr) -> VirtAddr {
-        paddr
+    fn mmio_phys_to_virt(paddr: PhysAddr, _size: usize) -> NonNull<u8> {
+        NonNull::new(paddr as _).unwrap()
     }
 
     fn share(buffer: NonNull<[u8]>, _direction: BufferDirection) -> PhysAddr {
@@ -45,6 +46,6 @@ impl Hal for HalImpl {
     }
 }
 
-fn virt_to_phys(vaddr: VirtAddr) -> PhysAddr {
+fn virt_to_phys(vaddr: usize) -> PhysAddr {
     vaddr
 }

examples/riscv/src/virtio_impl.rs

@@ -4,7 +4,7 @@ use core::{
 };
 use lazy_static::lazy_static;
 use log::trace;
-use virtio_drivers::{BufferDirection, Hal, PhysAddr, VirtAddr, PAGE_SIZE};
+use virtio_drivers::{BufferDirection, Hal, PhysAddr, PAGE_SIZE};
 
 extern "C" {
     fn end();
@@ -17,19 +17,20 @@ lazy_static! {
 pub struct HalImpl;
 
 impl Hal for HalImpl {
-    fn dma_alloc(pages: usize, _direction: BufferDirection) -> PhysAddr {
+    fn dma_alloc(pages: usize, _direction: BufferDirection) -> (PhysAddr, NonNull<u8>) {
         let paddr = DMA_PADDR.fetch_add(PAGE_SIZE * pages, Ordering::SeqCst);
         trace!("alloc DMA: paddr={:#x}, pages={}", paddr, pages);
-        paddr
+        let vaddr = NonNull::new(paddr as _).unwrap();
+        (paddr, vaddr)
     }
 
-    fn dma_dealloc(paddr: PhysAddr, pages: usize) -> i32 {
+    fn dma_dealloc(paddr: PhysAddr, _vaddr: NonNull<u8>, pages: usize) -> i32 {
         trace!("dealloc DMA: paddr={:#x}, pages={}", paddr, pages);
         0
     }
 
-    fn phys_to_virt(paddr: PhysAddr) -> VirtAddr {
-        paddr
+    fn mmio_phys_to_virt(paddr: PhysAddr, _size: usize) -> NonNull<u8> {
+        NonNull::new(paddr as _).unwrap()
     }
 
     fn share(buffer: NonNull<[u8]>, _direction: BufferDirection) -> PhysAddr {
@@ -44,6 +45,6 @@ impl Hal for HalImpl {
     }
 }
 
-fn virt_to_phys(vaddr: VirtAddr) -> PhysAddr {
+fn virt_to_phys(vaddr: usize) -> PhysAddr {
     vaddr
 }

src/hal.rs

@@ -4,9 +4,6 @@ pub mod fake;
 use crate::{Error, Result, PAGE_SIZE};
 use core::{marker::PhantomData, ptr::NonNull};
 
-/// A virtual memory address in the address space of the program.
-pub type VirtAddr = usize;
-
 /// A physical address as used for virtio.
 pub type PhysAddr = usize;
 
@@ -14,6 +11,7 @@ pub type PhysAddr = usize;
 #[derive(Debug)]
 pub struct Dma<H: Hal> {
     paddr: usize,
+    vaddr: NonNull<u8>,
     pages: usize,
     _phantom: PhantomData<H>,
 }
@@ -22,12 +20,13 @@ impl<H: Hal> Dma<H> {
     /// Allocates the given number of pages of physically contiguous memory to be used for DMA in
     /// the given direction.
     pub fn new(pages: usize, direction: BufferDirection) -> Result<Self> {
-        let paddr = H::dma_alloc(pages, direction);
+        let (paddr, vaddr) = H::dma_alloc(pages, direction);
         if paddr == 0 {
             return Err(Error::DmaError);
         }
         Ok(Self {
             paddr,
+            vaddr,
             pages,
             _phantom: PhantomData::default(),
         })
@@ -41,7 +40,7 @@ impl<H: Hal> Dma<H> {
     /// Returns a pointer to the given offset within the DMA region.
     pub fn vaddr(&self, offset: usize) -> NonNull<u8> {
         assert!(offset < self.pages * PAGE_SIZE);
-        NonNull::new((H::phys_to_virt(self.paddr) + offset) as _).unwrap()
+        NonNull::new((self.vaddr.as_ptr() as usize + offset) as _).unwrap()
     }
 
     /// Returns a pointer to the entire DMA region as a slice.
@@ -54,23 +53,26 @@ impl<H: Hal> Dma<H> {
 
 impl<H: Hal> Drop for Dma<H> {
     fn drop(&mut self) {
-        let err = H::dma_dealloc(self.paddr, self.pages);
+        let err = H::dma_dealloc(self.paddr, self.vaddr, self.pages);
         assert_eq!(err, 0, "failed to deallocate DMA");
     }
 }
 
 /// The interface which a particular hardware implementation must implement.
 pub trait Hal {
-    /// Allocates the given number of contiguous physical pages of DMA memory for virtio use.
-    fn dma_alloc(pages: usize, direction: BufferDirection) -> PhysAddr;
+    /// Allocates the given number of contiguous physical pages of DMA memory for VirtIO use.
+    ///
+    /// Returns both the physical address which the device can use to access the memory, and a
+    /// pointer to the start of it which the driver can use to access it.
+    fn dma_alloc(pages: usize, direction: BufferDirection) -> (PhysAddr, NonNull<u8>);
     /// Deallocates the given contiguous physical DMA memory pages.
-    fn dma_dealloc(paddr: PhysAddr, pages: usize) -> i32;
-    /// Converts a physical address used for virtio to a virtual address which the program can
-    /// access.
+    fn dma_dealloc(paddr: PhysAddr, vaddr: NonNull<u8>, pages: usize) -> i32;
+    /// Converts a physical address used for MMIO to a virtual address which the driver can access.
     ///
-    /// This is used both for DMA regions allocated by `dma_alloc`, and for MMIO addresses within
-    /// BARs read from the device (for the PCI transport).
-    fn phys_to_virt(paddr: PhysAddr) -> VirtAddr;
+    /// This is only used for MMIO addresses within BARs read from the device, for the PCI
+    /// transport. It may check that the address range up to the given size is within the region
+    /// expected for MMIO.
+    fn mmio_phys_to_virt(paddr: PhysAddr, size: usize) -> NonNull<u8>;
     /// Shares the given memory range with the device, and returns the physical address that the
     /// device can use to access it.
     ///

src/hal/fake.rs

@@ -1,6 +1,6 @@
 //! Fake HAL implementation for tests.
 
-use crate::{BufferDirection, Hal, PhysAddr, VirtAddr, PAGE_SIZE};
+use crate::{BufferDirection, Hal, PhysAddr, PAGE_SIZE};
 use alloc::alloc::{alloc_zeroed, dealloc, handle_alloc_error};
 use core::{alloc::Layout, ptr::NonNull};
 
@@ -9,30 +9,31 @@ pub struct FakeHal;
 
 /// Fake HAL implementation for use in unit tests.
 impl Hal for FakeHal {
-    fn dma_alloc(pages: usize, _direction: BufferDirection) -> PhysAddr {
+    fn dma_alloc(pages: usize, _direction: BufferDirection) -> (PhysAddr, NonNull<u8>) {
         assert_ne!(pages, 0);
         let layout = Layout::from_size_align(pages * PAGE_SIZE, PAGE_SIZE).unwrap();
         // Safe because the size and alignment of the layout are non-zero.
         let ptr = unsafe { alloc_zeroed(layout) };
-        if ptr.is_null() {
+        if let Some(ptr) = NonNull::new(ptr) {
+            (ptr.as_ptr() as PhysAddr, ptr)
+        } else {
             handle_alloc_error(layout);
         }
-        ptr as PhysAddr
     }
 
-    fn dma_dealloc(paddr: PhysAddr, pages: usize) -> i32 {
+    fn dma_dealloc(_paddr: PhysAddr, vaddr: NonNull<u8>, pages: usize) -> i32 {
         assert_ne!(pages, 0);
         let layout = Layout::from_size_align(pages * PAGE_SIZE, PAGE_SIZE).unwrap();
         // Safe because the layout is the same as was used when the memory was allocated by
         // `dma_alloc` above.
         unsafe {
-            dealloc(paddr as *mut u8, layout);
+            dealloc(vaddr.as_ptr(), layout);
         }
         0
     }
 
-    fn phys_to_virt(paddr: PhysAddr) -> VirtAddr {
-        paddr
+    fn mmio_phys_to_virt(paddr: PhysAddr, _size: usize) -> NonNull<u8> {
+        NonNull::new(paddr as _).unwrap()
     }
 
     fn share(buffer: NonNull<[u8]>, _direction: BufferDirection) -> PhysAddr {
@@ -47,6 +48,6 @@ impl Hal for FakeHal {
     }
 }
 
-fn virt_to_phys(vaddr: VirtAddr) -> PhysAddr {
+fn virt_to_phys(vaddr: usize) -> PhysAddr {
     vaddr
 }

src/lib.rs

@@ -58,7 +58,7 @@ use core::{
     ptr::{self, NonNull},
 };
 
-pub use self::hal::{BufferDirection, Hal, PhysAddr, VirtAddr};
+pub use self::hal::{BufferDirection, Hal, PhysAddr};
 
 /// The page size in bytes supported by the library (4 KiB).
 pub const PAGE_SIZE: usize = 0x1000;

src/queue.rs

@@ -1,5 +1,3 @@
-#[cfg(test)]
-use crate::hal::VirtAddr;
 use crate::hal::{BufferDirection, Dma, Hal, PhysAddr};
 use crate::transport::Transport;
 use crate::{align_up, nonnull_slice_from_raw_parts, pages, Error, Result, PAGE_SIZE};
@@ -565,14 +563,13 @@ struct UsedElem {
 /// The fake device always uses descriptors in order.
 #[cfg(test)]
 pub(crate) fn fake_read_write_queue<const QUEUE_SIZE: usize>(
-    queue_descriptors: *const Descriptor,
-    queue_driver_area: VirtAddr,
-    queue_device_area: VirtAddr,
+    descriptors: *const [Descriptor; QUEUE_SIZE],
+    queue_driver_area: *const u8,
+    queue_device_area: *mut u8,
     handler: impl FnOnce(Vec<u8>) -> Vec<u8>,
 ) {
     use core::{ops::Deref, slice};
 
-    let descriptors = ptr::slice_from_raw_parts(queue_descriptors, QUEUE_SIZE);
     let available_ring = queue_driver_area as *const AvailRing<QUEUE_SIZE>;
     let used_ring = queue_device_area as *mut UsedRing<QUEUE_SIZE>;
 

src/transport/fake.rs

@@ -111,10 +111,10 @@ impl State {
     pub fn write_to_queue<const QUEUE_SIZE: usize>(&mut self, queue_index: u16, data: &[u8]) {
         let queue = &self.queues[queue_index as usize];
         assert_ne!(queue.descriptors, 0);
-        fake_read_write_queue::<QUEUE_SIZE>(
-            queue.descriptors as *const Descriptor,
-            queue.driver_area,
-            queue.device_area,
+        fake_read_write_queue(
+            queue.descriptors as *const [Descriptor; QUEUE_SIZE],
+            queue.driver_area as *const u8,
+            queue.device_area as *mut u8,
             |input| {
                 assert_eq!(input, Vec::new());
                 data.to_owned()
@@ -134,10 +134,10 @@ impl State {
         let mut ret = None;
 
         // Read data from the queue but don't write any response.
-        fake_read_write_queue::<QUEUE_SIZE>(
-            queue.descriptors as *const Descriptor,
-            queue.driver_area,
-            queue.device_area,
+        fake_read_write_queue(
+            queue.descriptors as *const [Descriptor; QUEUE_SIZE],
+            queue.driver_area as *const u8,
+            queue.device_area as *mut u8,
             |input| {
                 ret = Some(input);
                 Vec::new()
@@ -157,10 +157,10 @@ impl State {
     ) {
         let queue = &self.queues[queue_index as usize];
         assert_ne!(queue.descriptors, 0);
-        fake_read_write_queue::<QUEUE_SIZE>(
-            queue.descriptors as *const Descriptor,
-            queue.driver_area,
-            queue.device_area,
+        fake_read_write_queue(
+            queue.descriptors as *const [Descriptor; QUEUE_SIZE],
+            queue.driver_area as *const u8,
+            queue.device_area as *mut u8,
             handler,
         )
     }

src/transport/pci.rs

@@ -5,7 +5,7 @@ pub mod bus;
 use self::bus::{DeviceFunction, DeviceFunctionInfo, PciError, PciRoot, PCI_CAP_ID_VNDR};
 use super::{DeviceStatus, DeviceType, Transport};
 use crate::{
-    hal::{Hal, PhysAddr, VirtAddr},
+    hal::{Hal, PhysAddr},
     nonnull_slice_from_raw_parts,
     volatile::{
         volread, volwrite, ReadOnly, Volatile, VolatileReadable, VolatileWritable, WriteOnly,
@@ -388,14 +388,14 @@ fn get_bar_region<H: Hal, T>(
         return Err(VirtioPciError::BarOffsetOutOfRange);
     }
     let paddr = bar_address as PhysAddr + struct_info.offset as PhysAddr;
-    let vaddr = H::phys_to_virt(paddr);
-    if vaddr % align_of::<T>() != 0 {
+    let vaddr = H::mmio_phys_to_virt(paddr, struct_info.length as usize);
+    if vaddr.as_ptr() as usize % align_of::<T>() != 0 {
         return Err(VirtioPciError::Misaligned {
             vaddr,
             alignment: align_of::<T>(),
         });
     }
-    Ok(NonNull::new(vaddr as _).unwrap())
+    Ok(vaddr.cast())
 }
 
 fn get_bar_region_slice<H: Hal, T>(
@@ -433,7 +433,7 @@ pub enum VirtioPciError {
     /// The virtual address was not aligned as expected.
     Misaligned {
         /// The virtual address in question.
-        vaddr: VirtAddr,
+        vaddr: NonNull<u8>,
         /// The expected alignment in bytes.
         alignment: usize,
     },
@@ -472,7 +472,7 @@ impl Display for VirtioPciError {
             Self::BarOffsetOutOfRange => write!(f, "Capability offset greater than BAR length."),
             Self::Misaligned { vaddr, alignment } => write!(
                 f,
-                "Virtual address {:#018x} was not aligned to a {} byte boundary as expected.",
+                "Virtual address {:#018?} was not aligned to a {} byte boundary as expected.",
                 vaddr, alignment
             ),
             Self::Pci(pci_error) => pci_error.fmt(f),