Fix cargo feature 'crashdump'

ludfjig · ludfjig · commit 967003a75025 · 2024-11-21T19:35:25.000Z
diff --git a/.github/workflows/dep_rust.yml b/.github/workflows/dep_rust.yml
@@ -131,6 +131,9 @@ jobs:
           # with only one driver enabled (driver mshv/kvm feature is ignored on windows) + seccomp + inprocess
           just test-rust ${{ matrix.config }} inprocess,seccomp,${{ matrix.hypervisor == 'mshv' && 'mshv' || 'kvm' }} 
 
+          # make sure certain cargo features compile
+          cargo check -p hyperlight-host --features dump_on_crash
+
           # without any driver (shouldn't compile)
           just test-rust-feature-compilation-fail ${{ matrix.config }}
 
diff --git a/src/hyperlight_host/Cargo.toml b/src/hyperlight_host/Cargo.toml
@@ -112,15 +112,15 @@ cfg_aliases = "0.2.1"
 built = { version = "0.7.0", features = ["chrono", "git2"] }
 
 [features]
-default = ["kvm", "mshv", "seccomp"]
+default = ["kvm", "mshv", "seccomp", "crashdump"]
 seccomp = ["dep:seccompiler"]
 function_call_metrics = []
 executable_heap = []
 # This feature enables printing of debug information to stdout in debug builds
 print_debug = []
 # This feature enables dunping of the VMs details to a file when an unexpected or error exit occurs in a VM in debug mode
 # the name of the file is output to stdout and logged.
-dump_on_crash = ["dep:tempfile"]
+crashdump = ["dep:tempfile"]
 kvm = ["dep:kvm-bindings", "dep:kvm-ioctls"]
 mshv = ["dep:mshv-bindings", "dep:mshv-ioctls"]
 inprocess = []
diff --git a/src/hyperlight_host/build.rs b/src/hyperlight_host/build.rs
@@ -94,6 +94,8 @@ fn main() -> Result<()> {
         // inprocess feature is aliased with debug_assertions to make it only available in debug-builds.
         // You should never use #[cfg(feature = "inprocess")] in the codebase. Use #[cfg(inprocess)] instead.
         inprocess: { all(feature = "inprocess", debug_assertions) },
+        // crashdump feature is aliased with debug_assertions to make it only available in debug-builds.
+        crashdump: { all(feature = "crashdump", debug_assertions) },
     }
 
     write_built_file()?;
diff --git a/src/hyperlight_host/src/hypervisor/crashdump.rs b/src/hyperlight_host/src/hypervisor/crashdump.rs
@@ -0,0 +1,47 @@
+use std::io::Write;
+
+use tempfile::NamedTempFile;
+
+use super::Hypervisor;
+use crate::{new_error, Result};
+
+/// Dump registers + memory regions + raw memory to a tempfile
+#[cfg(crashdump)]
+pub(crate) fn crashdump_to_tempfile(hv: &dyn Hypervisor) -> Result<()> {
+    let mem_regions = hv.get_memory_regions();
+    let mem_size = mem_regions
+        .iter()
+        .map(|region| region.host_region.len())
+        .sum();
+    let mem_start_addr = mem_regions[0].host_region.start as *const u8;
+
+    if mem_start_addr.is_null() || mem_size == 0 {
+        return Err(new_error!(
+            "Invalid address or size while creating crashdump"
+        ));
+    }
+
+    let mut temp_file = NamedTempFile::with_prefix("mem")?;
+
+    let hv_details = format!("{:#x?}", hv);
+
+    // write hypervisor details such as registers, memory regions, etc.
+    temp_file.write_all(hv_details.as_bytes())?;
+    // write memory dump
+    temp_file.write_all(b"================ MEMORY DUMP =================\n")?;
+    // SAFETY: Address and size non-null and non-zero
+    unsafe {
+        let slice = std::slice::from_raw_parts(mem_start_addr, mem_size);
+        temp_file.write_all(slice)?;
+        temp_file.flush()?;
+    }
+    let persist_path = temp_file.path().with_extension("dmp");
+    temp_file
+        .persist(&persist_path)
+        .map_err(|e| new_error!("Failed to persist crashdump file: {:?}", e))?;
+
+    println!("Memory dumped to file: {:?}", persist_path);
+    log::error!("Memory dumped to file: {:?}", persist_path);
+
+    Ok(())
+}
diff --git a/src/hyperlight_host/src/hypervisor/hyperv_linux.rs b/src/hyperlight_host/src/hypervisor/hyperv_linux.rs
@@ -311,8 +311,6 @@ impl Hypervisor for HypervLinuxDriver {
                         "mshv MMIO unmapped GPA -Details: Address: {} \n {:#?}",
                         addr, &self
                     );
