dsg/cloud-hypervisor
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

misc: do not use u64 to represent host pointers

Browse source 
To ensure that struct sizes are the same on 32-bit and 64-bit, various
kernel APIs use __u64 (Rust u64) to represent userspace pointers.
Userspace is expected to cast pointers to __u64 before passing them to
the kernel, and cast kernel-provided __u64 to a pointer before using
them.  However, various safe APIs in Cloud Hypervisor took
caller-provided u64 values and passed them to syscalls that interpret
them as userspace addresses.  Therefore, passing bad u64 values would
cause memory disclosure or corruption.

Fix the bug by using usize and pointer types as appropriate.  To make
soundness of the code easier to reason about, the PCI code gains a new
MmapRegion abstraction that ensures the validity of pointers.  The rest
of the code already has an MmapRegion abstraction it can use.  To avoid
having to reason about whether something is keeping the MmapRegion
alive, reference counting is added.  MmapRegion cannot hold references
to other objects, so the reference counting cannot introduce cycles.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
This commit is contained in:
Demi Marie Obenour 2025-06-13 14:27:00 -04:00 • committed by Rob Bradford
parent fdc19ad85e
commit 42522a88c0
17 changed files with 382 additions and 263 deletions
Download patch file Download diff file Expand all files Collapse all files

1
pci/src/lib.rs
View file

@ -11,6 +11,7 @@ extern crate log;
mod bus;
mod configuration;
mod device;
mod mmap;
mod msi;
mod msix;
mod vfio;

89
pci/src/mmap.rs Normal file
View file

@ -0,0 +1,89 @@
// Copyright © 2025 Demi Marie Obenour
//
// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
//! Helpers for `mmap()`
use core::ffi::c_int;
use core::ptr::null_mut;
use std::io::{Error, ErrorKind};
use std::os::fd::{AsRawFd as _, BorrowedFd};
use libc::size_t;
/// A region of `mmap()`-allocated memory that calls `munmap()` when dropped.
/// This guarantees that the buffer is valid and that its address space
/// will be reserved. The address space is not guaranteed to be accessible.
/// Atomic access to the data will not cause undefined behavior but might
/// cause SIGSEGV or SIGBUS. Non-atomic access will generally cause data
/// races and thus Undefined Behavior.
#[derive(Debug)]
pub struct MmapRegion {
addr: *mut u8,
len: size_t,
}
impl Drop for MmapRegion {
fn drop(&mut self) {
// SAFETY: guaranteed by type validity invariant
unsafe { assert_eq!(libc::munmap(self.addr as *mut _, self.len), 0) }
}
}
// SAFETY: the caller is responsible for avoiding data races
unsafe impl Send for MmapRegion {}
// SAFETY: the caller is responsible for avoiding data races
unsafe impl Sync for MmapRegion {}
impl MmapRegion {
#[inline]
pub fn addr(&self) -> *mut u8 {
self.addr
}
/// Return the length of the region.
/// This function promises that the return value fits in [`libc::size_t`]
/// and in [`isize`] and `unsafe` code can rely on this.
#[inline]
pub fn len(&self) -> usize {
self.len
}
/// Create an [`MmapRegion`] using `mmap` of a file descriptor.
pub fn mmap(
len: u64,
prot: c_int,
fd: BorrowedFd,
offset1: u64,
offset2: u64,
) -> std::io::Result<Self> {
const BAD_LENGTH: &str = "Offsets must fit in libc::off_t";
const BAD_OFFSET: &str = "Mapping length must fit \
in both isize and libc::size_t";
let Some(offset) = offset1.checked_add(offset2) else {
return Err(Error::new(ErrorKind::InvalidInput, BAD_OFFSET));
};
let Ok(offset) = libc::off_t::try_from(offset) else {
return Err(Error::new(ErrorKind::InvalidInput, BAD_OFFSET));
};
if isize::try_from(len).is_err() {
return Err(Error::new(ErrorKind::InvalidInput, BAD_LENGTH));
}
let Ok(len) = libc::size_t::try_from(len) else {
return Err(Error::new(ErrorKind::InvalidInput, BAD_LENGTH));
};
assert!(
(prot & !(libc::PROT_READ | libc::PROT_WRITE | libc::PROT_EXEC)) == 0,
"bad protection"
);
let flags = libc::MAP_SHARED;
// SAFETY: FFI call with correct parameters.
let addr = unsafe { libc::mmap(null_mut(), len, prot, flags, fd.as_raw_fd(), offset) };
if addr == libc::MAP_FAILED {
Err(Error::last_os_error())
} else {
let addr = addr as _;
Ok(Self { addr, len })
}
}
}

