Mark VfioContainer::vfio_dma_map as unsafe

vfio_syscall::map_dma causes the kernel to make an arbitrary address
accessible for DMA by a device the guest typically controls.  This is
unsafe, as it can change memory that Rust assumes is immutable.

I found this through a review of Cloud Hypervisor [1], where I saw a
safe function that took a host address cast to u64 as an argument and
accessed that address.  It turns out that this function was the source
of the unsoundness.

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

Author: DemiMarie

vfio-ioctls/CHANGELOG.md

@@ -1,3 +1,16 @@
+# Upcoming Release
+
+## Changed
+
+- Functions that map memory into the VFIO device are now `unsafe`.
+  The caller of these functions are responsible for enforcing
+  various complex, documented invariants.  These functions were
+  never actually safe to begin with, but in the past they had
+  (incorrectly) not been marked as `unsafe`.
+
+  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,36 @@ 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)`, it is not safe to use any of the address range in
+    /// `[user_addr..user_addr+size]` for almost any purpose.  The only permitted purposes are
+    ///
+    /// - Atomic and/or volatile operations.
+    /// - Assignment to a guest.
+    /// - Sharing with another process.
+    /// - 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 +320,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 +340,54 @@ 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
+    ///
+    /// You have to promise that the requirements of [`Self::vfio_dma_map`] are upheld for each
+    /// of the regions.  A future version of the crate will ensure that they are upheld by having
+    /// `vfio-ioctls` own a reference to the memory mapped into the guest.  You also have to
+    /// promise that the [`GuestMemory`] implementation is well-behaved and upholds the contracts
+    /// in its documentation.
+    ///
+    /// # 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.
     ///
     /// # 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 +1404,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 +1554,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<()> {