NVMe admin commands should better mind their PRPs

The Identify and GetLogPage admin commands in NVMe should not assume
that the output buffers provided to them in the PRPs consist of a single
page-sized page-aligned entry.  Guest (such as Linux) can and will issue
those commands with a page offset in PRP1, splitting the output into
another page.

Fixes #427

Author: pfmooney

lib/propolis/src/hw/nvme/admin.rs

@@ -1,8 +1,7 @@
 use std::cmp::min;
 use std::mem::size_of;
 
-use crate::common::GuestAddr;
-use crate::common::PAGE_SIZE;
+use crate::common::{GuestAddr, GuestRegion, PAGE_SIZE};
 use crate::vmm::MemCtx;
 
 use super::bits::*;
@@ -186,14 +185,19 @@ impl NvmeCtrl {
         cmd: &cmds::GetLogPageCmd,
         mem: &MemCtx,
     ) -> cmds::Completion {
-        assert!((cmd.len as usize) < PAGE_SIZE);
-        let buf = cmd
+        if let Some(regions) = cmd
             .data(mem)
-            .next()
-            .expect("missing prp entry for log page response");
-        // TODO: actually keep a log that we can write back instead of all zeros
-        assert!(mem.write_byte(buf.0, 0, cmd.len as usize));
-        cmds::Completion::success()
+            .map(|r| mem.writable_region(&r))
+            .collect::<Option<Vec<_>>>()
+        {
+            // TODO: Keep a log to write back instead of 0s
+            for region in regions {
+                let _ = region.write_byte(0, region.len());
+            }
+            cmds::Completion::success()
+        } else {
+            cmds::Completion::generic_err(STS_DATA_XFER_ERR)
+        }
     }
 
     /// Service Identify command.
@@ -208,12 +212,16 @@ impl NvmeCtrl {
             IDENT_CNS_NAMESPACE => match cmd.nsid {
                 1 => {
                     assert!(size_of::<IdentifyNamespace>() <= PAGE_SIZE);
-                    let buf = cmd
-                        .data(mem)
-                        .next()
-                        .expect("missing prp entry for ident response");
-                    assert!(mem.write(buf.0, &self.ns_ident));
-                    cmds::Completion::success()
+                    match Self::write_admin_result(
+                        cmd.data(mem),
+                        &self.ns_ident,
+                        mem,
+                    ) {
+                        Some(_) => cmds::Completion::success(),
+                        None => {
+                            cmds::Completion::generic_err(STS_DATA_XFER_ERR)
+                        }
+                    }
                 }
                 // 0 is not a valid NSID (See NVMe 1.0e, Section 6.1 Namespaces)
                 // We also don't currently support namespace management
@@ -224,12 +232,15 @@ impl NvmeCtrl {
             },
             IDENT_CNS_CONTROLLER => {
                 assert!(size_of::<IdentifyController>() <= PAGE_SIZE);
-                let buf = cmd
-                    .data(mem)
-                    .next()
-                    .expect("missing prp entry for ident response");
-                assert!(mem.write(buf.0, &self.ctrl_ident));
-                cmds::Completion::success()
+
+                match Self::write_admin_result(
+                    cmd.data(mem),
+                    &self.ctrl_ident,
+                    mem,
+                ) {
+                    Some(_) => cmds::Completion::success(),
+                    None => cmds::Completion::generic_err(STS_DATA_XFER_ERR),
+                }
             }
             // We currently present NVMe version 1.0 in which CNS is a 1-bit field
             // and hence only need to support the NAMESPACE and CONTROLLER cases
@@ -282,4 +293,58 @@ impl NvmeCtrl {
             }
         }
     }
+
+    /// Write result data from an admin command into host memory
+    ///
+    /// The `data` type must be `repr(packed(1))`
+    ///
+    /// Returns `Some(())` if successful, else None
+    fn write_admin_result<T: Copy>(
+        prp: cmds::PrpIter,
+        data: &T,
+        mem: &MemCtx,
+    ) -> Option<()> {
+        let bufs: Vec<GuestRegion> = prp.collect();
+        if size_of::<T>() > bufs.iter().map(|r| r.1).sum::<usize>() {
+            // Not enough space
+            return None;
+        }
+        let regions = bufs
+            .into_iter()
+            .map(|r| mem.writable_region(&r))
+            .collect::<Option<Vec<_>>>()?;
+        if regions.len() == 1 {
+            // Can be copied to one contiguous page
+            regions[0].write(data).ok()?;
+            Some(())
+        } else {
+            // Split across multiple pages
+
+            // Safety:
+            //
+            // We expect and demand that the resulting structs written through
+            // this function are packed, such that there is no padding to risk
+            // UB through the [u8] slice creation.
+            let mut raw = unsafe {
+                std::slice::from_raw_parts(
+                    data as *const T as *const u8,
+                    size_of::<T>(),
+                )
+            };
+            let mut copied = 0;
+            for region in regions {
+                let write_len = usize::min(region.len(), raw.len());
+
+                let to_copy;
+                (to_copy, raw) = raw.split_at(write_len);
+                copied += region.write_bytes(&to_copy).ok()?;
+
+                if raw.is_empty() {
+                    break;
+                }
+            }
+            assert_eq!(copied, size_of::<T>());
+            Some(())
+        }
+    }
 }

lib/propolis/src/hw/nvme/cmds.rs

@@ -244,9 +244,13 @@ pub struct GetLogPageCmd {
 }
 
 impl GetLogPageCmd {
-    /// Returns an Iterator that yields [`GuestRegion`]'s to write the log page data to.
+    /// Returns an Iterator that yields [`GuestRegion`]'s to write the log page
+    /// data to.
+    ///
+    /// The expected size of the memory covered by the PRPs is defined by
+    /// `NUMD`, stored as bytes (rather than number Dwords) in [`Self::len`]
     pub fn data<'a>(&self, mem: &'a MemCtx) -> PrpIter<'a> {
-        PrpIter::new(PAGE_SIZE as u64, self.prp1, self.prp2, mem)
+        PrpIter::new(self.len as u64, self.prp1, self.prp2, mem)
     }
 }