From 4048671d155c353abe1b012305041bd4a46cca8e Mon Sep 17 00:00:00 2001 From: iximeow Date: Thu, 17 Oct 2024 21:36:23 +0000 Subject: [PATCH] add marker trait to help check safety of guest memory reads 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. --- lib/propolis/src/hw/nvme/bits.rs | 3 ++ lib/propolis/src/hw/qemu/fwcfg.rs | 5 +++ lib/propolis/src/hw/virtio/queue.rs | 2 + lib/propolis/src/vmm/mem.rs | 60 ++++++++++++++++++++++++----- 4 files changed, 61 insertions(+), 9 deletions(-) diff --git a/lib/propolis/src/hw/nvme/bits.rs b/lib/propolis/src/hw/nvme/bits.rs index 0ae31b2fb..b19d66a8b 100644 --- a/lib/propolis/src/hw/nvme/bits.rs +++ b/lib/propolis/src/hw/nvme/bits.rs @@ -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. /// diff --git a/lib/propolis/src/hw/qemu/fwcfg.rs b/lib/propolis/src/hw/qemu/fwcfg.rs index d3493ea83..53db2802a 100644 --- a/lib/propolis/src/hw/qemu/fwcfg.rs +++ b/lib/propolis/src/hw/qemu/fwcfg.rs @@ -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)] diff --git a/lib/propolis/src/hw/virtio/queue.rs b/lib/propolis/src/hw/virtio/queue.rs index fb841a16d..ca927ce4f 100644 --- a/lib/propolis/src/hw/virtio/queue.rs +++ b/lib/propolis/src/hw/virtio/queue.rs @@ -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 { diff --git a/lib/propolis/src/vmm/mem.rs b/lib/propolis/src/vmm/mem.rs index f6185ceb7..19fb768f1 100644 --- a/lib/propolis/src/vmm/mem.rs +++ b/lib/propolis/src/vmm/mem.rs @@ -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` too. +unsafe impl AlwaysInhabited for MaybeUninit {} + bitflags! { /// Bitflags representing memory protections. #[derive(Debug, Copy, Clone)] @@ -480,7 +513,7 @@ impl<'a> SubMapping<'a> { } /// Reads a `T` object from the mapping. - pub fn read(&self) -> Result { + pub fn read(&self) -> Result { self.check_read_access()?; let typed = self.ptr.as_ptr() as *const T; if self.len < std::mem::size_of::() { @@ -488,13 +521,17 @@ impl<'a> SubMapping<'a> { } // 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(&self, values: &mut [T]) -> Result<()> { + pub fn read_many( + &self, + values: &mut [T], + ) -> Result<()> { self.check_read_access()?; let copy_len = size_of_val(values); if self.len < copy_len { @@ -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 @@ -788,7 +827,10 @@ pub struct MemCtx { } impl MemCtx { /// Reads a generic value from a specified guest address. - pub fn read(&self, addr: GuestAddr) -> Option { + pub fn read( + &self, + addr: GuestAddr, + ) -> Option { if let Some(mapping) = self.region_covered(addr, size_of::(), Prot::READ) { @@ -828,7 +870,7 @@ impl MemCtx { } /// Reads multiple objects from a guest address. - pub fn read_many( + pub fn read_many( &self, base: GuestAddr, count: usize, @@ -1015,7 +1057,7 @@ pub struct MemMany<'a, T: Copy> { pos: usize, phantom: PhantomData, } -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. @@ -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 {