misc: Work around vfio_dma_map being unsound

This API passes a u64 to a kernel API that treats the u64 as a userspace
address.  Therefore, it should be marked unsafe, but it currently is not
[1].  Wrap the call in an unsafe block to document that invariants must
be upheld to avoid undefined behavior.  This causes a compiler warning,
so suppress the warning with #[allow(unused_unsafe)].

[1]: https://github.com/rust-vmm/vfio/issues/100

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

pci/src/vfio.rs

@@ -1703,17 +1703,22 @@ impl VfioPciDevice {
                     .map_err(VfioPciError::CreateUserMemoryRegion)?;
 
                     if !self.iommu_attached {
-                        self.container
+                        // vfio_dma_map should be unsafe but isn't.
-                            .vfio_dma_map(
+                        #[allow(unused_unsafe)]
+                        // SAFETY: MmapRegion invariants guarantee that
+                        // user_memory_region.mapping.addr() points to
+                        // user_memory_region.mapping.len() bytes of
+                        // valid memory that will only be unmapped with munmap().
+                        unsafe {
+                            self.container.vfio_dma_map(
                                 user_memory_region.start,
                                 user_memory_region.mapping.len().try_into().unwrap(),
                                 (user_memory_region.mapping.addr() as usize)
                                     .try_into()
                                     .unwrap(),
                             )
-                            .map_err(|e| {
+                        }
-                                VfioPciError::DmaMap(e, self.device_path.clone(), self.bdf)
+                        .map_err(|e| VfioPciError::DmaMap(e, self.device_path.clone(), self.bdf))?;
-                            })?;
                     }
                     region.user_memory_regions.push(user_memory_region);
                 }
@@ -1745,6 +1750,7 @@ impl VfioPciDevice {
                 // Remove region
                 // SAFETY: only valid entries are added to the user_memory_regions field
                 // of the entries of self.common.mmio_regions.
+                // Also, host_addr..host_addr + len is valid by the MmapRegion invariants.
                 if let Err(e) = unsafe {
                     self.vm.remove_user_memory_region(
                         user_memory_region.slot,
@@ -1897,7 +1903,9 @@ iova 0x{:x}, size 0x{:x}: {}, ",
                         );
                     }
                     // Remove old region
-                    // SAFETY: validity of len and host_addr guaranteed by hypervisor::mmap::MmapRegion
+                    // SAFETY: MmapRegion invariants guarantee that
+                    // host_addr points to len bytes of
+                    // valid memory that will only be unmapped with munmap().
                     unsafe {
                         self.vm.remove_user_memory_region(
                             user_memory_region.slot,
@@ -1918,7 +1926,9 @@ iova 0x{:x}, size 0x{:x}: {}, ",
                     }
 
                     // Insert new region
-                    // SAFETY: validity of len and host_addr guaranteed by hypervisor::mmap::MmapRegion
+                    // SAFETY: MmapRegion invariants guarantee that
+                    // host_addr points to len bytes of
+                    // valid memory that will only be unmapped with munmap().
                     unsafe {
                         self.vm.create_user_memory_region(
                             user_memory_region.slot,
@@ -1933,22 +1943,26 @@ iova 0x{:x}, size 0x{:x}: {}, ",
 
                     // Map the moved mmio region to vfio container
                     if !self.iommu_attached {
-                        self.container
+                        // vfio_dma_map is unsound and ought to be marked as unsafe
-                            .vfio_dma_map(
+                        #[allow(unused_unsafe)]
+                        // SAFETY: MmapRegion invariants guarantee that
+                        // host_addr points to len bytes of
+                        // valid memory that will only be unmapped with munmap().
+                        unsafe {
+                            self.container.vfio_dma_map(
                                 user_memory_region.start,
                                 len.try_into().unwrap(),
                                 (host_addr as usize).try_into().unwrap(),
                             )
-                            .map_err(|e| {
+                        }
-                                VfioPciError::DmaMap(e, self.device_path.clone(), self.bdf)
+                        .map_err(|e| VfioPciError::DmaMap(e, self.device_path.clone(), self.bdf))
-                            })
+                        .map_err(|e| {
-                            .map_err(|e| {
+                            io::Error::other(format!(
-                                io::Error::other(format!(
+                                "Could not map mmio region to vfio container: \
-                                    "Could not map mmio region to vfio container: \
 iova 0x{:x}, size 0x{:x}: {}, ",
-                                    user_memory_region.start, len, e
+                                user_memory_region.start, len, e
-                                ))
+                            ))
-                            })?;
+                        })?;
                     }
                 }
             }
@@ -2049,17 +2063,21 @@ impl<M: GuestAddressSpace + Sync + Send> ExternalDmaMapping for VfioDmaMapping<M
             Err(p) => p,
         };
 
+        // vfio_dma_map is unsound and ought to be marked as unsafe
+        #[allow(unused_unsafe)]
         // SAFETY: find_user_address and GuestMemory::get_slice() guarantee that
         // the returned pointer is valid for up to `usize_size` bytes.
         // `usize_size` is always equal to `size` due to the above `try_into()` call.
-        self.container
+        unsafe {
-            .vfio_dma_map(iova, size, (user_addr as usize).try_into().unwrap())
+            self.container
-            .map_err(|e| {
+                .vfio_dma_map(iova, size, (user_addr as usize).try_into().unwrap())
-                io::Error::other(format!(
+        }
-                    "failed to map memory for VFIO container, \
+        .map_err(|e| {
+            io::Error::other(format!(
+                "failed to map memory for VFIO container, \
                          iova 0x{iova:x}, gpa 0x{gpa:x}, size 0x{size:x}: {e:?}"
-                ))
+            ))
-            })
+        })
     }
 
     fn unmap(&self, iova: u64, size: u64) -> std::result::Result<(), io::Error> {

virtio-devices/src/mem.rs

@@ -399,7 +399,7 @@ impl BlocksState {
 
 struct MemEpollHandler {
     mem: GuestMemoryAtomic<GuestMemoryMmap>,
-    host_addr: u64,
+    region: Arc<GuestRegionMmap>,
     host_fd: Option<RawFd>,
     blocks_state: Arc<Mutex<BlocksState>>,
     config: Arc<Mutex<VirtioMemConfig>>,
@@ -412,8 +412,45 @@ struct MemEpollHandler {
     dma_mapping_handlers: Arc<Mutex<BTreeMap<VirtioMemMappingSource, Arc<dyn ExternalDmaMapping>>>>,
 }
 
+fn usize_to_u64(i: usize) -> u64 {
+    const _: () = assert!(size_of::<usize>() <= size_of::<u64>());
+    i as _
+}
+
 impl MemEpollHandler {
+    /// # Panics
+    ///
+    /// Panics if any of the following hold:
+    /// - region size exceeds [`libc::off64_t::MAX`], [`libc::size_t::MAX`],
+    ///   or [`isize::MAX`]
+    /// - `size + offset` exceeds the size of the region (including overflow).
     fn discard_memory_range(&self, offset: u64, size: u64) -> Result<(), Error> {
+        let max_size = usize_to_u64(self.region.size());
+
+        // Validate the region size to ensure the below casts
+        // are lossless.
+        libc::size_t::try_from(max_size).unwrap();
+        libc::off64_t::try_from(max_size).unwrap();
+        isize::try_from(max_size).unwrap();
+
+        // Check that offset is in bounds.
+        assert!(max_size >= offset);
+
+        if size == 0 {
+            // Do not try to deallocate a zero size.
+            return Ok(());
+        }
+
+        // Check that offset + size is in bounds and does not overflow.
+        // Since size is checked to be nonzero above, this also means that
+        // offset is not past the end.
+        assert!(max_size - offset >= size);
+
+        // Since offset and size are each bounded above by max_size,
+        // and max_size came from usize and was checked to be able to be
+        // losslessly cast to size_t and off64_t, this also checks that offset
+        // and size can each be losslessly cast to all of these types.
+
         // Use fallocate if the memory region is backed by a file.
         if let Some(fd) = self.host_fd {
             // SAFETY: FFI call with valid arguments
@@ -435,10 +472,13 @@ impl MemEpollHandler {
         // Only use madvise if the memory region is not allocated with
         // hugepages.
         if !self.hugepages {
-            // SAFETY: FFI call with valid arguments
+            // SAFETY: FFI call with valid arguments.
+            // offset + madvize_size was checked in bounds above,
+            // and madvise_size is checked to not be zero so ptr_offset
+            // alone is not past the end.
             let res = unsafe {
                 libc::madvise(
-                    (self.host_addr + offset) as *mut libc::c_void,
+                    self.region.as_ptr().offset(offset as isize) as *mut libc::c_void,
                     size as libc::size_t,
                     libc::MADV_DONTNEED,
                 )
@@ -687,7 +727,7 @@ pub struct MemState {
 pub struct Mem {
     common: VirtioCommon,
     id: String,
-    host_addr: u64,
+    region: Arc<GuestRegionMmap>,
     host_fd: Option<RawFd>,
     config: Arc<Mutex<VirtioMemConfig>>,
     seccomp_action: SeccompAction,
@@ -780,7 +820,7 @@ impl Mem {
                 ..Default::default()
             },
             id,
-            host_addr: region.as_ptr() as u64,
+            region: region.clone(),
             host_fd,
             config: Arc::new(Mutex::new(config)),
             seccomp_action,
@@ -923,7 +963,7 @@ impl VirtioDevice for Mem {
 
         let mut handler = MemEpollHandler {
             mem,
-            host_addr: self.host_addr,
+            region: self.region.clone(),
             host_fd: self.host_fd,
             blocks_state: Arc::clone(&self.blocks_state),
             config: self.config.clone(),

vmm/src/device_manager.rs

@@ -3754,13 +3754,19 @@ impl DeviceManager {
             // virtio-mem device itself.
             for (_, zone) in self.memory_manager.lock().unwrap().memory_zones().iter() {
                 for region in zone.regions() {
-                    vfio_container
+                    // vfio_dma_map is unsound and ought to be marked as unsafe
-                        .vfio_dma_map(
+                    #[allow(unused_unsafe)]
+                    // SAFETY: GuestMemoryMmap guarantees that region points
+                    // to len bytes of valid memory starting at as_ptr()
+                    // that will only be freed with munmap().
+                    unsafe {
+                        vfio_container.vfio_dma_map(
                             region.start_addr().raw_value(),
                             region.len(),
                             region.as_ptr() as u64,
                         )
-                        .map_err(DeviceManagerError::VfioDmaMap)?;
+                    }
+                    .map_err(DeviceManagerError::VfioDmaMap)?;
                 }
             }
 
@@ -4368,13 +4374,19 @@ impl DeviceManager {
 
         // Take care of updating the memory for VFIO PCI devices.
         if let Some(vfio_container) = &self.vfio_container {
-            vfio_container
+            // vfio_dma_map is unsound and ought to be marked as unsafe
-                .vfio_dma_map(
+            #[allow(unused_unsafe)]
+            // SAFETY: GuestMemoryMmap guarantees that region points
+            // to len bytes of valid memory starting at as_ptr()
+            // that will only be freed with munmap().
+            unsafe {
+                vfio_container.vfio_dma_map(
                     new_region.start_addr().raw_value(),
                     new_region.len(),
                     new_region.as_ptr() as u64,
                 )
-                .map_err(DeviceManagerError::UpdateMemoryForVfioPciDevice)?;
+            }
+            .map_err(DeviceManagerError::UpdateMemoryForVfioPciDevice)?;
         }
 
         // Take care of updating the memory for vfio-user devices.