diff --git a/vfio-ioctls/CHANGELOG.md b/vfio-ioctls/CHANGELOG.md index be38432..61b0c36 100644 --- a/vfio-ioctls/CHANGELOG.md +++ b/vfio-ioctls/CHANGELOG.md @@ -4,6 +4,16 @@ - [[114]](https://github.com/rust-vmm/vfio/pull/114) Cargo.toml: Update deps to latest version +- [[103]](https://github.com/rust-vmm/vfio/pull/103) 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. + ## Added ## Fixed diff --git a/vfio-ioctls/src/vfio_device.rs b/vfio-ioctls/src/vfio_device.rs index 12221f2..fe66572 100644 --- a/vfio-ioctls/src/vfio_device.rs +++ b/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::() >= mem::size_of::<*mut u8>()); + const _: () = assert!(mem::size_of::() >= mem::size_of::()); let dma_map = vfio_iommu_type1_dma_map { argsz: mem::size_of::() 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,17 +321,17 @@ 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::() as u32, flags: 0, iova, - size, + size: size.try_into().unwrap(), ..Default::default() }; 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); } @@ -320,29 +342,68 @@ impl VfioContainer { /// /// # Parameters /// * mem: pinned guest memory which could be accessed by devices binding to the container. - pub fn vfio_map_guest_memory(&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(&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(&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(), + ) }) } @@ -1361,8 +1422,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(); } @@ -1509,7 +1572,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(); diff --git a/vfio-ioctls/src/vfio_ioctls.rs b/vfio-ioctls/src/vfio_ioctls.rs index 9bc5ea2..8ad0e92 100644 --- a/vfio-ioctls/src/vfio_ioctls.rs +++ b/vfio-ioctls/src/vfio_ioctls.rs @@ -85,12 +85,15 @@ 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 + // SAFETY: File is vfio container and the ioctl number is valid. Other + // invariants are the caller's responsibility. let ret = unsafe { ioctl_with_ref(container, VFIO_IOMMU_MAP_DMA(), dma_map) }; if ret != 0 { Err(VfioError::IommuDmaMap(SysError::last())) @@ -268,7 +271,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<()> {