diff --git a/bin/propolis-utils/src/bin/cpuid-gen.rs b/bin/propolis-utils/src/bin/cpuid-gen.rs index 0af6628a9..d5f59fc17 100644 --- a/bin/propolis-utils/src/bin/cpuid-gen.rs +++ b/bin/propolis-utils/src/bin/cpuid-gen.rs @@ -6,15 +6,15 @@ use clap::Parser; fn create_vm() -> anyhow::Result { let name = format!("cpuid-gen-{}", std::process::id()); - let mut req = bhyve_api::vm_create_req::new(&name); + let mut req = + bhyve_api::vm_create_req::new(name.as_bytes()).expect("valid VM name"); let ctl = VmmCtlFd::open()?; let _ = unsafe { ctl.ioctl(bhyve_api::VMM_CREATE_VM, &mut req) }?; let vm = VmmFd::open(&name).map_err(|e| { // Attempt to manually destroy the VM if we cannot open it - let mut destroy = bhyve_api::vm_destroy_req::new(&name); - let _ = unsafe { ctl.ioctl(bhyve_api::VMM_DESTROY_VM, &mut destroy) }; + let _ = ctl.vm_destroy(name.as_bytes()); e })?; diff --git a/crates/bhyve-api/src/lib.rs b/crates/bhyve-api/src/lib.rs index 510797e17..c1a29e03e 100644 --- a/crates/bhyve-api/src/lib.rs +++ b/crates/bhyve-api/src/lib.rs @@ -103,6 +103,13 @@ impl VmmCtlFd { Ok(req) } + /// Destroy VM instance + pub fn vm_destroy(&self, name: &[u8]) -> Result<()> { + let mut req = vm_destroy_req::new(name)?; + unsafe { self.ioctl(ioctls::VMM_DESTROY_VM, &mut req)? }; + Ok(()) + } + /// Check VMM ioctl command against those known to not require any /// copyin/copyout to function. const fn ioctl_usize_safe(cmd: i32) -> bool { diff --git a/crates/bhyve-api/sys/src/structs.rs b/crates/bhyve-api/sys/src/structs.rs index 27acd03bf..e81cd9601 100644 --- a/crates/bhyve-api/sys/src/structs.rs +++ b/crates/bhyve-api/sys/src/structs.rs @@ -1,3 +1,4 @@ +use std::io::{Error, ErrorKind, Result}; use std::os::raw::{c_int, c_uint, c_void}; use libc::size_t; @@ -430,11 +431,20 @@ pub struct vm_legacy_cpuid { pub const VM_MAX_NAMELEN: usize = 128; pub const VM_MAX_SEG_NAMELEN: usize = 128; -// Copy VM name, paying no heed to whether a trailing NUL is left in the -// destination byte slice. The kernel will do that error handling. -fn copy_name(field: &mut [u8], name: &str) { - let copy_len = name.len().min(field.len()); - field[..copy_len].copy_from_slice(name.as_bytes()); +/// Copy VM name into array appropriately sized for create/destroy request. +/// Advanced checks are left to the kernel logic consuming that value. +fn validate_name(value: &[u8]) -> Result<[u8; VM_MAX_NAMELEN]> { + let mut buf = [0u8; VM_MAX_NAMELEN]; + + if value.len() > buf.len() { + return Err(Error::new( + ErrorKind::InvalidInput, + "name length exceeds VM_MAX_NAMELEN", + )); + } + + buf[..(value.len())].copy_from_slice(value); + Ok(buf) } #[repr(C)] @@ -449,10 +459,8 @@ impl Default for vm_create_req { } } impl vm_create_req { - pub fn new(name: &str) -> Self { - let mut res = Self::default(); - copy_name(&mut res.name, name); - res + pub fn new(name: &[u8]) -> Result { + Ok(Self { name: validate_name(name)?, flags: 0 }) } } @@ -476,10 +484,8 @@ impl Default for vm_destroy_req { } } impl vm_destroy_req { - pub fn new(name: &str) -> Self { - let mut res = Self::default(); - copy_name(&mut res.name, name); - res + pub fn new(name: &[u8]) -> Result { + Ok(Self { name: validate_name(name)? }) } } diff --git a/lib/propolis/src/vmm/hdl.rs b/lib/propolis/src/vmm/hdl.rs index e21828808..df7ca17d0 100644 --- a/lib/propolis/src/vmm/hdl.rs +++ b/lib/propolis/src/vmm/hdl.rs @@ -44,7 +44,7 @@ pub struct CreateOpts { pub(crate) fn create_vm(name: &str, opts: CreateOpts) -> Result { let ctl = bhyve_api::VmmCtlFd::open()?; - let mut req = bhyve_api::vm_create_req::new(name); + let mut req = bhyve_api::vm_create_req::new(name.as_bytes())?; if opts.use_reservoir { req.flags |= bhyve_api::VCF_RESERVOIR_MEM; } @@ -58,12 +58,10 @@ pub(crate) fn create_vm(name: &str, opts: CreateOpts) -> Result { } // try to nuke(!) the existing vm - let mut dreq = bhyve_api::vm_destroy_req::new(name); - let _ = unsafe { ctl.ioctl(bhyve_api::VMM_DESTROY_VM, &mut dreq) } - .or_else(|e| match e.kind() { - ErrorKind::NotFound => Ok(0), - _ => Err(e), - })?; + ctl.vm_destroy(name.as_bytes()).or_else(|e| match e.kind() { + ErrorKind::NotFound => Ok(()), + _ => Err(e), + })?; // now attempt to create in its presumed absence let _ = unsafe { ctl.ioctl(bhyve_api::VMM_CREATE_VM, &mut req) }?; @@ -82,18 +80,6 @@ pub(crate) fn create_vm(name: &str, opts: CreateOpts) -> Result { }) } -/// Destroys the virtual machine matching the provided `name`. -fn destroy_vm_impl(name: &str) -> Result<()> { - let ctl = bhyve_api::VmmCtlFd::open()?; - let mut dreq = bhyve_api::vm_destroy_req::new(name); - let _ = unsafe { ctl.ioctl(bhyve_api::VMM_DESTROY_VM, &mut dreq) } - .or_else(|e| match e.kind() { - ErrorKind::NotFound => Ok(0), - _ => Err(e), - })?; - Ok(()) -} - /// A wrapper around a file which must uphold the guarantee that the underlying /// structure may not be truncated. pub struct VmmFile(File); @@ -403,7 +389,11 @@ impl VmmHdl { // If that failed (which may occur on older platforms without // self-destruction), then fall back to performing the destroy through // the vmmctl device. - destroy_vm_impl(&self.name) + let ctl = bhyve_api::VmmCtlFd::open()?; + ctl.vm_destroy(self.name.as_bytes()).or_else(|e| match e.kind() { + ErrorKind::NotFound => Ok(()), + _ => Err(e), + }) } /// Set whether instance should auto-destruct when closed