From fbd701c0a54f25208712b0b3b2dc7931c875d347 Mon Sep 17 00:00:00 2001 From: Patrick Mooney Date: Fri, 14 Jul 2023 21:40:14 -0500 Subject: [PATCH] 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 --- lib/propolis/src/hw/nvme/admin.rs | 107 ++++++++++++++++++++++++------ lib/propolis/src/hw/nvme/cmds.rs | 8 ++- 2 files changed, 92 insertions(+), 23 deletions(-) diff --git a/lib/propolis/src/hw/nvme/admin.rs b/lib/propolis/src/hw/nvme/admin.rs index e84cad9be..0dad4492a 100644 --- a/lib/propolis/src/hw/nvme/admin.rs +++ b/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::>>() + { + // 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::() <= 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::() <= 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( + prp: cmds::PrpIter, + data: &T, + mem: &MemCtx, + ) -> Option<()> { + let bufs: Vec = prp.collect(); + if size_of::() > bufs.iter().map(|r| r.1).sum::() { + // Not enough space + return None; + } + let regions = bufs + .into_iter() + .map(|r| mem.writable_region(&r)) + .collect::>>()?; + 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::(), + ) + }; + 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::()); + Some(()) + } + } } diff --git a/lib/propolis/src/hw/nvme/cmds.rs b/lib/propolis/src/hw/nvme/cmds.rs index ce7c0c534..53fa9ea4f 100644 --- a/lib/propolis/src/hw/nvme/cmds.rs +++ b/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) } }