PR feedback

Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>

Author: ludfjig

.github/workflows/ValidatePullRequest.yml

@@ -67,7 +67,7 @@ jobs:
       - docs-pr
       - build-guests
     strategy:
-      fail-fast: false
+      fail-fast: true
       matrix:
         hypervisor: [hyperv, 'hyperv-ws2025', mshv3, kvm]
         cpu: [amd, intel]

src/hyperlight_host/src/error.rs

@@ -327,9 +327,8 @@ impl HyperlightError {
             | HyperlightError::MemoryAccessViolation(_, _, _)
             | HyperlightError::SnapshotSizeMismatch(_, _)
             | HyperlightError::MemoryRegionSizeMismatch(_, _, _)
-            // HyperlightVmError::Restore doesn't technically need to poison the sandbox,
-            // because if MultiUseSandbox::restore() fails, the sandbox is still in the same
-            // poisoned state as before.
+            // HyperlightVmError::Restore is already handled manually in restore(), but we mark it
+            // as poisoning here too for defense in depth.
             | HyperlightError::HyperlightVmError(HyperlightVmError::Restore(_)) => true,
 
             // HyperlightVmError::DispatchGuestCall may poison the sandbox

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

@@ -875,11 +875,11 @@ impl HyperlightVm {
         Ok(())
     }
 
-    // Resets the following vCPU state:
-    // - General purpose registers
-    // - Debug registers
-    // - XSAVE (includes FPU/SSE state with proper FCW and MXCSR defaults)
-    // - Special registers (with saved PML4 if feature enabled)
+    /// Resets the following vCPU state:
+    /// - General purpose registers
+    /// - Debug registers
+    /// - XSAVE (includes FPU/SSE state with proper FCW and MXCSR defaults)
+    /// - Special registers (with saved PML4 if feature enabled)
     // TODO: check if other state needs to be reset
     pub(crate) fn reset_vcpu(&self) -> std::result::Result<(), RegisterError> {
         self.vm.set_regs(&CommonRegisters {
@@ -2344,7 +2344,8 @@ mod tests {
             normalize_fpu_mxcsr_for_kvm(&mut expected_dirty, fpu.mxcsr);
             assert_eq!(fpu, expected_dirty);
 
-            // Verify MXCSR via xsave on KVM (since fpu() doesn't return it)
+            // KVM's get_fpu/set_fpu ioctls don't include MXCSR (it's in the SSE state,
+            // not x87 FPU state). We must use xsave to verify MXCSR on KVM.
             #[cfg(kvm)]
             if available_hv == HypervisorType::Kvm {
                 let xsave = hyperlight_vm.vm.xsave().unwrap();
@@ -2358,7 +2359,7 @@ mod tests {
             // Check FPU is reset to defaults
             assert_fpu_reset(hyperlight_vm.vm.as_ref());
 
-            // Verify MXCSR via xsave on KVM
+            // Verify MXCSR via xsave on KVM (fpu() doesn't include it)
             #[cfg(kvm)]
             if available_hv == HypervisorType::Kvm {
                 let xsave = hyperlight_vm.vm.xsave().unwrap();

src/hyperlight_host/src/hypervisor/virtual_machine/kvm.rs

@@ -255,16 +255,21 @@ impl VirtualMachine for KvmVm {
     fn reset_xsave(&self) -> std::result::Result<(), RegisterError> {
         let mut xsave = kvm_xsave::default(); // default is zeroed 4KB buffer with no FAM
 
-        // XSAVE area layout from Intel SDM Vol. 1 Section 13.4.1:
-        // - Bytes 0-1: FCW (x87 FPU Control Word)
+        // XSAVE legacy region layout (Intel SDM Vol. 1 Section 13.4.1):
+        // - Bytes 0-1: FCW, 2-3: FSW
         // - Bytes 24-27: MXCSR
-        // - Bytes 512-519: XSTATE_BV (bitmap of valid state components)
+        // - Bytes 512-519: XSTATE_BV
+        // - Bytes 520-527: XCOMP_BV (compaction format indicator)
+        //
+        // kvm_xsave.region is [u32], so region[0] covers FCW (low 16) and FSW (high 16, stays 0).
         xsave.region[0] = FP_CONTROL_WORD_DEFAULT as u32;
         xsave.region[6] = MXCSR_DEFAULT;
         // XSTATE_BV = 0x3: bits 0,1 = x87 + SSE valid. This tells KVM to apply
         // the legacy region from this buffer. Without this, some KVM versions
         // may ignore set_xsave entirely when XSTATE_BV=0.
         xsave.region[128] = 0x3;
+        // Note: Unlike MSHV/WHP, we don't preserve XCOMP_BV because KVM uses
+        // standard (non-compacted) XSAVE format where XCOMP_BV remains 0.
 
         // SAFETY: No dynamic features enabled, 4KB is sufficient
         unsafe {

src/hyperlight_host/src/hypervisor/virtual_machine/mod.rs

@@ -100,6 +100,12 @@ pub(crate) enum HypervisorType {
     Whp,
 }
 
+/// Minimum XSAVE buffer size: 512 bytes legacy region + 64 bytes header.
+/// Only used by MSHV and WHP which use compacted XSAVE format and need to
+/// validate buffer size before accessing XCOMP_BV.
+#[cfg(any(mshv3, target_os = "windows"))]
+pub(crate) const XSAVE_MIN_SIZE: usize = 576;
+
 // Compiler error if no hypervisor type is available
 #[cfg(not(any(kvm, mshv3, target_os = "windows")))]
 compile_error!(

src/hyperlight_host/src/hypervisor/virtual_machine/mshv.rs

@@ -41,7 +41,7 @@ use crate::hypervisor::regs::{
 };
 use crate::hypervisor::virtual_machine::{
     CreateVmError, MapMemoryError, RegisterError, RunVcpuError, UnmapMemoryError, VirtualMachine,
-    VmExit,
+    VmExit, XSAVE_MIN_SIZE,
 };
 use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags};
 #[cfg(feature = "trace_guest")]
@@ -293,8 +293,6 @@ impl VirtualMachine for MshvVm {
     }
 
     fn reset_xsave(&self) -> std::result::Result<(), RegisterError> {
-        const XSAVE_MIN_SIZE: usize = 576; // 512 legacy + 64 header
-
         let current_xsave = self
             .vcpu_fd
             .get_xsave()

src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs

@@ -37,7 +37,7 @@ use crate::hypervisor::surrogate_process::SurrogateProcess;
 use crate::hypervisor::surrogate_process_manager::get_surrogate_process_manager;
 use crate::hypervisor::virtual_machine::{
     CreateVmError, HypervisorError, MapMemoryError, RegisterError, RunVcpuError, UnmapMemoryError,
-    VirtualMachine, VmExit,
+    VirtualMachine, VmExit, XSAVE_MIN_SIZE,
 };
 use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags};
 #[cfg(feature = "trace_guest")]
@@ -506,7 +506,6 @@ impl VirtualMachine for WhpVm {
     }
 
     fn reset_xsave(&self) -> std::result::Result<(), RegisterError> {
-        const XSAVE_MIN_SIZE: usize = 576; // 512 legacy + 64 header
         // WHP uses compacted XSAVE format (bit 63 of XCOMP_BV set).
         // We cannot just zero out the xsave area, we need to preserve the XCOMP_BV.
 
@@ -611,7 +610,7 @@ impl VirtualMachine for WhpVm {
         }
 
         let provided_size = std::mem::size_of_val(xsave) as u32;
-        if buffer_size_needed > provided_size {
+        if provided_size != buffer_size_needed {
             return Err(RegisterError::XsaveSizeMismatch {
                 expected: buffer_size_needed,
                 actual: provided_size,

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

@@ -197,6 +197,12 @@ impl MultiUseSandbox {
     /// - **Locked mutexes** - All lock state is reset
     /// - **Partial updates** - Data structures restored to their pre-execution state
     ///
+    /// # Errors
+    ///
+    /// If this method returns an error, the sandbox will be poisoned regardless of its
+    /// previous state. This is because a failed restore may leave the sandbox in an
+    /// inconsistent state (e.g., memory partially restored but vCPU state not reset).
+    /// The sandbox should not be used after a failed restore.
     ///
     /// # Examples
     ///
@@ -289,8 +295,13 @@ impl MultiUseSandbox {
             unsafe { self.vm.map_region(region) }.map_err(HyperlightVmError::MapRegion)?;
         }
 
-        // if this fails, then the sandbox will not have been restored so keep it poisoned (if it is)
-        self.vm.reset_vcpu().map_err(HyperlightVmError::Restore)?;
+        // Reset vCPU state (registers, FPU, debug registers, etc.)
+        // If this fails, we're in an inconsistent state (memory restored but vCPU not reset),
+        // so we must poison the sandbox.
+        self.vm.reset_vcpu().map_err(|e| {
+            self.poisoned = true;
+            HyperlightVmError::Restore(e)
+        })?;
 
         // The restored snapshot is now our most current snapshot
         self.snapshot = Some(snapshot.clone());