-                    #[cfg(all(debug_assertions, feature = "dump_on_crash"))]
-                    self.dump_on_crash(self.mem_regions.clone());
                     HyperlightExit::Mmio(addr)
                 }
                 INVALID_GPA_ACCESS_MESSAGE => {
@@ -323,8 +321,6 @@ impl Hypervisor for HypervLinuxDriver {
                         "mshv MMIO invalid GPA access -Details: Address: {} \n {:#?}",
                         gpa, &self
                     );
-                    #[cfg(all(debug_assertions, feature = "dump_on_crash"))]
-                    self.dump_on_crash(self.mem_regions.clone());
                     match self.get_memory_access_violation(
                         gpa as usize,
                         &self.mem_regions,
@@ -336,8 +332,6 @@ impl Hypervisor for HypervLinuxDriver {
                 }
                 other => {
                     debug!("mshv Other Exit: Exit: {:#?} \n {:#?}", other, &self);
-                    #[cfg(all(debug_assertions, feature = "dump_on_crash"))]
-                    self.dump_on_crash(self.mem_regions.clone());
                     log_then_return!("unknown Hyper-V run message type {:?}", other);
                 }
             },
@@ -347,8 +341,6 @@ impl Hypervisor for HypervLinuxDriver {
                 libc::EAGAIN => HyperlightExit::Retry(),
                 _ => {
                     debug!("mshv Error - Details: Error: {} \n {:#?}", e, &self);
-                    #[cfg(all(debug_assertions, feature = "dump_on_crash"))]
-                    self.dump_on_crash(self.mem_regions.clone());
                     log_then_return!("Error running VCPU {:?}", e);
                 }
             },
@@ -360,6 +352,11 @@ impl Hypervisor for HypervLinuxDriver {
     fn as_mut_hypervisor(&mut self) -> &mut dyn Hypervisor {
         self as &mut dyn Hypervisor
     }
+
+    #[cfg(crashdump)]
+    fn get_memory_regions(&self) -> &[MemoryRegion] {
+        &self.mem_regions
+    }
 }
 
 impl Drop for HypervLinuxDriver {
diff --git a/src/hyperlight_host/src/hypervisor/inprocess.rs b/src/hyperlight_host/src/hypervisor/inprocess.rs
@@ -18,6 +18,8 @@ use std::fmt::Debug;
 use std::os::raw::c_void;
 
 use super::{HyperlightExit, Hypervisor};
+#[cfg(crashdump)]
+use crate::mem::memory_region::MemoryRegion;
 use crate::sandbox::leaked_outb::LeakedOutBWrapper;
 use crate::Result;
 
@@ -123,4 +125,9 @@ impl<'a> Hypervisor for InprocessDriver<'a> {
     fn get_partition_handle(&self) -> windows::Win32::System::Hypervisor::WHV_PARTITION_HANDLE {
         unimplemented!("get_partition_handle should not be needed since we are in in-process mode")
     }
+
+    #[cfg(crashdump)]
+    fn get_memory_regions(&self) -> &[MemoryRegion] {
+        unimplemented!("get_memory_regions is not supported since we are in in-process mode")
+    }
 }
diff --git a/src/hyperlight_host/src/hypervisor/kvm.rs b/src/hyperlight_host/src/hypervisor/kvm.rs
@@ -17,7 +17,6 @@ limitations under the License.
 use std::convert::TryFrom;
 use std::fmt::Debug;
 
-use cfg_if::cfg_if;
 use kvm_bindings::{kvm_fpu, kvm_regs, kvm_userspace_memory_region, KVM_MEM_READONLY};
 use kvm_ioctls::Cap::UserMemory;
 use kvm_ioctls::{Kvm, VcpuExit, VcpuFd, VmFd};
@@ -283,11 +282,9 @@ impl Hypervisor for KVMDriver {
             }
             Ok(VcpuExit::MmioRead(addr, _)) => {
                 debug!("KVM MMIO Read -Details: Address: {} \n {:#?}", addr, &self);
-                #[cfg(all(debug_assertions, feature = "dump_on_crash"))]
-                self.dump_on_crash(self.mem_regions.clone());
-                let gpa = addr as usize;
+
                 match self.get_memory_access_violation(
-                    gpa,
+                    addr as usize,
                     &self.mem_regions,
                     MemoryRegionFlags::READ,
                 ) {
@@ -297,11 +294,9 @@ impl Hypervisor for KVMDriver {
             }
             Ok(VcpuExit::MmioWrite(addr, _)) => {
                 debug!("KVM MMIO Write -Details: Address: {} \n {:#?}", addr, &self);
-                #[cfg(all(debug_assertions, feature = "dump_on_crash"))]
-                self.dump_on_crash(self.mem_regions.clone());
-                let gpa = addr as usize;
+
                 match self.get_memory_access_violation(
-                    gpa,
+                    addr as usize,
                     &self.mem_regions,
                     MemoryRegionFlags::WRITE,
                 ) {
@@ -315,25 +310,12 @@ impl Hypervisor for KVMDriver {
                 libc::EAGAIN => HyperlightExit::Retry(),
                 _ => {
                     debug!("KVM Error -Details: Address: {} \n {:#?}", e, &self);
-                    #[cfg(all(debug_assertions, feature = "dump_on_crash"))]
-                    self.dump_on_crash(self.mem_regions.clone());
                     log_then_return!("Error running VCPU {:?}", e);
                 }
             },
             Ok(other) => {
-                cfg_if! {
-                    if #[cfg(all(feature = "print_debug", debug_assertions))] {
-                        let _ = other;
-                        debug!("KVM Other Exit: \n {:#?}", &self);
-                        HyperlightExit::Unknown("Unexpected KVM Exit".to_string())
-                    } else if #[cfg(all(feature = "dump_on_crash", debug_assertions))] {
-                            self.dump_on_crash(self.mem_regions.clone());
-                            HyperlightExit::Unknown(format!("Unexpected KVM Exit {:?}", other))
-                    } else{
-                        debug!("KVM Other Exit {:?}", other);
-                        HyperlightExit::Unknown(format!("Unexpected KVM Exit {:?}", other))
-                    }
-                }
+                debug!("KVM Other Exit {:?}", other);
+                HyperlightExit::Unknown(format!("Unexpected KVM Exit {:?}", other))
             }
         };
         Ok(result)
@@ -343,6 +325,11 @@ impl Hypervisor for KVMDriver {
     fn as_mut_hypervisor(&mut self) -> &mut dyn Hypervisor {
         self as &mut dyn Hypervisor
     }
+
+    #[cfg(crashdump)]
+    fn get_memory_regions(&self) -> &[MemoryRegion] {
+        &self.mem_regions
+    }
 }
 
 #[cfg(test)]
diff --git a/src/hyperlight_host/src/hypervisor/mod.rs b/src/hyperlight_host/src/hypervisor/mod.rs
@@ -55,6 +55,9 @@ pub(crate) mod windows_hypervisor_platform;
 #[cfg(target_os = "windows")]
 pub(crate) mod wrappers;
 
+#[cfg(crashdump)]
+pub(crate) mod crashdump;
+
 use std::fmt::Debug;
 use std::sync::{Arc, Mutex};
 
@@ -184,70 +187,8 @@ pub(crate) trait Hypervisor: Debug + Sync + Send {
     #[cfg(target_os = "windows")]
     fn get_partition_handle(&self) -> windows::Win32::System::Hypervisor::WHV_PARTITION_HANDLE;
 
-    /// Dump memory to a file on crash
-    #[cfg(all(debug_assertions, feature = "dump_on_crash", target_os = "linux"))]
-    fn dump_on_crash(&self, mem_regions: Vec<MemoryRegion>) {
-        let memory_size = mem_regions
-            .iter()
-            .map(|region| region.guest_region.end - region.guest_region.start)
-            .sum();
-        if let Err(e) = unsafe {
-            self.write_dump_file(
-                mem_regions.clone(),
-                mem_regions[0].host_region.start as *const u8,
-                memory_size,
-            )
-        } {
-            println!("Error dumping memory: {}", e);
-        }
-    }
-
-    /// A function that takes an address and a size and writes the memory at that address to a file in the temp/tmp directory
-    ///  # Safety
-    /// This function is unsafe because it is writing memory to a file, make sure that the address is valid and that the size is correct
-    /// This function is only available when the `dump_on_crash` feature is enabled and running in debug mode
-    #[cfg(all(feature = "dump_on_crash", debug_assertions))]
-    unsafe fn write_dump_file(
-        &self,
-        regions: Vec<MemoryRegion>,
-        address: *const u8,
-        size: usize,
-    ) -> Result<()> {
-        use std::io::Write;
-
-        use tempfile::NamedTempFile;
-
-        if address.is_null() || size == 0 {
-            return Err(new_error!("Invalid address or size"));
-        }
-
-        let hv_details = format!("{:#?}", self);
-        let regions_details = format!("{:#?}", regions);
-
-        // Create a temporary file
-        let mut file = NamedTempFile::with_prefix("mem")?;
-        let temp_path = file.path().to_path_buf();
-
-        file.write_all(hv_details.as_bytes())?;
-        file.write_all(b"\n")?;
-        file.write_all(regions_details.as_bytes())?;
-        file.write_all(b"\n")?;
-
-        // SAFETY: Ensure the address and size are valid and accessible
-        unsafe {
-            let slice = std::slice::from_raw_parts(address, size);
-            file.write_all(slice)?;
-            file.flush()?;
-        }
-        let persist_path = temp_path.with_extension("dmp");
-        file.persist(&persist_path)
-            .map_err(|e| new_error!("Failed to persist file: {:?}", e))?;
-
-        print!("Memory dumped to file: {:?}", persist_path);
-        log::error!("Memory dumped to file: {:?}", persist_path);
-
-        Ok(())
-    }
+    #[cfg(crashdump)]
+    fn get_memory_regions(&self) -> &[MemoryRegion];
 }
 
 /// A virtual CPU that can be run until an exit occurs
@@ -271,11 +212,15 @@ impl VirtualCPU {
                     hv.handle_io(port, data, rip, instruction_length, outb_handle_fn.clone())?
                 }
                 HyperlightExit::Mmio(addr) => {
+                    #[cfg(crashdump)]
+                    crashdump::crashdump_to_tempfile(hv)?;
+
                     mem_access_fn
                         .clone()
                         .try_lock()
                         .map_err(|e| new_error!("Error locking at {}:{}: {}", file!(), line!(), e))?
                         .call()?;
+
                     log_then_return!("MMIO access address {:#x}", addr);
                 }
                 HyperlightExit::AccessViolation(addr, tried, region_permission) => {
diff --git a/src/hyperlight_host/tests/integration_test.rs b/src/hyperlight_host/tests/integration_test.rs
@@ -493,6 +493,17 @@ fn recursive_stack_allocate_overflow() {
     assert!(matches!(res, HyperlightError::StackOverflow()));
 }
 
+#[test]
+fn read_invalid_address() {
+    let sbox1: SingleUseSandbox = new_uninit().unwrap().evolve(Noop::default()).unwrap();
+
+    let res = sbox1
+        .call_guest_function_by_name("MmioRead", ReturnType::Int, None)
+        .unwrap_err();
+
+    assert!(matches!(res, HyperlightError::Error(msg) if msg.starts_with("MMIO access address")));
+}
+
 // Check that log messages are emitted correctly from the guest
 // This test is ignored as it sets a logger and therefore maybe impacted by other tests running concurrently
 // or it may impact other tests.
diff --git a/src/tests/rust_guests/simpleguest/src/main.rs b/src/tests/rust_guests/simpleguest/src/main.rs
@@ -724,6 +724,14 @@ fn add(function_call: &FunctionCall) -> Result<Vec<u8>> {
     }
 }
 
+// Used to test the behavior of the sandbox when reading from an invalid address.
+// Currently, the vm will exit with an MMIO exit
+fn mmio_read(_: &FunctionCall) -> Result<Vec<u8>> {
+    let ptr: *const i32 = core::ptr::null();
+    let val = unsafe { *(ptr.add(5000000)) };
+    Ok(get_flatbuffer_result_from_int(val))
+}
+
 #[no_mangle]
 pub extern "C" fn hyperlight_main() {
     let set_static_def = GuestFunctionDefinition::new(
@@ -1105,6 +1113,14 @@ pub extern "C" fn hyperlight_main() {
         add as i64,
     );
     register_function(add_def);
+
+    let mmio_read_def = GuestFunctionDefinition::new(
+        "MmioRead".to_string(),
+        Vec::new(),
+        ReturnType::Int,
+        mmio_read as i64,
+    );
+    register_function(mmio_read_def);
 }
 
 #[no_mangle]

Original file line number	Diff line number	Diff line change
`@@ -94,6 +94,8 @@ fn main() -> Result<()> {`
`94`	`94`	`// inprocess feature is aliased with debug_assertions to make it only available in debug-builds.`
`95`	`95`	`// You should never use #[cfg(feature = "inprocess")] in the codebase. Use #[cfg(inprocess)] instead.`
`96`	`96`	`inprocess: { all(feature = "inprocess", debug_assertions) },`
	`97`	`+ // crashdump feature is aliased with debug_assertions to make it only available in debug-builds.`
	`98`	`+ crashdump: { all(feature = "crashdump", debug_assertions) },`
`97`	`99`	`}`
`98`	`100`
`99`	`101`	`write_built_file()?;`