Mark VfioContainer::vfio_dma_map as unsafe

vfio_syscall::map_dma() accepts a u64 and tells the Linux kernel to make
the corresponding address accessible for DMA by a device.  Therefore,
passing bad u64 values can result in memory corruption or disclosure.
Change the u64 argument to a pointer to avoid confusion.  To reflect
that the caller must uphold invariants to prevent undefined behavior,
mark the API as unsafe.

I found this through a review of Cloud Hypervisor [1].  Some of its
internal APIs took a host took a host address cast to u64 as an argument
and accessed that address.  In one case, the u64 was passed directly to
vfio_syscall::map_dma().

[1]: cloud-hypervisor/cloud-hypervisor#7129

Signed-off-by: Demi Marie Obenour <[email protected]>

Author: DemiMarie

vfio-ioctls/CHANGELOG.md

@@ -1,3 +1,17 @@
+# Upcoming Release
+
+## Changed
+
+- Functions that map memory into the VFIO device are now marked as
+  `unsafe`.  The caller of these functions is responsible for enforcing
+  various complex but documented invariants to avoid undefined behavior.
+  This requirement is also present in previous versions of the crate,
+  but the function was not marked unsafe and the invariants were not
+  documented.
+
+  In the future a high-level safe API will be provided that avoids
+  these requirements at the cost of some flexibility.
+
 # [v0.5.0]
 
 ## Changed

vfio-ioctls/src/vfio_device.rs

@@ -4,6 +4,7 @@
 // SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
 
 use std::collections::HashMap;
+use std::convert::{TryFrom as _, TryInto as _};
 use std::ffi::CString;
 use std::fs::{File, OpenOptions};
 use std::mem::{self, ManuallyDrop};