209
pci/src/vfio.rs
View file

@ -6,9 +6,9 @@
use std::any::Any;
use std::collections::{BTreeMap, HashMap};
use std::io;
use std::os::fd::BorrowedFd;
use std::os::unix::io::AsRawFd;
use std::path::PathBuf;
use std::ptr::null_mut;
use std::sync::{Arc, Barrier, Mutex};
use anyhow::anyhow;
@ -34,6 +34,7 @@ use vm_memory::{Address, GuestAddress, GuestAddressSpace, GuestMemory, GuestUsiz
use vm_migration::{Migratable, MigratableError, Pausable, Snapshot, Snapshottable, Transportable};
use vmm_sys_util::eventfd::EventFd;
use crate::mmap::MmapRegion;
use crate::msi::{MSI_CONFIG_ID, MsiConfigState};
use crate::msix::MsixConfigState;
use crate::{
@ -258,12 +259,11 @@ impl Interrupt {
}
}
#[derive(Copy, Clone)]
#[derive(Clone)]
pub struct UserMemoryRegion {
pub slot: u32,
pub start: u64,
pub size: u64,
pub host_addr: u64,
pub mapping: Arc<crate::mmap::MmapRegion>,
}
#[derive(Clone)]
@ -275,12 +275,17 @@ pub struct MmioRegion {
pub(crate) user_memory_regions: Vec<UserMemoryRegion>,
}
trait MmioRegionRange {
/// # Safety
///
/// [`Self::find_user_address`] must always either return `Err`
/// or a pointer to `size` bytes of valid memory.
unsafe trait MmioRegionRange {
fn check_range(&self, guest_addr: u64, size: u64) -> bool;
fn find_user_address(&self, guest_addr: u64) -> Result<u64, io::Error>;
fn find_user_address(&self, guest_addr: u64, size: u64) -> Result<*mut u8, io::Error>;
}
impl MmioRegionRange for Vec<MmioRegion> {
// SAFETY: See the comment in `find_user_address`.
unsafe impl MmioRegionRange for Vec<MmioRegion> {
// Check if a guest address is within the range of mmio regions
fn check_range(&self, guest_addr: u64, size: u64) -> bool {
for region in self.iter() {
@ -298,14 +303,33 @@ impl MmioRegionRange for Vec<MmioRegion> {
}
// Locate the user region address for a guest address within all mmio regions
fn find_user_address(&self, guest_addr: u64) -> Result<u64, io::Error> {
fn find_user_address(&self, guest_addr: u64, size: u64) -> Result<*mut u8, io::Error> {
for region in self.iter() {
for user_region in region.user_memory_regions.iter() {
if guest_addr >= user_region.start
&& guest_addr < user_region.start + user_region.size
{
return Ok(user_region.host_addr + (guest_addr - user_region.start));
let mapping: &MmapRegion = &user_region.mapping;
let start: u64 = user_region.start;
let len: u64 = mapping.len().try_into().unwrap();
// See if the guest address is inside the region.
let Some(offset_from_start) = guest_addr.checked_sub(start) else {
continue;
};
if offset_from_start >= len {
continue;
}
// Check that the size is in bounds.
// This enforces the invariant promised by implementing MmioRegionRange.
assert!(
size <= len - offset_from_start,
"Attempt to read {size} bytes at offset {offset_from_start} into \
a region of size {len}"
);
// SAFETY: MmapRegion guarantees that mapping.addr points to at least
// mapping.len() bytes of valid memory. offset_from_start is equal
// to guest_addr - start, which was checked to be less than mapping.len().
// Therefore, the returned pointer is still in the range of valid memory.
// Also, since mapping.len() fit in usize, offset_from_start must as well,
// so the cast is safe.
return Ok(unsafe { mapping.addr().add(offset_from_start as usize) });
}
}
@ -1585,7 +1609,8 @@ impl VfioPciDevice {
/// * `mem_slot` - The closure to return a memory slot.
pub fn map_mmio_regions(&mut self) -> Result<(), VfioPciError> {
let fd = self.device.as_raw_fd();
// SAFETY: fd is guaranteed valid
let fd = unsafe { BorrowedFd::borrow_raw(fd) };
for region in self.common.mmio_regions.iter_mut() {
let region_flags = self.device.get_region_flags(region.index);
if region_flags & VFIO_REGION_INFO_FLAG_MMAP != 0 {
@ -1625,71 +1650,72 @@ impl VfioPciDevice {
self.common.interrupt.msix.as_ref(),
)?;
for area in sparse_areas.iter() {
// SAFETY: FFI call with correct arguments
let host_addr = unsafe {
libc::mmap(
null_mut(),
area.size as usize,
prot,
libc::MAP_SHARED,
fd,
mmap_offset as libc::off_t + area.offset as libc::off_t,
)
};
if std::ptr::eq(host_addr, libc::MAP_FAILED) {
for area in &sparse_areas {
if !is_page_size_aligned(area.size) || !is_page_size_aligned(area.offset) {
error!(
"Could not mmap sparse area (offset = 0x{:x}, size = 0x{:x}): {}",
area.offset,
area.size,
std::io::Error::last_os_error()
"Could not mmap sparse area that is not page size aligned \
(offset = 0x{:x}, size = 0x{:x})",
area.offset, area.size
);
return Err(VfioPciError::MmapArea);
}
}
if !is_page_size_aligned(area.size) || !is_page_size_aligned(area.offset) {
warn!(
"Could not mmap sparse area that is not page size aligned (offset = 0x{:x}, size = 0x{:x})",
area.offset, area.size,
);
return Ok(());
}
for area in sparse_areas.iter() {
let mapping = match MmapRegion::mmap(
area.size,
prot,
fd,
mmap_offset,
area.offset,
) {
Ok(mapping) => mapping,
Err(_) => {
error!(
"Could not mmap sparse area (offset = 0x{:x}, size = 0x{:x}): {}",
mmap_offset,
area.size,
std::io::Error::last_os_error()
);
return Err(VfioPciError::MmapArea);
}
};
let user_memory_region = UserMemoryRegion {
slot: self.memory_slot_allocator.next_memory_slot(),
start: region.start.0 + area.offset,
size: area.size,
host_addr: host_addr as u64,
mapping: Arc::new(mapping),
};
// SAFETY: host_addr was allocated by mmap() and points to size
// bytes of memory.
// 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.vm.create_user_memory_region(
user_memory_region.slot,
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
user_memory_region.mapping.len(),
user_memory_region.mapping.addr(),
false,
false,
)
}
.map_err(VfioPciError::CreateUserMemoryRegion)?;
region.user_memory_regions.push(user_memory_region);
if !self.iommu_attached {
self.container
.vfio_dma_map(
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
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)
})?;
}
region.user_memory_regions.push(user_memory_region);
}
}
}
@ -1698,19 +1724,21 @@ impl VfioPciDevice {
}
pub fn unmap_mmio_regions(&mut self) {
for region in self.common.mmio_regions.iter() {
for user_memory_region in region.user_memory_regions.iter() {
for region in self.common.mmio_regions.iter_mut() {
for user_memory_region in region.user_memory_regions.drain(..) {
let len = user_memory_region.mapping.len();
let host_addr = user_memory_region.mapping.addr();
// Unmap from vfio container
if !self.iommu_attached
&& let Err(e) = self
.container
.vfio_dma_unmap(user_memory_region.start, user_memory_region.size)
.vfio_dma_unmap(user_memory_region.start, len.try_into().unwrap())
.map_err(|e| VfioPciError::DmaUnmap(e, self.device_path.clone(), self.bdf))
{
error!(
"Could not unmap mmio region from vfio container: \
iova 0x{:x}, size 0x{:x}: {}, ",
user_memory_region.start, user_memory_region.size, e
user_memory_region.start, len, e
);
}
@ -1721,8 +1749,8 @@ impl VfioPciDevice {
self.vm.remove_user_memory_region(
user_memory_region.slot,
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
len,
host_addr,
false,
false,
)
@ -1732,21 +1760,6 @@ impl VfioPciDevice {
self.memory_slot_allocator
.free_memory_slot(user_memory_region.slot);
// SAFETY: FFI call with correct arguments
let ret = unsafe {
libc::munmap(
user_memory_region.host_addr as *mut libc::c_void,
user_memory_region.size as usize,
)
};
if ret != 0 {
error!(
"Could not unmap region {}, error:{}",
region.index,
io::Error::last_os_error()
);
}
}
}
}
@ -1886,29 +1899,31 @@ impl PciDevice for VfioPciDevice {
region.start = GuestAddress(new_base);
for user_memory_region in region.user_memory_regions.iter_mut() {
let len = user_memory_region.mapping.len();
let host_addr = user_memory_region.mapping.addr();
// Unmap the old MMIO region from vfio container
if !self.iommu_attached
&& let Err(e) = self
.container
.vfio_dma_unmap(user_memory_region.start, user_memory_region.size)
.vfio_dma_unmap(user_memory_region.start, len.try_into().unwrap())
.map_err(|e| {
VfioPciError::DmaUnmap(e, self.device_path.clone(), self.bdf)
})
{
error!(
"Could not unmap mmio region from vfio container: \
iova 0x{:x}, size 0x{:x}: {}, ",
user_memory_region.start, user_memory_region.size, e
iova 0x{:x}, size 0x{:x}: {}, ",
user_memory_region.start, len, e
);
}
// Remove old region
// SAFETY: user_memory_regions has valid entries
// SAFETY: validity of len and host_addr guaranteed by hypervisor::mmap::MmapRegion
unsafe {
self.vm.remove_user_memory_region(
user_memory_region.slot,
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
len,
host_addr,
false,
false,
)
@ -1923,13 +1938,13 @@ impl PciDevice for VfioPciDevice {
}
// Insert new region
// SAFETY: mmio_regions only has valid values
// SAFETY: validity of len and host_addr guaranteed by hypervisor::mmap::MmapRegion
unsafe {
self.vm.create_user_memory_region(
user_memory_region.slot,
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
len,
host_addr,
false,
false,
)
@ -1941,8 +1956,8 @@ impl PciDevice for VfioPciDevice {
self.container
.vfio_dma_map(
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
len.try_into().unwrap(),
(host_addr as usize).try_into().unwrap(),
)
.map_err(|e| {
VfioPciError::DmaMap(e, self.device_path.clone(), self.bdf)
@ -1950,8 +1965,8 @@ impl PciDevice for VfioPciDevice {
.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, user_memory_region.size, e
iova 0x{:x}, size 0x{:x}: {}, ",
user_memory_region.start, len, e
))
})?;
}
@ -1987,6 +2002,7 @@ impl Snapshottable for VfioPciDevice {
Ok(vfio_pci_dev_snapshot)
}
}
impl Transportable for VfioPciDevice {}
impl Migratable for VfioPciDevice {}
@ -2020,11 +2036,17 @@ impl<M: GuestAddressSpace> VfioDmaMapping<M> {
impl<M: GuestAddressSpace + Sync + Send> ExternalDmaMapping for VfioDmaMapping<M> {
fn map(&self, iova: u64, gpa: u64, size: u64) -> std::result::Result<(), io::Error> {
let Ok(usize_size): Result<usize, _> = size.try_into() else {
return Err(io::Error::other(format!("size {size} overflows usize")));
};
let mem = self.memory.memory();
let guest_addr = GuestAddress(gpa);
let user_addr = if mem.check_range(guest_addr, size as usize) {
match mem.get_host_address(guest_addr) {
Ok(t) => t as u64,
let user_addr = if mem.check_range(guest_addr, usize_size) {
match mem.get_slice(guest_addr, usize_size) {
Ok(t) => {
assert!(t.len() >= usize_size);
Ok(t.ptr_guard_mut())
}
Err(e) => {
return Err(io::Error::other(format!(
"unable to retrieve user address for gpa 0x{gpa:x} from guest memory region: {e}"
@ -2032,15 +2054,26 @@ impl<M: GuestAddressSpace + Sync + Send> ExternalDmaMapping for VfioDmaMapping<M
}
}
} else if self.mmio_regions.lock().unwrap().check_range(gpa, size) {
self.mmio_regions.lock().unwrap().find_user_address(gpa)?
Err(self
.mmio_regions
.lock()
.unwrap()
.find_user_address(gpa, size)?)
} else {
return Err(io::Error::other(format!(
"failed to locate guest address 0x{gpa:x} in guest memory"
)));
};
let user_addr = match user_addr {
Ok(p) => p.as_ptr(),
Err(p) => p,
};
// 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)
.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, \

96
pci/src/vfio_user.rs
View file

@ -4,8 +4,8 @@
//
use std::any::Any;
use std::os::fd::AsFd;
use std::os::unix::prelude::AsRawFd;
use std::ptr::null_mut;
use std::sync::{Arc, Barrier, Mutex};
use hypervisor::HypervisorVmError;
@ -24,6 +24,7 @@ use vm_memory::{
use vm_migration::{Migratable, MigratableError, Pausable, Snapshot, Snapshottable, Transportable};
use vmm_sys_util::eventfd::EventFd;
use crate::mmap::MmapRegion;
use crate::vfio::{UserMemoryRegion, VFIO_COMMON_ID, Vfio, VfioCommon, VfioError};
use crate::{
BarReprogrammingParams, PciBarConfiguration, PciBdf, PciDevice, PciDeviceError, PciSubclass,
@ -52,6 +53,8 @@ pub enum VfioUserPciDeviceError {
InitializeLegacyInterrupts(#[source] VfioPciError),
#[error("Failed to create VfioCommon")]
CreateVfioCommon(#[source] VfioPciError),
#[error("Other OS error")]
Other(#[source] std::io::Error),
}
#[derive(Copy, Clone)]
@ -110,10 +113,8 @@ impl VfioUserPciDevice {
})
}
/// # Safety
///
/// Not known yet (TODO)
pub unsafe fn map_mmio_regions(&mut self) -> Result<(), VfioUserPciDeviceError> {
/// Map all of the MMIO regions.
pub fn map_mmio_regions(&mut self) -> Result<(), VfioUserPciDeviceError> {
for mmio_region in &mut self.common.mmio_regions {
let region_flags = self
.client
@ -158,43 +159,39 @@ impl VfioUserPciDevice {
sparse_areas
};
for s in mmaps.iter() {
// SAFETY: FFI call with correct arguments
let host_addr = unsafe {
libc::mmap(
null_mut(),
s.size as usize,
prot,
libc::MAP_SHARED,
file_offset.as_ref().unwrap().file().as_raw_fd(),
file_offset.as_ref().unwrap().start() as libc::off_t
+ s.offset as libc::off_t,
)
};
let file_offset = file_offset.as_ref().unwrap();
if std::ptr::eq(host_addr, libc::MAP_FAILED) {
error!(
"Could not mmap regions, error:{}",
std::io::Error::last_os_error()
);
continue;
}
for s in mmaps.iter() {
let mapping = match MmapRegion::mmap(
s.size,
prot,
file_offset.file().as_fd(),
file_offset.start(),
s.offset,
) {
Ok(mapping) => Arc::new(mapping),
Err(e) => {
error!(
"Could not mmap sparse area (offset = 0x{:x}, size = 0x{:x}): {}",
s.offset, s.size, e
);
return Err(VfioUserPciDeviceError::Other(e));
}
};
let user_memory_region = UserMemoryRegion {
slot: self.memory_slot_allocator.next_memory_slot(),
start: mmio_region.start.0 + s.offset,
size: s.size,
host_addr: host_addr as u64,
mapping,
};
// SAFETY: host_addr was just allocated with mmap()
// and points to size bytes of valid address.
// SAFETY: validity of len and host_addr guaranteed by hypervisor::mmap::MmapRegion
unsafe {
self.vm.create_user_memory_region(
user_memory_region.slot,
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
user_memory_region.mapping.len(),
user_memory_region.mapping.addr(),
false,
false,
)
@ -209,17 +206,17 @@ impl VfioUserPciDevice {
Ok(())
}
pub fn unmap_mmio_regions(&mut self) {
for mmio_region in self.common.mmio_regions.iter() {
for user_memory_region in mmio_region.user_memory_regions.iter() {
fn unmap_mmio_regions(&mut self) {
for mmio_region in self.common.mmio_regions.iter_mut() {
for user_memory_region in mmio_region.user_memory_regions.drain(..) {
// Remove region
// SAFETY: only valid regions are in user_memory_regions
// SAFETY: guaranteed by hypervisor::mmap::MmapRegion invariants
if let Err(e) = unsafe {
self.vm.remove_user_memory_region(
user_memory_region.slot,
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
user_memory_region.mapping.len(),
user_memory_region.mapping.addr(),
false,
false,
)
@ -229,22 +226,7 @@ impl VfioUserPciDevice {
self.memory_slot_allocator
.free_memory_slot(user_memory_region.slot);
// Remove mmaps
// SAFETY: FFI call with correct arguments
let ret = unsafe {
libc::munmap(
user_memory_region.host_addr as *mut libc::c_void,
user_memory_region.size as usize,
)
};
if ret != 0 {
error!(
"Could not unmap region {}, error:{}",
mmio_region.index,
std::io::Error::last_os_error()
);
}
// memory will be unmapped on drop
}
}
}
@ -465,8 +447,8 @@ impl PciDevice for VfioUserPciDevice {
self.vm.remove_user_memory_region(
user_memory_region.slot,
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
user_memory_region.mapping.len(),
user_memory_region.mapping.addr(),
false,
false,
)
@ -486,8 +468,8 @@ impl PciDevice for VfioUserPciDevice {
self.vm.create_user_memory_region(
user_memory_region.slot,
user_memory_region.start,
user_memory_region.size,
user_memory_region.host_addr,
user_memory_region.mapping.len(),
user_memory_region.mapping.addr(),
false,
false,
)