Skip to content

Commit

Permalink
add marker trait to help check safety of guest memory reads
Browse files Browse the repository at this point in the history
we noted that a pointer into guest memory must point to a
properly-initialized T when read into Propolis, but there was no way to
actually check that was a case. for example, it may be tempting to write
an enum describing states of a guest device like:
```
enum MyCoolDevicePower {
  Off = 0,
  On = 1,
}
```
and read/write to guest memory using the convenient read/write helpers.
but a devious guest could put a `2` at that address, where reading that
into Propolis would be UB.

so, add a new `unsafe trait AlwaysInhabited` that `MemCtx::read` and friends can
rely on as attestation that they are safe to read guest memory as a given type.
impl this new trait for the handful of types we read from guest memory, as well.
  • Loading branch information
iximeow committed Oct 17, 2024
1 parent 93ed767 commit 4048671
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 9 deletions.
3 changes: 3 additions & 0 deletions lib/propolis/src/hw/nvme/bits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ pub struct SubmissionQueueEntry {
pub cdw15: u32,
}

/// Safety: all fields of SubmissionQueueEntry are valid for all bit patterns.
unsafe impl crate::vmm::AlwaysInhabited for SubmissionQueueEntry {}

impl SubmissionQueueEntry {
/// Returns the Identifier (CID) of this Submission Queue Entry.
///
Expand Down
5 changes: 5 additions & 0 deletions lib/propolis/src/hw/qemu/fwcfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,11 @@ mod bits {
pub len: BE32,
pub addr: BE64,
}

/// Safety: all fields of FwCfgDmaAccess are valid for all bit patterns.
///
/// They're just big-endian instead of little-endian.
unsafe impl crate::vmm::AlwaysInhabited for FwCfgDmaAccess {}
}

#[cfg(test)]
Expand Down
2 changes: 2 additions & 0 deletions lib/propolis/src/hw/virtio/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ struct VqdDesc {
flags: u16,
next: u16,
}
/// Safety: all fields of VqdDesc are valid for all bit patterns
unsafe impl crate::vmm::AlwaysInhabited for VqdDesc {}
#[repr(C)]
#[derive(Copy, Clone, Debug)]
struct VqdUsed {
Expand Down
60 changes: 51 additions & 9 deletions lib/propolis/src/vmm/mem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,39 @@ use crate::common::{GuestAddr, GuestRegion};
use crate::util::aspace::ASpace;
use crate::vmm::VmmHdl;

/// Marker trait declaring that a type is valid for all bit patterns. This is
/// one of the necessary requirements to read a type from guest memory.
///
/// Notably a type like
/// ```rust
/// #[repr(u8)]
/// enum Foo {
/// A,
/// B,
/// C,
/// Other(u8)
/// }
/// ```
///
/// *can not* impl `AlwaysInhabited`: the discriminant may only be one of four
/// bit pattens.
///
/// TODO: it'd be nice to have a proc macro to derive this on structs comprised
/// entirely of `AlwaysInhabited` types..
pub unsafe trait AlwaysInhabited {}

/// All bit patterns are valid `u8`.
unsafe impl AlwaysInhabited for u8 {}

/// All bit patterns are valid `u16`.
unsafe impl AlwaysInhabited for u16 {}

/// All bit patterns are valid `u64`.
unsafe impl AlwaysInhabited for u64 {}

/// All bit patterns are valid `u8`, so they are valid `MaybeUninit<u8>` too.
unsafe impl AlwaysInhabited for MaybeUninit<u8> {}

bitflags! {
/// Bitflags representing memory protections.
#[derive(Debug, Copy, Clone)]
Expand Down Expand Up @@ -480,21 +513,25 @@ impl<'a> SubMapping<'a> {
}

/// Reads a `T` object from the mapping.
pub fn read<T: Copy>(&self) -> Result<T> {
pub fn read<T: Copy + AlwaysInhabited>(&self) -> Result<T> {
self.check_read_access()?;
let typed = self.ptr.as_ptr() as *const T;
if self.len < std::mem::size_of::<T>() {
return Err(Error::new(ErrorKind::InvalidData, "Buffer too small"));
}

// Safety:
// - typed must be valid for reads
// - typed must point to a properly initialized value of T
// - typed must be valid for reads: `check_read_access()` succeeded
// - typed must point to a properly initialized value of T: always true
// because we require `T: AlwaysInhabited`
Ok(unsafe { typed.read_unaligned() })
}

/// Read `values` from the mapping.
pub fn read_many<T: Copy>(&self, values: &mut [T]) -> Result<()> {
pub fn read_many<T: Copy + AlwaysInhabited>(
&self,
values: &mut [T],
) -> Result<()> {
self.check_read_access()?;
let copy_len = size_of_val(values);
if self.len < copy_len {
Expand All @@ -508,11 +545,13 @@ impl<'a> SubMapping<'a> {
// not guaranteed for the source pointer. Cast it down to a u8, which
// will appease those alignment concerns
let src = self.ptr.as_ptr() as *const u8;
// We know reinterpreting `*mut T` as `*mut u8` and writing to it cannot
// result in invalid `T` because `T: AlwaysInhabited`
let dst = values.as_mut_ptr() as *mut u8;

// Safety
// - `src` is valid for read for the `copy_len` as checked above
// - `src` is valid for writes for its entire length, since it is from a
// - `dst` is valid for writes for its entire length, since it is from a
// valid mutable reference passed in to us
// - both are aligned for a `u8` copy
// - `dst` cannot be overlapped by `src`, since the former came from a
Expand Down Expand Up @@ -788,7 +827,10 @@ pub struct MemCtx {
}
impl MemCtx {
/// Reads a generic value from a specified guest address.
pub fn read<T: Copy>(&self, addr: GuestAddr) -> Option<T> {
pub fn read<T: Copy + AlwaysInhabited>(
&self,
addr: GuestAddr,
) -> Option<T> {
if let Some(mapping) =
self.region_covered(addr, size_of::<T>(), Prot::READ)
{
Expand Down Expand Up @@ -828,7 +870,7 @@ impl MemCtx {
}

/// Reads multiple objects from a guest address.
pub fn read_many<T: Copy>(
pub fn read_many<T: Copy + AlwaysInhabited>(
&self,
base: GuestAddr,
count: usize,
Expand Down Expand Up @@ -1015,7 +1057,7 @@ pub struct MemMany<'a, T: Copy> {
pos: usize,
phantom: PhantomData<T>,
}
impl<'a, T: Copy> MemMany<'a, T> {
impl<'a, T: Copy + AlwaysInhabited> MemMany<'a, T> {
/// Gets the object at position `pos` within the memory region.
///
/// Returns [`Option::None`] if out of range.
Expand All @@ -1028,7 +1070,7 @@ impl<'a, T: Copy> MemMany<'a, T> {
}
}
}
impl<'a, T: Copy> Iterator for MemMany<'a, T> {
impl<'a, T: Copy + AlwaysInhabited> Iterator for MemMany<'a, T> {
type Item = T;

fn next(&mut self) -> Option<Self::Item> {
Expand Down

0 comments on commit 4048671

Please sign in to comment.