From ff0b76d5da5a4da1fbdf54b5ed7eaa8fd4093651 Mon Sep 17 00:00:00 2001 From: iximeow Date: Fri, 18 Oct 2024 02:01:21 +0000 Subject: [PATCH] add marker trait to help check safety of guest memory reads (#794) * 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. zerocopy::FromBytes happens to have the same requirements about its implementors as we need, that they're always valid to view from bytes, so use it to check that we can safely read a type out of guest memory. in our case we'll always copy those bytes to our own buffer, but zerocopy::FromBytes also comes with a great proc macro so we can #[derive(FromBytes)] on structs to be copied out. --- lib/propolis/src/hw/nvme/bits.rs | 3 ++- lib/propolis/src/hw/qemu/fwcfg.rs | 4 ++-- lib/propolis/src/hw/virtio/queue.rs | 4 +++- lib/propolis/src/vmm/mem.rs | 28 +++++++++++++++++++--------- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/lib/propolis/src/hw/nvme/bits.rs b/lib/propolis/src/hw/nvme/bits.rs index 0ae31b2fb..7407e06da 100644 --- a/lib/propolis/src/hw/nvme/bits.rs +++ b/lib/propolis/src/hw/nvme/bits.rs @@ -5,11 +5,12 @@ #![allow(dead_code)] use bitstruct::bitstruct; +use zerocopy::{FromBytes, FromZeroes}; /// A Submission Queue Entry as represented in memory. /// /// See NVMe 1.0e Section 4.2 Submission Queue Entry - Command Format -#[derive(Debug, Default, Copy, Clone)] +#[derive(Debug, Default, Copy, Clone, FromBytes, FromZeroes)] #[repr(C, packed(1))] pub struct SubmissionQueueEntry { /// Command Dword 0 (CDW0) diff --git a/lib/propolis/src/hw/qemu/fwcfg.rs b/lib/propolis/src/hw/qemu/fwcfg.rs index d3493ea83..2fb8b635f 100644 --- a/lib/propolis/src/hw/qemu/fwcfg.rs +++ b/lib/propolis/src/hw/qemu/fwcfg.rs @@ -815,7 +815,7 @@ mod bits { use zerocopy::byteorder::big_endian::{ U16 as BE16, U32 as BE32, U64 as BE64, }; - use zerocopy::AsBytes; + use zerocopy::{AsBytes, FromBytes, FromZeroes}; pub const FW_CFG_IOP_SELECTOR: u16 = 0x0510; pub const FW_CFG_IOP_DATA: u16 = 0x0511; @@ -867,7 +867,7 @@ mod bits { } } - #[derive(AsBytes, Default, Copy, Clone, Debug)] + #[derive(AsBytes, Default, Copy, Clone, Debug, FromBytes, FromZeroes)] #[repr(C)] pub struct FwCfgDmaAccess { pub ctrl: BE32, diff --git a/lib/propolis/src/hw/virtio/queue.rs b/lib/propolis/src/hw/virtio/queue.rs index fb841a16d..fdaa88d57 100644 --- a/lib/propolis/src/hw/virtio/queue.rs +++ b/lib/propolis/src/hw/virtio/queue.rs @@ -17,8 +17,10 @@ use crate::common::*; use crate::migrate::MigrateStateError; use crate::vmm::MemCtx; +use zerocopy::{FromBytes, FromZeroes}; + #[repr(C)] -#[derive(Copy, Clone)] +#[derive(Copy, Clone, FromBytes, FromZeroes)] struct VqdDesc { addr: u64, len: u32, diff --git a/lib/propolis/src/vmm/mem.rs b/lib/propolis/src/vmm/mem.rs index f6185ceb7..c54245e20 100644 --- a/lib/propolis/src/vmm/mem.rs +++ b/lib/propolis/src/vmm/mem.rs @@ -19,6 +19,8 @@ use crate::common::{GuestAddr, GuestRegion}; use crate::util::aspace::ASpace; use crate::vmm::VmmHdl; +use zerocopy::FromBytes; + bitflags! { /// Bitflags representing memory protections. #[derive(Debug, Copy, Clone)] @@ -480,7 +482,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 +490,19 @@ 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: FromBytes`. `zerocopy::FromBytes` happens + // to have the same concerns as us - that T is valid for all bit + // patterns. 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 +516,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: FromBytes` 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 +798,7 @@ 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 +838,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 +1025,7 @@ pub struct MemMany<'a, T: Copy> { pos: usize, phantom: PhantomData, } -impl<'a, T: Copy> MemMany<'a, T> { +impl<'a, T: Copy + FromBytes> MemMany<'a, T> { /// Gets the object at position `pos` within the memory region. /// /// Returns [`Option::None`] if out of range. @@ -1028,7 +1038,7 @@ impl<'a, T: Copy> MemMany<'a, T> { } } } -impl<'a, T: Copy> Iterator for MemMany<'a, T> { +impl<'a, T: Copy + FromBytes> Iterator for MemMany<'a, T> { type Item = T; fn next(&mut self) -> Option {