diff --git a/Justfile b/Justfile index 9ae92795f..1a066e2da 100644 --- a/Justfile +++ b/Justfile @@ -43,10 +43,10 @@ build target=default-target: guests: build-and-move-rust-guests build-and-move-c-guests ensure-cargo-hyperlight: - command -v cargo-hyperlight >/dev/null 2>&1 || cargo install --locked cargo-hyperlight + {{ if os() == "windows" { "if (-not (Get-Command cargo-hyperlight -ErrorAction SilentlyContinue)) { cargo install --locked cargo-hyperlight }" } else { "command -v cargo-hyperlight >/dev/null 2>&1 || cargo install --locked cargo-hyperlight" } }} witguest-wit: - command -v wasm-tools >/dev/null 2>&1 || cargo install --locked wasm-tools + {{ if os() == "windows" { "if (-not (Get-Command wasm-tools -ErrorAction SilentlyContinue)) { cargo install --locked wasm-tools }" } else { "command -v wasm-tools >/dev/null 2>&1 || cargo install --locked wasm-tools" } }} cd src/tests/rust_guests/witguest && wasm-tools component wit guest.wit -w -o interface.wasm cd src/tests/rust_guests/witguest && wasm-tools component wit two_worlds.wit -w -o twoworlds.wasm @@ -287,19 +287,21 @@ check: {{ cargo-cmd }} check -p hyperlight-host --features nanvix-unstable {{ target-triple-flag }} {{ cargo-cmd }} check -p hyperlight-host --features nanvix-unstable,executable_heap {{ target-triple-flag }} -fmt-check: - rustup +nightly component list | grep -q "rustfmt.*installed" || rustup component add rustfmt --toolchain nightly +fmt-check: (ensure-nightly-fmt) cargo +nightly fmt --all -- --check cargo +nightly fmt --manifest-path src/tests/rust_guests/simpleguest/Cargo.toml -- --check cargo +nightly fmt --manifest-path src/tests/rust_guests/dummyguest/Cargo.toml -- --check cargo +nightly fmt --manifest-path src/tests/rust_guests/witguest/Cargo.toml -- --check cargo +nightly fmt --manifest-path src/hyperlight_guest_capi/Cargo.toml -- --check +[private] +ensure-nightly-fmt: + {{ if os() == "windows" { "if (-not (rustup +nightly component list | Select-String 'rustfmt.*installed')) { rustup component add rustfmt --toolchain nightly }" } else { "rustup +nightly component list | grep -q 'rustfmt.*installed' || rustup component add rustfmt --toolchain nightly" } }} + check-license-headers: ./dev/check-license-headers.sh -fmt-apply: - rustup +nightly component list | grep -q "rustfmt.*installed" || rustup component add rustfmt --toolchain nightly +fmt-apply: (ensure-nightly-fmt) cargo +nightly fmt --all cargo +nightly fmt --manifest-path src/tests/rust_guests/simpleguest/Cargo.toml cargo +nightly fmt --manifest-path src/tests/rust_guests/dummyguest/Cargo.toml diff --git a/src/hyperlight_host/src/hypervisor/surrogate_process.rs b/src/hyperlight_host/src/hypervisor/surrogate_process.rs index f054dc996..6c439dbf6 100644 --- a/src/hyperlight_host/src/hypervisor/surrogate_process.rs +++ b/src/hyperlight_host/src/hypervisor/surrogate_process.rs @@ -23,19 +23,23 @@ use tracing::{Span, instrument}; use windows::Win32::Foundation::HANDLE; use windows::Win32::System::Memory::{ MEMORY_MAPPED_VIEW_ADDRESS, MapViewOfFileNuma2, PAGE_NOACCESS, PAGE_PROTECTION_FLAGS, - PAGE_READWRITE, UNMAP_VIEW_OF_FILE_FLAGS, UnmapViewOfFile2, VirtualProtectEx, + PAGE_READONLY, PAGE_READWRITE, UNMAP_VIEW_OF_FILE_FLAGS, UnmapViewOfFile2, VirtualProtectEx, }; use windows::Win32::System::SystemServices::NUMA_NO_PREFERRED_NODE; use super::surrogate_process_manager::get_surrogate_process_manager; use super::wrappers::HandleWrapper; use crate::HyperlightError::WindowsAPIError; +use crate::mem::memory_region::SurrogateMapping; use crate::{Result, log_then_return}; #[derive(Debug)] pub(crate) struct HandleMapping { pub(crate) use_count: u64, pub(crate) surrogate_base: *mut c_void, + /// The mapping type used when this entry was first created. + /// Used for debug assertions to catch conflicting re-maps. + pub(crate) mapping_type: SurrogateMapping, } /// Contains details of a surrogate process to be used by a Sandbox for providing memory to a HyperV VM on Windows. @@ -57,18 +61,45 @@ impl SurrogateProcess { } } + /// Maps a file mapping handle into the surrogate process. + /// + /// The `mapping` parameter controls the page protection and guard page + /// behaviour: + /// - [`SurrogateMapping::SandboxMemory`]: uses `PAGE_READWRITE` and sets + /// guard pages (`PAGE_NOACCESS`) on the first and last pages. + /// - [`SurrogateMapping::ReadOnlyFile`]: uses `PAGE_READONLY` with no + /// guard pages. + /// + /// If `host_base` was already mapped, the existing mapping is reused + /// and the reference count is incremented (the `mapping` parameter is + /// ignored in that case). pub(super) fn map( &mut self, handle: HandleWrapper, host_base: usize, host_size: usize, + mapping: &SurrogateMapping, ) -> Result<*mut c_void> { match self.mappings.entry(host_base) { Entry::Occupied(mut oe) => { + if oe.get().mapping_type != *mapping { + tracing::warn!( + "Conflicting SurrogateMapping for host_base {host_base:#x}: \ + existing={:?}, requested={:?}", + oe.get().mapping_type, + mapping + ); + } oe.get_mut().use_count += 1; Ok(oe.get().surrogate_base) } Entry::Vacant(ve) => { + // Derive the page protection from the mapping type + let page_protection = match mapping { + SurrogateMapping::SandboxMemory => PAGE_READWRITE, + SurrogateMapping::ReadOnlyFile => PAGE_READONLY, + }; + // Use MapViewOfFile2 to map memory into the surrogate process, the MapViewOfFile2 API is implemented in as an inline function in a windows header file // (see https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-mapviewoffile2#remarks) so we use the same API it uses in the header file here instead of // MapViewOfFile2 which does not exist in the rust crate (see https://github.com/microsoft/windows-rs/issues/2595) @@ -80,43 +111,60 @@ impl SurrogateProcess { None, host_size, 0, - PAGE_READWRITE.0, + page_protection.0, NUMA_NO_PREFERRED_NODE, ) }; - let mut unused_out_old_prot_flags = PAGE_PROTECTION_FLAGS(0); - // the first page of the raw_size is the guard page - let first_guard_page_start = surrogate_base.Value; - if let Err(e) = unsafe { - VirtualProtectEx( - self.process_handle.into(), - first_guard_page_start, - PAGE_SIZE_USIZE, - PAGE_NOACCESS, - &mut unused_out_old_prot_flags, - ) - } { - log_then_return!(WindowsAPIError(e.clone())); + if surrogate_base.Value.is_null() { + log_then_return!( + "MapViewOfFileNuma2 failed: {:?}", + std::io::Error::last_os_error() + ); } - // the last page of the raw_size is the guard page - let last_guard_page_start = - unsafe { first_guard_page_start.add(host_size - PAGE_SIZE_USIZE) }; - if let Err(e) = unsafe { - VirtualProtectEx( - self.process_handle.into(), - last_guard_page_start, - PAGE_SIZE_USIZE, - PAGE_NOACCESS, - &mut unused_out_old_prot_flags, - ) - } { - log_then_return!(WindowsAPIError(e.clone())); + // Only set guard pages for SandboxMemory mappings. + // File-backed read-only mappings do not need guard pages + // because the host does not write to them. + if *mapping == SurrogateMapping::SandboxMemory { + let mut unused_out_old_prot_flags = PAGE_PROTECTION_FLAGS(0); + + // the first page of the raw_size is the guard page + let first_guard_page_start = surrogate_base.Value; + if let Err(e) = unsafe { + VirtualProtectEx( + self.process_handle.into(), + first_guard_page_start, + PAGE_SIZE_USIZE, + PAGE_NOACCESS, + &mut unused_out_old_prot_flags, + ) + } { + self.unmap_helper(surrogate_base.Value); + log_then_return!(WindowsAPIError(e.clone())); + } + + // the last page of the raw_size is the guard page + let last_guard_page_start = + unsafe { first_guard_page_start.add(host_size - PAGE_SIZE_USIZE) }; + if let Err(e) = unsafe { + VirtualProtectEx( + self.process_handle.into(), + last_guard_page_start, + PAGE_SIZE_USIZE, + PAGE_NOACCESS, + &mut unused_out_old_prot_flags, + ) + } { + self.unmap_helper(surrogate_base.Value); + log_then_return!(WindowsAPIError(e.clone())); + } } + ve.insert(HandleMapping { use_count: 1, surrogate_base: surrogate_base.Value, + mapping_type: *mapping, }); Ok(surrogate_base.Value) } @@ -126,15 +174,25 @@ impl SurrogateProcess { pub(super) fn unmap(&mut self, host_base: usize) { match self.mappings.entry(host_base) { Entry::Occupied(mut oe) => { - oe.get_mut().use_count -= 1; + oe.get_mut().use_count = oe.get().use_count.checked_sub(1).unwrap_or_else(|| { + tracing::error!( + "Surrogate unmap ref count underflow for host_base {:#x}", + host_base + ); + 0 + }); if oe.get().use_count == 0 { let entry = oe.remove(); self.unmap_helper(entry.surrogate_base); } } Entry::Vacant(_) => { + tracing::error!( + "Attempted to unmap from surrogate a region at host_base {:#x} that was never mapped", + host_base + ); #[cfg(debug_assertions)] - panic!("Attempted to unmap from surrogate a region that was never mapped") + panic!("Attempted to unmap from surrogate a region that was never mapped"); } } } diff --git a/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs b/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs index 966ed50f4..08c60e370 100644 --- a/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs +++ b/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs @@ -459,6 +459,7 @@ mod tests { HandleWrapper::from(mem.get_mmap_file_handle()), mem.raw_ptr() as usize, mem.raw_mem_size(), + &crate::mem::memory_region::SurrogateMapping::SandboxMemory, ) .unwrap(); @@ -498,4 +499,127 @@ mod tests { assert!(success.is_err()); } } + + /// Tests that [`SurrogateMapping::ReadOnlyFile`] skips guard pages entirely. + /// + /// When mapping with `ReadOnlyFile`, the first and last pages should be + /// accessible (no `PAGE_NOACCESS` guard pages set), unlike `SandboxMemory` + /// which marks them as guard pages. + #[test] + fn readonly_file_mapping_skips_guard_pages() { + const SIZE: usize = 4096; + let mgr = get_surrogate_process_manager().unwrap(); + let mem = ExclusiveSharedMemory::new(SIZE).unwrap(); + + let mut process = mgr.get_surrogate_process().unwrap(); + let surrogate_address = process + .map( + HandleWrapper::from(mem.get_mmap_file_handle()), + mem.raw_ptr() as usize, + mem.raw_mem_size(), + &crate::mem::memory_region::SurrogateMapping::ReadOnlyFile, + ) + .unwrap(); + + let buffer = vec![0u8; SIZE]; + let bytes_read: Option<*mut usize> = None; + let process_handle: HANDLE = process.process_handle.into(); + + unsafe { + // read the first page — should succeed (no guard page for ReadOnlyFile) + let success = windows::Win32::System::Diagnostics::Debug::ReadProcessMemory( + process_handle, + surrogate_address, + buffer.as_ptr() as *mut c_void, + SIZE, + bytes_read, + ); + assert!( + success.is_ok(), + "First page should be readable with ReadOnlyFile (no guard page)" + ); + + // read the middle page — should also succeed + let success = windows::Win32::System::Diagnostics::Debug::ReadProcessMemory( + process_handle, + surrogate_address.wrapping_add(SIZE), + buffer.as_ptr() as *mut c_void, + SIZE, + bytes_read, + ); + assert!( + success.is_ok(), + "Middle page should be readable with ReadOnlyFile" + ); + + // read the last page — should succeed (no guard page for ReadOnlyFile) + let success = windows::Win32::System::Diagnostics::Debug::ReadProcessMemory( + process_handle, + surrogate_address.wrapping_add(2 * SIZE), + buffer.as_ptr() as *mut c_void, + SIZE, + bytes_read, + ); + assert!( + success.is_ok(), + "Last page should be readable with ReadOnlyFile (no guard page)" + ); + } + } + + /// Tests that the reference counting in [`SurrogateProcess::map`] works + /// correctly — repeated maps to the same `host_base` increment the count + /// and return the same surrogate address, regardless of the mapping type + /// passed on subsequent calls. + #[test] + fn surrogate_map_ref_counting() { + let mgr = get_surrogate_process_manager().unwrap(); + let mem = ExclusiveSharedMemory::new(4096).unwrap(); + + let mut process = mgr.get_surrogate_process().unwrap(); + let handle = HandleWrapper::from(mem.get_mmap_file_handle()); + let host_base = mem.raw_ptr() as usize; + let host_size = mem.raw_mem_size(); + + // First map — creates the mapping + let addr1 = process + .map( + handle, + host_base, + host_size, + &crate::mem::memory_region::SurrogateMapping::SandboxMemory, + ) + .unwrap(); + + // Second map — should reuse (ref count incremented) + let addr2 = process + .map( + handle, + host_base, + host_size, + &crate::mem::memory_region::SurrogateMapping::SandboxMemory, + ) + .unwrap(); + + assert_eq!( + addr1, addr2, + "Repeated map should return the same surrogate address" + ); + + // First unmap — decrements ref count but should NOT actually unmap + process.unmap(host_base); + + // The mapping should still be present (ref count was 2, now 1) + assert!( + process.mappings.contains_key(&host_base), + "Mapping should still exist after first unmap (ref count > 0)" + ); + + // Second unmap — ref count hits 0, actually unmaps + process.unmap(host_base); + assert!( + !process.mappings.contains_key(&host_base), + "Mapping should be removed after ref count reaches 0" + ); + } } diff --git a/src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs b/src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs index be81f7f72..de6fa7497 100644 --- a/src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs +++ b/src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs @@ -144,6 +144,7 @@ impl VirtualMachine for WhpVm { region.host_region.start.from_handle, region.host_region.start.handle_base, region.host_region.start.handle_size, + ®ion.host_region.start.surrogate_mapping, ) .map_err(|e| MapMemoryError::SurrogateProcess(e.to_string()))?; let surrogate_addr = surrogate_base.wrapping_add(region.host_region.start.offset); diff --git a/src/hyperlight_host/src/mem/memory_region.rs b/src/hyperlight_host/src/mem/memory_region.rs index f93adb478..2bfcfdf49 100644 --- a/src/hyperlight_host/src/mem/memory_region.rs +++ b/src/hyperlight_host/src/mem/memory_region.rs @@ -112,10 +112,13 @@ impl TryFrom for MemoryRegionFlags { } } -// only used for debugging +// NOTE: In the future, all host-side knowledge about memory region types +// should collapse down to Snapshot vs Scratch (see shared_mem.rs). +// Until then, these variants help distinguish regions for diagnostics +// and crash dumps. Not part of the public API. #[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] /// The type of memory region -pub enum MemoryRegionType { +pub(crate) enum MemoryRegionType { /// The region contains the guest's code Code, /// The region contains the guest's init data @@ -128,6 +131,11 @@ pub enum MemoryRegionType { Scratch, /// The snapshot region Snapshot, + /// An externally-mapped file (via [`MultiUseSandbox::map_file_cow`]). + /// These regions are backed by file handles (Windows) or mmap + /// (Linux) and are read-only + executable. They are cleaned up + /// during restore/drop — not part of the guest's own allocator. + MappedFile, } /// A trait that distinguishes between different kinds of memory region representations. @@ -162,6 +170,22 @@ impl MemoryRegionKind for HostGuestMemoryRegion { base + size } } +/// Describes how a memory region should be mapped through the surrogate process +/// pipeline on Windows (WHP). +/// +/// Different mapping types require different page protections and guard page +/// behaviour when projected into the surrogate process via `MapViewOfFileNuma2`. +#[cfg(target_os = "windows")] +#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] +pub(crate) enum SurrogateMapping { + /// Standard sandbox shared memory: mapped with `PAGE_READWRITE` protection + /// and guard pages (`PAGE_NOACCESS`) set on the first and last pages. + SandboxMemory, + /// File-backed read-only mapping: mapped with `PAGE_READONLY` protection + /// and **no** guard pages. + ReadOnlyFile, +} + /// A [`HostRegionBase`] keeps track of not just a pointer, but also a /// file mapping into which it is pointing. This is used on WHP, /// where mapping the actual pointer into the VM actually involves @@ -178,6 +202,9 @@ pub struct HostRegionBase { /// The offset into file mapping region where this /// [`HostRegionBase`] is pointing. pub offset: usize, + /// How this region should be mapped through the surrogate process. + /// Controls page protection and guard page behaviour. + pub(crate) surrogate_mapping: SurrogateMapping, } #[cfg(target_os = "windows")] impl std::hash::Hash for HostRegionBase { @@ -189,6 +216,7 @@ impl std::hash::Hash for HostRegionBase { self.handle_base.hash(state); self.handle_size.hash(state); self.offset.hash(state); + self.surrogate_mapping.hash(state); } } #[cfg(target_os = "windows")] @@ -214,6 +242,7 @@ impl MemoryRegionKind for HostGuestMemoryRegion { handle_base: base.handle_base, handle_size: base.handle_size, offset: base.offset + size, + surrogate_mapping: base.surrogate_mapping, } } } @@ -243,7 +272,7 @@ pub struct MemoryRegion_ { /// memory access flags for the given region pub flags: MemoryRegionFlags, /// the type of memory region - pub region_type: MemoryRegionType, + pub(crate) region_type: MemoryRegionType, } /// A memory region that tracks both host and guest addresses. @@ -413,3 +442,161 @@ impl From<&MemoryRegion> for kvm_bindings::kvm_userspace_memory_region { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn memory_region_flags_display() { + assert_eq!(format!("{}", MemoryRegionFlags::NONE), "NONE"); + assert_eq!(format!("{}", MemoryRegionFlags::READ), "READ"); + assert_eq!( + format!("{}", MemoryRegionFlags::READ | MemoryRegionFlags::WRITE), + "READ | WRITE" + ); + assert_eq!( + format!( + "{}", + MemoryRegionFlags::READ | MemoryRegionFlags::WRITE | MemoryRegionFlags::EXECUTE + ), + "READ | WRITE | EXECUTE" + ); + } + + #[cfg(target_os = "windows")] + mod windows_tests { + use std::collections::HashSet; + use std::hash::{DefaultHasher, Hash, Hasher}; + + use super::*; + + /// Helper to create a `HostRegionBase` with the given `SurrogateMapping`. + fn make_host_region_base(mapping: SurrogateMapping) -> HostRegionBase { + HostRegionBase { + from_handle: windows::Win32::Foundation::INVALID_HANDLE_VALUE.into(), + handle_base: 0x1000, + handle_size: 0x2000, + offset: 0x100, + surrogate_mapping: mapping, + } + } + + fn hash_of(val: &impl Hash) -> u64 { + let mut hasher = DefaultHasher::new(); + val.hash(&mut hasher); + hasher.finish() + } + + #[test] + fn surrogate_mapping_equality() { + assert_eq!( + SurrogateMapping::SandboxMemory, + SurrogateMapping::SandboxMemory + ); + assert_eq!( + SurrogateMapping::ReadOnlyFile, + SurrogateMapping::ReadOnlyFile + ); + assert_ne!( + SurrogateMapping::SandboxMemory, + SurrogateMapping::ReadOnlyFile + ); + } + + #[test] + fn surrogate_mapping_variants_are_distinct() { + // Verify the two variants are distinct values that can be + // used to key different behaviour in the surrogate pipeline + let sandbox = SurrogateMapping::SandboxMemory; + let readonly = SurrogateMapping::ReadOnlyFile; + assert_ne!(sandbox, readonly); + + // Verify Copy semantics work (enum is Copy + Eq) + let copy = sandbox; + assert_eq!(sandbox, copy); + } + + #[test] + fn host_region_base_different_surrogate_mapping_not_equal() { + let sandbox = make_host_region_base(SurrogateMapping::SandboxMemory); + let readonly = make_host_region_base(SurrogateMapping::ReadOnlyFile); + assert_ne!(sandbox, readonly); + } + + #[test] + fn host_region_base_different_surrogate_mapping_different_hash() { + let sandbox = make_host_region_base(SurrogateMapping::SandboxMemory); + let readonly = make_host_region_base(SurrogateMapping::ReadOnlyFile); + assert_ne!(hash_of(&sandbox), hash_of(&readonly)); + } + + #[test] + fn host_region_base_same_surrogate_mapping_equal_and_same_hash() { + let a = make_host_region_base(SurrogateMapping::SandboxMemory); + let b = make_host_region_base(SurrogateMapping::SandboxMemory); + assert_eq!(a, b); + assert_eq!(hash_of(&a), hash_of(&b)); + } + + #[test] + fn host_region_base_into_usize() { + let hrb = make_host_region_base(SurrogateMapping::SandboxMemory); + let addr: usize = hrb.into(); + assert_eq!(addr, 0x1000 + 0x100); // handle_base + offset + } + + #[test] + fn memory_region_kind_add_preserves_surrogate_mapping() { + let base = make_host_region_base(SurrogateMapping::ReadOnlyFile); + let result = ::add(base, 0x400); + + assert_eq!(result.from_handle, base.from_handle); + assert_eq!(result.handle_base, base.handle_base); + assert_eq!(result.handle_size, base.handle_size); + assert_eq!(result.offset, base.offset + 0x400); + assert_eq!(result.surrogate_mapping, SurrogateMapping::ReadOnlyFile); + } + + #[test] + fn host_region_base_works_in_hashset() { + let sandbox = make_host_region_base(SurrogateMapping::SandboxMemory); + let readonly = make_host_region_base(SurrogateMapping::ReadOnlyFile); + + let mut set = HashSet::new(); + set.insert(sandbox); + set.insert(readonly); + // Both should be present since they differ by surrogate_mapping + assert_eq!(set.len(), 2); + + // Inserting a duplicate should not increase the count + set.insert(make_host_region_base(SurrogateMapping::SandboxMemory)); + assert_eq!(set.len(), 2); + } + + #[test] + fn memory_region_with_surrogate_mapping_equality() { + let base_sandbox = make_host_region_base(SurrogateMapping::SandboxMemory); + let base_readonly = make_host_region_base(SurrogateMapping::ReadOnlyFile); + + let region_sandbox = MemoryRegion { + guest_region: 0x0..0x2000, + host_region: base_sandbox + ..::add(base_sandbox, 0x2000), + flags: MemoryRegionFlags::READ, + region_type: MemoryRegionType::Code, + }; + + let region_readonly = MemoryRegion { + guest_region: 0x0..0x2000, + host_region: base_readonly + ..::add(base_readonly, 0x2000), + flags: MemoryRegionFlags::READ, + region_type: MemoryRegionType::Code, + }; + + // Regions with different surrogate_mapping should not be equal + assert_ne!(region_sandbox, region_readonly); + } + } +} diff --git a/src/hyperlight_host/src/mem/shared_mem.rs b/src/hyperlight_host/src/mem/shared_mem.rs index 2cd3b8acc..a5306dcaf 100644 --- a/src/hyperlight_host/src/mem/shared_mem.rs +++ b/src/hyperlight_host/src/mem/shared_mem.rs @@ -759,6 +759,7 @@ pub trait SharedMemory { handle_base: self.region().ptr as usize, handle_size: self.region().size, offset: PAGE_SIZE_USIZE, + surrogate_mapping: super::memory_region::SurrogateMapping::SandboxMemory, } } diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index 82da52912..aae4b6dd9 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -30,6 +30,13 @@ use hyperlight_common::flatbuffer_wrappers::function_types::{ }; use hyperlight_common::flatbuffer_wrappers::util::estimate_flatbuffer_capacity; use tracing::{Span, instrument}; +#[cfg(target_os = "windows")] +use windows::Win32::Foundation::CloseHandle; +#[cfg(target_os = "windows")] +use windows::Win32::System::Memory::{ + CreateFileMappingW, FILE_MAP_READ, MEMORY_MAPPED_VIEW_ADDRESS, MapViewOfFile, PAGE_READONLY, + UnmapViewOfFile, +}; use super::Callable; use super::host_funcs::FunctionRegistry; @@ -38,9 +45,11 @@ use crate::HyperlightError::{self, SnapshotSandboxMismatch}; use crate::func::{ParameterTuple, SupportedReturnType}; use crate::hypervisor::InterruptHandle; use crate::hypervisor::hyperlight_vm::{HyperlightVm, HyperlightVmError}; -use crate::mem::memory_region::MemoryRegion; -#[cfg(unix)] -use crate::mem::memory_region::{MemoryRegionFlags, MemoryRegionType}; +#[cfg(target_os = "windows")] +use crate::hypervisor::wrappers::HandleWrapper; +#[cfg(target_os = "windows")] +use crate::mem::memory_region::{HostRegionBase, MemoryRegionKind, SurrogateMapping}; +use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionType}; use crate::mem::mgr::SandboxMemoryManager; use crate::mem::shared_mem::HostSharedMemory; use crate::metrics::{ @@ -48,6 +57,54 @@ use crate::metrics::{ }; use crate::{Result, log_then_return}; +/// Tracks a host-side file mapping created by [`MultiUseSandbox::map_file_cow`] +/// on Windows, providing RAII cleanup of the underlying Windows handles. +/// +/// When dropped, releases both the `MapViewOfFile` view and the +/// `CreateFileMappingW` handle, preventing resource leaks. +#[cfg(target_os = "windows")] +struct OwnedFileMapping { + /// Guest base address used to identify which region this mapping + /// belongs to (for cleanup during [`MultiUseSandbox::restore`]). + guest_base: usize, + /// Host-side base pointer returned by `MapViewOfFile`. + view_base: *mut core::ffi::c_void, + /// File mapping handle returned by `CreateFileMappingW`. + mapping_handle: HandleWrapper, +} + +#[cfg(target_os = "windows")] +impl Drop for OwnedFileMapping { + fn drop(&mut self) { + // Unmap the view first, then close the file mapping handle. + // Order matters: the view holds a reference to the mapping. + unsafe { + if let Err(e) = UnmapViewOfFile(MEMORY_MAPPED_VIEW_ADDRESS { + Value: self.view_base, + }) { + tracing::error!("Failed to unmap file view at {:?}: {:?}", self.view_base, e); + } + let mapping_handle = self.mapping_handle.into(); + if let Err(e) = CloseHandle(mapping_handle) { + tracing::error!( + "Failed to close file mapping handle {:?}: {:?}", + mapping_handle, + e + ); + } + } + } +} + +// SAFETY: `view_base` is a pointer to a Windows memory-mapped view created by +// `MapViewOfFile`. Both `UnmapViewOfFile` and `CloseHandle` are thread-safe +// Win32 APIs that operate on kernel objects, so they can safely be called +// from any thread. The `HandleWrapper` is already `Send + Sync`. +#[cfg(target_os = "windows")] +unsafe impl Send for OwnedFileMapping {} +#[cfg(target_os = "windows")] +unsafe impl Sync for OwnedFileMapping {} + /// A fully initialized sandbox that can execute guest functions multiple times. /// /// Guest functions can be called repeatedly while maintaining state between calls. @@ -99,6 +156,11 @@ pub struct MultiUseSandbox { /// If the current state of the sandbox has been captured in a snapshot, /// that snapshot is stored here. snapshot: Option>, + /// Tracks host-side file mappings created by [`Self::map_file_cow`] for + /// RAII cleanup. Declared **after** `vm` so that Rust's field drop order + /// releases hypervisor mappings before closing host-side file handles. + #[cfg(target_os = "windows")] + file_mappings: Vec, } impl MultiUseSandbox { @@ -123,6 +185,8 @@ impl MultiUseSandbox { #[cfg(gdb)] dbg_mem_access_fn, snapshot: None, + #[cfg(target_os = "windows")] + file_mappings: Vec::new(), } } @@ -329,6 +393,13 @@ impl MultiUseSandbox { self.vm .unmap_region(region) .map_err(HyperlightVmError::UnmapRegion)?; + + // Clean up host-side file mapping resources for regions that + // were created by map_file_cow. The OwnedFileMapping::drop + // will call UnmapViewOfFile + CloseHandle. + #[cfg(target_os = "windows")] + self.file_mappings + .retain(|fm| fm.guest_base != region.guest_region.start); } for region in regions_to_map { @@ -552,17 +623,112 @@ impl MultiUseSandbox { /// /// This method will return [`crate::HyperlightError::PoisonedSandbox`] if the sandbox /// is currently poisoned. Use [`restore()`](Self::restore) to recover from a poisoned state. - #[instrument(err(Debug), skip(self, _fp, _guest_base), parent = Span::current())] - pub fn map_file_cow(&mut self, _fp: &Path, _guest_base: u64) -> Result { + #[instrument(err(Debug), skip(self, file_path, guest_base), parent = Span::current())] + pub fn map_file_cow(&mut self, file_path: &Path, guest_base: u64) -> Result { if self.poisoned { return Err(crate::HyperlightError::PoisonedSandbox); } #[cfg(windows)] - log_then_return!("mmap'ing a file into the guest is not yet supported on Windows"); + { + use std::os::windows::io::AsRawHandle; + + use windows::Win32::Foundation::HANDLE; + + let file = std::fs::File::options().read(true).open(file_path)?; + let file_size = file.metadata()?.len(); + if file_size == 0 { + log_then_return!("map_file_cow: cannot map an empty file: {:?}", file_path); + } + let page_size = page_size::get(); + let size = usize::try_from(file_size).map_err(|_| { + HyperlightError::Error(format!( + "File size {file_size} exceeds addressable range on this platform" + )) + })?; + let size = size.div_ceil(page_size) * page_size; + + let file_handle = HANDLE(file.as_raw_handle()); + + // Create a read-only file mapping object backed by the actual file. + // Pass 0,0 for size to use the file's actual size — Windows will + // NOT extend a read-only file, so requesting page-aligned size + // would fail for files smaller than one page. MapViewOfFile and + // the surrogate process will round up to page boundaries internally. + let mapping_handle = + unsafe { CreateFileMappingW(file_handle, None, PAGE_READONLY, 0, 0, None) } + .map_err(|e| { + HyperlightError::Error(format!("CreateFileMappingW failed: {e}")) + })?; + + // Map a read-only view into the host process. + // Passing 0 for dwNumberOfBytesToMap maps the entire file; the OS + // rounds up to the next page boundary and zero-fills the remainder. + let view = unsafe { MapViewOfFile(mapping_handle, FILE_MAP_READ, 0, 0, 0) }; + if view.Value.is_null() { + // Clean up the mapping handle before returning + unsafe { + let _ = CloseHandle(mapping_handle); + } + log_then_return!( + "MapViewOfFile failed: {:?}", + std::io::Error::last_os_error() + ); + } + + let host_base = HostRegionBase { + from_handle: HandleWrapper::from(mapping_handle), + handle_base: view.Value as usize, + handle_size: size, + offset: 0, + surrogate_mapping: SurrogateMapping::ReadOnlyFile, + }; + + let host_end = + ::add( + host_base, size, + ); + + let region = MemoryRegion { + host_region: host_base..host_end, + guest_region: guest_base as usize..guest_base as usize + size, + flags: MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, + region_type: MemoryRegionType::MappedFile, + }; + + // Reset snapshot since we are mutating the sandbox state + self.snapshot = None; + + if let Err(err) = unsafe { self.vm.map_region(®ion) } + .map_err(HyperlightVmError::MapRegion) + .map_err(HyperlightError::HyperlightVmError) + { + // Clean up host-side resources on failure + unsafe { + let _ = UnmapViewOfFile(view); + let _ = CloseHandle(mapping_handle); + } + return Err(err); + } + + self.mem_mgr.mapped_rgns += 1; + self.file_mappings.push(OwnedFileMapping { + guest_base: guest_base as usize, + view_base: view.Value, + mapping_handle: HandleWrapper::from(mapping_handle), + }); + + Ok(size as u64) + } #[cfg(unix)] unsafe { - let file = std::fs::File::options().read(true).write(true).open(_fp)?; + let file = std::fs::File::options() + .read(true) + .write(true) + .open(file_path)?; let file_size = file.metadata()?.st_size(); + if file_size == 0 { + log_then_return!("map_file_cow: cannot map an empty file: {:?}", file_path); + } let page_size = page_size::get(); let size = (file_size as usize).div_ceil(page_size) * page_size; let base = libc::mmap( @@ -579,9 +745,9 @@ impl MultiUseSandbox { if let Err(err) = self.map_region(&MemoryRegion { host_region: base as usize..base.wrapping_add(size) as usize, - guest_region: _guest_base as usize.._guest_base as usize + size, + guest_region: guest_base as usize..guest_base as usize + size, flags: MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, - region_type: MemoryRegionType::Heap, + region_type: MemoryRegionType::MappedFile, }) { libc::munmap(base, size); return Err(err); @@ -1539,4 +1705,450 @@ mod tests { let start = code_gva + 4096 - 100; assert_gva_read_matches(&mut sbox, start, 200); } + + /// Tests that [`OwnedFileMapping`] properly releases Windows handles on drop. + /// + /// Creates a temp file, opens a `CreateFileMappingW` + `MapViewOfFile`, + /// wraps in `OwnedFileMapping`, drops it, and verifies the file can be + /// deleted (no leaked handles holding it open). + #[test] + #[cfg(target_os = "windows")] + fn owned_file_mapping_drop_releases_handles() { + use std::io::Write; + use std::os::windows::io::AsRawHandle; + + use windows::Win32::Foundation::HANDLE; + use windows::Win32::System::Memory::{ + CreateFileMappingW, FILE_MAP_READ, MapViewOfFile, PAGE_READONLY, + }; + + use super::OwnedFileMapping; + use crate::hypervisor::wrappers::HandleWrapper; + + // Create a temp file with some content + let temp_dir = std::env::temp_dir(); + let temp_path = temp_dir.join("hyperlight_test_owned_file_mapping.bin"); + // Clean up from any previous failed run + let _ = std::fs::remove_file(&temp_path); + + let mapping_handle; + let view_base; + + { + // Create and write to the file, then keep it open for mapping + let file = std::fs::OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(true) + .open(&temp_path) + .unwrap(); + file.set_len(4096).unwrap(); + (&file).write_all(&[0xAA; 4096]).unwrap(); + + let file_handle = HANDLE(file.as_raw_handle()); + + mapping_handle = unsafe { + CreateFileMappingW(file_handle, None, PAGE_READONLY, 0, 0, None) + .expect("CreateFileMappingW failed") + }; + + // File is dropped here (fd closed), but the mapping keeps the file open + } + + { + let view = unsafe { MapViewOfFile(mapping_handle, FILE_MAP_READ, 0, 0, 0) }; + assert!( + !view.Value.is_null(), + "MapViewOfFile should return a non-null pointer" + ); + view_base = view.Value; + + let _owned = OwnedFileMapping { + guest_base: 0x1000, + view_base, + mapping_handle: HandleWrapper::from(mapping_handle), + }; + // _owned is dropped here — should call UnmapViewOfFile + CloseHandle + } + + // After drop, the file should no longer be held open by our handles + std::fs::remove_file(&temp_path) + .expect("File should be deletable after OwnedFileMapping is dropped"); + } + + /// Helper: create a temp file with known content, padded to be + /// at least page-aligned (4096 bytes). Returns the path and the + /// *original* content bytes (before padding). + fn create_test_file(name: &str, content: &[u8]) -> (std::path::PathBuf, Vec) { + use std::io::Write; + + let page_size = page_size::get(); + let padded_len = content.len().max(page_size).div_ceil(page_size) * page_size; + let mut padded = vec![0u8; padded_len]; + padded[..content.len()].copy_from_slice(content); + + let temp_dir = std::env::temp_dir(); + let path = temp_dir.join(name); + let _ = std::fs::remove_file(&path); // clean up from previous runs + let mut f = std::fs::File::create(&path).unwrap(); + f.write_all(&padded).unwrap(); + (path, content.to_vec()) + } + + /// Tests the basic `map_file_cow` flow: map a file, read its content + /// from the guest, and verify it matches. + #[test] + fn test_map_file_cow_basic() { + let expected = b"hello world from map_file_cow"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_basic.bin", expected); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + let mapped_size = sbox.map_file_cow(&path, guest_base).unwrap(); + assert!(mapped_size > 0, "mapped_size should be positive"); + assert!( + mapped_size >= expected.len() as u64, + "mapped_size should be >= file content length" + ); + + // Read the content back from the guest + let actual: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + + assert_eq!( + actual, expected_bytes, + "Guest should read back the exact file content" + ); + + // Clean up + let _ = std::fs::remove_file(&path); + } + + /// Tests that `map_file_cow` enforces read-only access: writing to + /// the mapped region from the guest should cause a MemoryAccessViolation. + #[test] + fn test_map_file_cow_read_only_enforcement() { + let content = &[0xBB; 4096]; + let (path, _) = create_test_file("hyperlight_test_map_file_cow_readonly.bin", content); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + sbox.map_file_cow(&path, guest_base).unwrap(); + + // Writing to the mapped region should fail with MemoryAccessViolation + let err = sbox + .call::("WriteMappedBuffer", (guest_base, content.len() as u64)) + .unwrap_err(); + + match err { + HyperlightError::MemoryAccessViolation(addr, ..) if addr == guest_base => {} + _ => panic!( + "Expected MemoryAccessViolation at guest_base, got: {:?}", + err + ), + }; + + // Clean up + let _ = std::fs::remove_file(&path); + } + + /// Tests that `map_file_cow` returns `PoisonedSandbox` when the + /// sandbox is poisoned. + #[test] + fn test_map_file_cow_poisoned() { + let (path, _) = create_test_file("hyperlight_test_map_file_cow_poison.bin", &[0xCC; 4096]); + + let mut sbox: MultiUseSandbox = { + let path = simple_guest_as_string().unwrap(); + let u_sbox = UninitializedSandbox::new(GuestBinary::FilePath(path), None).unwrap(); + u_sbox.evolve() + } + .unwrap(); + let snapshot = sbox.snapshot().unwrap(); + + // Poison the sandbox + let _ = sbox + .call::<()>("guest_panic", "hello".to_string()) + .unwrap_err(); + assert!(sbox.poisoned()); + + // map_file_cow should fail with PoisonedSandbox + let err = sbox.map_file_cow(&path, 0x1_0000_0000).unwrap_err(); + assert!(matches!(err, HyperlightError::PoisonedSandbox)); + + // Restore and verify map_file_cow works again + sbox.restore(snapshot).unwrap(); + assert!(!sbox.poisoned()); + let result = sbox.map_file_cow(&path, 0x1_0000_0000); + assert!(result.is_ok()); + + let _ = std::fs::remove_file(&path); + } + + /// Tests that two separate sandboxes can map the same file + /// simultaneously and both read it correctly. + #[test] + fn test_map_file_cow_multi_vm_same_file() { + let expected = b"shared file content across VMs"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_multi_vm.bin", expected); + + let guest_base: u64 = 0x1_0000_0000; + + let mut sbox1 = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let mut sbox2 = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + // Map the same file into both sandboxes + sbox1.map_file_cow(&path, guest_base).unwrap(); + sbox2.map_file_cow(&path, guest_base).unwrap(); + + // Both should read the correct content + let actual1: Vec = sbox1 + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + let actual2: Vec = sbox2 + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + + assert_eq!( + actual1, expected_bytes, + "Sandbox 1 should read correct content" + ); + assert_eq!( + actual2, expected_bytes, + "Sandbox 2 should read correct content" + ); + + let _ = std::fs::remove_file(&path); + } + + /// Tests that multiple threads can each create a sandbox, map the + /// same file, read it, and drop without errors. + #[test] + fn test_map_file_cow_multi_vm_threaded() { + let expected = b"threaded file mapping test data"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_threaded.bin", expected); + + const NUM_THREADS: usize = 5; + let path = Arc::new(path); + let expected_bytes = Arc::new(expected_bytes); + let barrier = Arc::new(Barrier::new(NUM_THREADS)); + let mut handles = vec![]; + + for _ in 0..NUM_THREADS { + let path = path.clone(); + let expected_bytes = expected_bytes.clone(); + let barrier = barrier.clone(); + + handles.push(thread::spawn(move || { + barrier.wait(); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + sbox.map_file_cow(&path, guest_base).unwrap(); + + let actual: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + + assert_eq!(actual, *expected_bytes); + })); + } + + for h in handles { + h.join().unwrap(); + } + + let _ = std::fs::remove_file(&*path); + } + + /// Tests that file cleanup works after dropping a sandbox that used + /// `map_file_cow` — the file should be deletable (no leaked handles). + #[test] + #[cfg(target_os = "windows")] + fn test_map_file_cow_cleanup_no_handle_leak() { + let (path, _) = create_test_file("hyperlight_test_map_file_cow_cleanup.bin", &[0xDD; 4096]); + + { + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + sbox.map_file_cow(&path, 0x1_0000_0000).unwrap(); + // sandbox dropped here + } + + std::fs::remove_file(&path) + .expect("File should be deletable after sandbox with map_file_cow is dropped"); + } + + /// Tests snapshot/restore cycle with map_file_cow: + /// snapshot₁ (no file) → map file → snapshot₂ → restore₁ (unmapped) + /// → restore₂ (data folded into snapshot). + #[test] + fn test_map_file_cow_snapshot_remapping_cycle() { + let expected = b"snapshot remapping cycle test!"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_snapshot_remap.bin", expected); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + + // 1. snapshot₁ — no file mapped + let snapshot1 = sbox.snapshot().unwrap(); + + // 2. Map the file + sbox.map_file_cow(&path, guest_base).unwrap(); + + // Verify we can read it + let actual: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + assert_eq!(actual, expected_bytes); + + // 3. snapshot₂ — file mapped (data folded into snapshot) + let snapshot2 = sbox.snapshot().unwrap(); + + // 4. Restore to snapshot₁ — file should be unmapped + sbox.restore(snapshot1.clone()).unwrap(); + let is_mapped: bool = sbox.call("CheckMapped", (guest_base,)).unwrap(); + assert!( + !is_mapped, + "Region should be unmapped after restoring to snapshot₁" + ); + + // 5. Restore to snapshot₂ — data should still be readable + // (folded into snapshot memory, not the original file mapping) + sbox.restore(snapshot2).unwrap(); + let is_mapped: bool = sbox.call("CheckMapped", (guest_base,)).unwrap(); + assert!( + is_mapped, + "Region should be mapped after restoring to snapshot₂" + ); + let actual2: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, false), + ) + .unwrap(); + assert_eq!( + actual2, expected_bytes, + "Data should be intact after snapshot₂ restore" + ); + + let _ = std::fs::remove_file(&path); + } + + /// Tests that snapshot correctly captures map_file_cow data and + /// restore brings it back. + #[test] + fn test_map_file_cow_snapshot_restore() { + let expected = b"snapshot restore basic test!!"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_snap_restore.bin", expected); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + sbox.map_file_cow(&path, guest_base).unwrap(); + + // Read the content to verify mapping works + let actual: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + assert_eq!(actual, expected_bytes); + + // Take snapshot — folds file data into snapshot memory + let snapshot = sbox.snapshot().unwrap(); + + // Restore — the file-backed region is unmapped but data is in snapshot + sbox.restore(snapshot).unwrap(); + + // Data should still be readable from snapshot memory + let actual2: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, false), + ) + .unwrap(); + assert_eq!( + actual2, expected_bytes, + "Data should be readable after restore from snapshot" + ); + + let _ = std::fs::remove_file(&path); + } }