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.

Author: iximeow

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> {