@@ -279,16 +280,37 @@ impl VfioContainer {
     /// Map a region of guest memory regions into the vfio container's iommu table.
     ///
     /// # Parameters
+    ///
     /// * iova: IO virtual address to mapping the memory.
     /// * size: size of the memory region.
     /// * user_addr: host virtual address for the guest memory region to map.
-    pub fn vfio_dma_map(&self, iova: u64, size: u64, user_addr: u64) -> Result<()> {
+    ///
+    /// # Safety
+    ///
+    /// Until [`Self::vfio_dma_unmap`] is successfully called with identical
+    /// values for iova and size, or until the entire range of `[user_addr..user_addr+size)`
+    /// has been unmapped with successful calls to `munmap` or replaced with successful calls
+    /// to `mmap(MAP_FIXED)`, the only safe uses of the address range
+    /// `[user_addr..user_addr+size)` are:
+    ///
+    /// - Atomic and/or volatile operations on raw pointers.
+    /// - Sharing the underlying storage with another process or a guest VM.
+    /// - Passing a pointer to the memory to a system call (such as `read()`
+    ///   or `write()`) that is safe regardless of the memory's contents.
+    /// - Passing a raw pointer to functions that only do one of the above things.
+    ///
+    /// In particular, creating a Rust reference to this memory is instant undefined behavior
+    /// due to the Rust aliasing rules.  It is also undefined behavior to call this function if
+    /// a Rust reference to this memory is live.
+    pub unsafe fn vfio_dma_map(&self, iova: u64, size: usize, user_addr: *mut u8) -> Result<()> {
+        const _: () = assert!(mem::size_of::<u64>() >= mem::size_of::<*mut u8>());
+        const _: () = assert!(mem::size_of::<u64>() >= mem::size_of::<usize>());
         let dma_map = vfio_iommu_type1_dma_map {
             argsz: mem::size_of::<vfio_iommu_type1_dma_map>() as u32,
             flags: VFIO_DMA_MAP_FLAG_READ | VFIO_DMA_MAP_FLAG_WRITE,
-            vaddr: user_addr,
+            vaddr: (user_addr as usize).try_into().unwrap(),
             iova,
-            size,
+            size: size.try_into().unwrap(),
         };
 
         vfio_syscall::map_dma(self, &dma_map)
@@ -299,16 +321,16 @@ impl VfioContainer {
     /// # Parameters
     /// * iova: IO virtual address to mapping the memory.
     /// * size: size of the memory region.
-    pub fn vfio_dma_unmap(&self, iova: u64, size: u64) -> Result<()> {
+    pub fn vfio_dma_unmap(&self, iova: u64, size: usize) -> Result<()> {
         let mut dma_unmap = vfio_iommu_type1_dma_unmap {
             argsz: mem::size_of::<vfio_iommu_type1_dma_unmap>() as u32,
             flags: 0,
             iova,
-            size,
+            size: size.try_into().unwrap(),
         };
 
         vfio_syscall::unmap_dma(self, &mut dma_unmap)?;
-        if dma_unmap.size != size {
+        if dma_unmap.size != u64::try_from(size).unwrap() {
             return Err(VfioError::InvalidDmaUnmapSize);
         }
 
@@ -319,29 +341,68 @@ impl VfioContainer {
     ///
     /// # Parameters
     /// * mem: pinned guest memory which could be accessed by devices binding to the container.
-    pub fn vfio_map_guest_memory<M: GuestMemory>(&self, mem: &M) -> Result<()> {
+    ///
+    /// # Safety
+    ///
+    /// Each of the memory regions must uphold the safety invariants of [`Self::vfio_dma_map`].
+    /// Additionally, the [`GuestMemory`] implementation must be well-behaved and uphold the
+    /// contracts in its documentation.
+    ///
+    /// If this function fails (returning [`core::result::Result::Err`]) there is no way to know
+    /// which parts of the guest memory were successfully mapped.  The only safe action to take
+    /// to avoid undefined guest behavior is to crash the guest.
+    ///
+    /// # Errors
+    ///
+    /// Fails if any of the mapping operations fail.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the length of one of the regions overflows `usize`.
+    pub unsafe fn vfio_map_guest_memory<M: GuestMemory>(&self, mem: &M) -> Result<()> {
         mem.iter().try_for_each(|region| {
             let host_addr = region
                 .get_host_address(MemoryRegionAddress(0))
                 .map_err(|_| VfioError::GetHostAddress)?;
-            self.vfio_dma_map(
-                region.start_addr().raw_value(),
-                region.len(),
-                host_addr as u64,
-            )
+            // SAFETY: GuestMemory guarantees the requirements
+            // are upheld.
+            unsafe {
+                self.vfio_dma_map(
+                    region.start_addr().raw_value(),
+                    region.len().try_into().unwrap(),
+                    host_addr,
+                )
+            }
         })
     }
 
     /// Remove all guest memory regions from the vfio container's iommu table.
     ///
-    /// The vfio kernel driver and device hardware couldn't access this guest memory after
-    /// returning from the function.
+    /// The vfio kernel driver and device hardware can't access this guest memory after
+    /// the function returns successfully, **provided that the following precondition holds**.
+    ///
+    /// # Precondition
+    ///
+    /// A previous call to [`Self::vfio_map_guest_memory`] must have succeeded, and iterating
+    /// over the regions in the [`GuestMemory`] must produce the same values it did in the
+    /// past.  This latter contract will be upheld by a correct [`GuestMemory`] implementation,
+    /// but [`GuestMemory`] is a safe trait and so unsafe code must not rely on this unless it
+    /// is explicitly part of a safety contract.
     ///
     /// # Parameters
     /// * mem: pinned guest memory which could be accessed by devices binding to the container.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the length of any of the regions overflows `usize`.  That should have been
+    /// caught by [`Self::vfio_map_guest_memory`], so it indicates a bogus [`GuestMemory`]
+    /// implementation.
     pub fn vfio_unmap_guest_memory<M: GuestMemory>(&self, mem: &M) -> Result<()> {
         mem.iter().try_for_each(|region| {
-            self.vfio_dma_unmap(region.start_addr().raw_value(), region.len())
+            self.vfio_dma_unmap(
+                region.start_addr().raw_value(),
+                region.len().try_into().unwrap(),
+            )
         })
     }
 
@@ -1358,8 +1419,10 @@ mod tests {
         container.put_group(group3);
         assert_eq!(Arc::strong_count(&group), 1);
 
-        container.vfio_dma_map(0x1000, 0x1000, 0x8000).unwrap();
-        container.vfio_dma_map(0x2000, 0x2000, 0x8000).unwrap_err();
+        // SAFETY: this is a test implementation that does not access memory
+        unsafe { container.vfio_dma_map(0x1000, 0x1000, 0x8000 as _) }.unwrap();
+        // SAFETY: this is a test implementation that does not access memory
+        unsafe { container.vfio_dma_map(0x2000, 0x2000, 0x8000 as _) }.unwrap_err();
         container.vfio_dma_unmap(0x1000, 0x1000).unwrap();
         container.vfio_dma_unmap(0x2000, 0x2000).unwrap_err();
     }
@@ -1506,7 +1569,9 @@ mod tests {
         let mem1 = GuestMemoryMmap::<()>::from_ranges(&[(addr1, 0x1000)]).unwrap();
         let container = create_vfio_container();
 
-        container.vfio_map_guest_memory(&mem1).unwrap();
+        // SAFETY: This is a dummy implementation that does not access
+        // memory.
+        unsafe { container.vfio_map_guest_memory(&mem1) }.unwrap();
 
         let addr2 = GuestAddress(0x3000);
         let mem2 = GuestMemoryMmap::<()>::from_ranges(&[(addr2, 0x1000)]).unwrap();

vfio-ioctls/src/vfio_ioctls.rs

@@ -85,12 +85,13 @@ pub(crate) mod vfio_syscall {
         }
     }
 
-    pub(crate) fn map_dma(
+    /// # Safety
+    ///
+    /// See [`VfioContainer::vfio_dma_map`]
+    pub(crate) unsafe fn map_dma(
         container: &VfioContainer,
         dma_map: &vfio_iommu_type1_dma_map,
     ) -> Result<()> {
-        // SAFETY: file is vfio container, dma_map is constructed by us, and
-        // we check the return value
         let ret = unsafe { ioctl_with_ref(container, VFIO_IOMMU_MAP_DMA(), dma_map) };
         if ret != 0 {
             Err(VfioError::IommuDmaMap(SysError::last()))
@@ -268,7 +269,7 @@ pub(crate) mod vfio_syscall {
         Ok(())
     }
 
-    pub(crate) fn map_dma(
+    pub(crate) unsafe fn map_dma(
         _container: &VfioContainer,
         dma_map: &vfio_iommu_type1_dma_map,
     ) -> Result<()> {