diff --git a/pci/src/vfio.rs b/pci/src/vfio.rs index 30bf9fbfe..a1c28c911 100644 --- a/pci/src/vfio.rs +++ b/pci/src/vfio.rs @@ -1703,17 +1703,22 @@ impl VfioPciDevice { .map_err(VfioPciError::CreateUserMemoryRegion)?; if !self.iommu_attached { - self.container - .vfio_dma_map( + // vfio_dma_map should be unsafe but isn't. + #[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( + // vfio_dma_map is unsound and ought to be marked as unsafe + #[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| { - io::Error::other(format!( - "Could not map mmio region to vfio container: \ + } + .map_err(|e| VfioPciError::DmaMap(e, self.device_path.clone(), self.bdf)) + .map_err(|e| { + io::Error::other(format!( + "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 ExternalDmaMapping for VfioDmaMapping 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 - .vfio_dma_map(iova, size, (user_addr as usize).try_into().unwrap()) - .map_err(|e| { - io::Error::other(format!( - "failed to map memory for VFIO container, \ + unsafe { + self.container + .vfio_dma_map(iova, size, (user_addr as usize).try_into().unwrap()) + } + .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> { diff --git a/virtio-devices/src/mem.rs b/virtio-devices/src/mem.rs index 06fd160d3..eda263c0f 100644 --- a/virtio-devices/src/mem.rs +++ b/virtio-devices/src/mem.rs @@ -399,7 +399,7 @@ impl BlocksState { struct MemEpollHandler { mem: GuestMemoryAtomic, - host_addr: u64, + region: Arc, host_fd: Option, blocks_state: Arc>, config: Arc>, @@ -412,8 +412,45 @@ struct MemEpollHandler { dma_mapping_handlers: Arc>>>, } +fn usize_to_u64(i: usize) -> u64 { + const _: () = assert!(size_of::() <= size_of::()); + 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, host_fd: Option, config: Arc>, 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(), diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 622c183f0..81a061aad 100644 --- a/vmm/src/device_manager.rs +++ b/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( + // vfio_dma_map is unsound and ought to be marked as unsafe + #[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( + // vfio_dma_map is unsound and ought to be marked as unsafe + #[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.