add marker trait to help check safety of guest memory reads

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)

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,

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,

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,21 +482,27 @@ impl<'a> SubMapping<'a> {
     }
 
     /// Reads a `T` object from the mapping.
-    pub fn read<T: Copy>(&self) -> Result<T> {
+    pub fn read<T: Copy + FromBytes>(&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: 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<T: Copy>(&self, values: &mut [T]) -> Result<()> {
+    pub fn read_many<T: Copy + FromBytes>(
+        &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<T: Copy>(&self, addr: GuestAddr) -> Option<T> {
+    pub fn read<T: Copy + FromBytes>(&self, addr: GuestAddr) -> Option<T> {
         if let Some(mapping) =
             self.region_covered(addr, size_of::<T>(), Prot::READ)
         {
@@ -828,7 +838,7 @@ impl MemCtx {
     }
 
     /// Reads multiple objects from a guest address.
-    pub fn read_many<T: Copy>(
+    pub fn read_many<T: Copy + FromBytes>(
         &self,
         base: GuestAddr,
         count: usize,
@@ -1015,7 +1025,7 @@ pub struct MemMany<'a, T: Copy> {
     pos: usize,
     phantom: PhantomData<T>,
 }
-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<Self::Item> {