From 820d60ce16856575486cdfe1013a850f5f494951 Mon Sep 17 00:00:00 2001 From: Lucy Menon <168595099+syntactically@users.noreply.github.com> Date: Tue, 17 Mar 2026 01:00:20 +0000 Subject: [PATCH 1/5] Move PEB setup to snapshot creation For historical reasons, PEB setup still happened when an uninitialised sandbox was created, instead of happening at snapshot time, which is far more sensible. This removes the need for the snapshot memory to be writable during initialisation. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com> --- src/hyperlight_host/src/error.rs | 5 -- src/hyperlight_host/src/mem/layout.rs | 58 +++++++++---------- src/hyperlight_host/src/mem/mgr.rs | 11 ---- src/hyperlight_host/src/sandbox/outb.rs | 16 +---- src/hyperlight_host/src/sandbox/snapshot.rs | 2 + .../src/sandbox/uninitialized.rs | 4 +- 6 files changed, 34 insertions(+), 62 deletions(-) diff --git a/src/hyperlight_host/src/error.rs b/src/hyperlight_host/src/error.rs index 831792cc3..0a5ec25a1 100644 --- a/src/hyperlight_host/src/error.rs +++ b/src/hyperlight_host/src/error.rs @@ -104,10 +104,6 @@ pub enum HyperlightError { #[error("Unsupported type: {0}")] GuestInterfaceUnsupportedType(String), - /// The guest offset is invalid. - #[error("The guest offset {0} is invalid.")] - GuestOffsetIsInvalid(usize), - /// The guest binary was built with a different hyperlight-guest-bin version than the host expects. /// Hyperlight currently provides no backwards compatibility guarantees for guest binaries, /// so the guest and host versions must match exactly. This might change in the future. @@ -369,7 +365,6 @@ impl HyperlightError { | HyperlightError::GuestExecutionHungOnHostFunctionCall() | HyperlightError::GuestFunctionCallAlreadyInProgress() | HyperlightError::GuestInterfaceUnsupportedType(_) - | HyperlightError::GuestOffsetIsInvalid(_) | HyperlightError::HostFunctionNotFound(_) | HyperlightError::HyperlightVmError(HyperlightVmError::Create(_)) | HyperlightError::HyperlightVmError(HyperlightVmError::Initialize(_)) diff --git a/src/hyperlight_host/src/mem/layout.rs b/src/hyperlight_host/src/mem/layout.rs index 829c61234..f7a6c2ec1 100644 --- a/src/hyperlight_host/src/mem/layout.rs +++ b/src/hyperlight_host/src/mem/layout.rs @@ -72,9 +72,7 @@ use super::memory_region::{ MemoryRegionVecBuilder, }; use super::shared_mem::{ExclusiveSharedMemory, SharedMemory}; -use crate::error::HyperlightError::{ - GuestOffsetIsInvalid, MemoryRequestTooBig, MemoryRequestTooSmall, -}; +use crate::error::HyperlightError::{MemoryRequestTooBig, MemoryRequestTooSmall}; use crate::sandbox::SandboxConfiguration; use crate::{Result, new_error}; @@ -568,68 +566,70 @@ impl SandboxMemoryLayout { /// Note: `shared_mem` may have been modified, even if `Err` was returned /// from this function. #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")] - pub(crate) fn write( - &self, - shared_mem: &mut ExclusiveSharedMemory, - guest_offset: usize, - //TODO: Unused remove - _size: usize, - ) -> Result<()> { + pub(crate) fn write_peb(&self, mem: &mut [u8]) -> Result<()> { + let guest_offset = SandboxMemoryLayout::BASE_ADDRESS; + + fn write_u64(mem: &mut [u8], offset: usize, value: u64) -> Result<()> { + if offset + 8 > mem.len() { + return Err(new_error!( + "Cannot write to offset {} in slice of len {}", + offset, + mem.len() + )); + } + mem[offset..offset + 8].copy_from_slice(&u64::to_ne_bytes(value)); + Ok(()) + } + macro_rules! get_address { ($something:ident) => { u64::try_from(guest_offset + self.$something)? }; } - if guest_offset != SandboxMemoryLayout::BASE_ADDRESS - && guest_offset != shared_mem.base_addr() - { - return Err(GuestOffsetIsInvalid(guest_offset)); - } - // Start of setting up the PEB. The following are in the order of the PEB fields - // Skip guest_dispatch_function_ptr_offset because it is set by the guest - - // Skip code, is set when loading binary - // skip outb and outb context, is set when running in_proc - // Set up input buffer pointer - shared_mem.write_u64( + write_u64( + mem, self.get_input_data_size_offset(), self.sandbox_memory_config .get_input_data_size() .try_into()?, )?; - shared_mem.write_u64( + write_u64( + mem, self.get_input_data_pointer_offset(), self.get_input_data_buffer_gva(), )?; // Set up output buffer pointer - shared_mem.write_u64( + write_u64( + mem, self.get_output_data_size_offset(), self.sandbox_memory_config .get_output_data_size() .try_into()?, )?; - shared_mem.write_u64( + write_u64( + mem, self.get_output_data_pointer_offset(), self.get_output_data_buffer_gva(), )?; // Set up init data pointer - shared_mem.write_u64( + write_u64( + mem, self.get_init_data_size_offset(), (self.get_unaligned_memory_size() - self.init_data_offset).try_into()?, )?; let addr = get_address!(init_data_offset); - shared_mem.write_u64(self.get_init_data_pointer_offset(), addr)?; + write_u64(mem, self.get_init_data_pointer_offset(), addr)?; // Set up heap buffer pointer let addr = get_address!(guest_heap_buffer_offset); - shared_mem.write_u64(self.get_heap_size_offset(), self.heap_size.try_into()?)?; - shared_mem.write_u64(self.get_heap_pointer_offset(), addr)?; + write_u64(mem, self.get_heap_size_offset(), self.heap_size.try_into()?)?; + write_u64(mem, self.get_heap_pointer_offset(), addr)?; // Set up the file_mappings descriptor in the PEB. // - The `size` field holds the number of valid FileMappingInfo diff --git a/src/hyperlight_host/src/mem/mgr.rs b/src/hyperlight_host/src/mem/mgr.rs index b3bd1177f..6fc192506 100644 --- a/src/hyperlight_host/src/mem/mgr.rs +++ b/src/hyperlight_host/src/mem/mgr.rs @@ -302,17 +302,6 @@ impl SandboxMemoryManager { Ok(Self::new(layout, shared_mem, scratch_mem, entrypoint)) } - /// Write memory layout - #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")] - pub(crate) fn write_memory_layout(&mut self) -> Result<()> { - let mem_size = self.shared_mem.mem_size(); - self.layout.write( - &mut self.shared_mem, - SandboxMemoryLayout::BASE_ADDRESS, - mem_size, - ) - } - /// Wraps ExclusiveSharedMemory::build // Morally, this should not have to be a Result: this operation is // infallible. The source of the Result is diff --git a/src/hyperlight_host/src/sandbox/outb.rs b/src/hyperlight_host/src/sandbox/outb.rs index 76ec53cee..868b57e19 100644 --- a/src/hyperlight_host/src/sandbox/outb.rs +++ b/src/hyperlight_host/src/sandbox/outb.rs @@ -274,13 +274,7 @@ mod tests { let new_mgr = || { let bin = GuestBinary::FilePath(simple_guest_as_string().unwrap()); let snapshot = crate::sandbox::snapshot::Snapshot::from_env(bin, sandbox_cfg).unwrap(); - let mut mgr = SandboxMemoryManager::from_snapshot(&snapshot).unwrap(); - let mem_size = mgr.get_shared_mem_mut().mem_size(); - let layout = mgr.layout; - let shared_mem = mgr.get_shared_mem_mut(); - layout - .write(shared_mem, SandboxMemoryLayout::BASE_ADDRESS, mem_size) - .unwrap(); + let mgr = SandboxMemoryManager::from_snapshot(&snapshot).unwrap(); let (hmgr, _) = mgr.build().unwrap(); hmgr }; @@ -386,13 +380,7 @@ mod tests { let bin = GuestBinary::FilePath(simple_guest_as_string().unwrap()); let snapshot = crate::sandbox::snapshot::Snapshot::from_env(bin, sandbox_cfg).unwrap(); - let mut mgr = SandboxMemoryManager::from_snapshot(&snapshot).unwrap(); - let mem_size = mgr.get_shared_mem_mut().mem_size(); - let layout = mgr.layout; - let shared_mem = mgr.get_shared_mem_mut(); - layout - .write(shared_mem, SandboxMemoryLayout::BASE_ADDRESS, mem_size) - .unwrap(); + let mgr = SandboxMemoryManager::from_snapshot(&snapshot).unwrap(); let (hmgr, _) = mgr.build().unwrap(); hmgr }; diff --git a/src/hyperlight_host/src/sandbox/snapshot.rs b/src/hyperlight_host/src/sandbox/snapshot.rs index 816bfe792..24912be8d 100644 --- a/src/hyperlight_host/src/sandbox/snapshot.rs +++ b/src/hyperlight_host/src/sandbox/snapshot.rs @@ -382,6 +382,8 @@ impl Snapshot { &mut memory[layout.get_guest_code_offset()..], )?; + layout.write_peb(&mut memory)?; + blob.map(|x| layout.write_init_data(&mut memory, x.data)) .transpose()?; diff --git a/src/hyperlight_host/src/sandbox/uninitialized.rs b/src/hyperlight_host/src/sandbox/uninitialized.rs index 6cbe7921b..e737d08da 100644 --- a/src/hyperlight_host/src/sandbox/uninitialized.rs +++ b/src/hyperlight_host/src/sandbox/uninitialized.rs @@ -362,11 +362,9 @@ impl UninitializedSandbox { } }; - let mut mem_mgr_wrapper = + let mem_mgr_wrapper = SandboxMemoryManager::::from_snapshot(snapshot.as_ref())?; - mem_mgr_wrapper.write_memory_layout()?; - let host_funcs = Arc::new(Mutex::new(FunctionRegistry::default())); let mut sandbox = Self { From 31ea550b39ac9fa655231ed0001a5c1a1010e037 Mon Sep 17 00:00:00 2001 From: Lucy Menon <168595099+syntactically@users.noreply.github.com> Date: Tue, 17 Mar 2026 02:20:38 +0000 Subject: [PATCH 2/5] Fix new toolchain clippy warning about is_multiple_of Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com> --- src/hyperlight_common/src/version_note.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hyperlight_common/src/version_note.rs b/src/hyperlight_common/src/version_note.rs index 41b97f2ed..643dbd8ef 100644 --- a/src/hyperlight_common/src/version_note.rs +++ b/src/hyperlight_common/src/version_note.rs @@ -103,13 +103,13 @@ impl ElfNote { // desc must start at an 8-byte aligned offset from the note start. assert!( - core::mem::offset_of!(Self, desc) % 8 == 0, + core::mem::offset_of!(Self, desc).is_multiple_of(8), "desc is not 8-byte aligned" ); // Total note size must be a multiple of 8 for next-entry alignment. assert!( - size_of::() % 8 == 0, + size_of::().is_multiple_of(8), "total note size is not 8-byte aligned" ); From f22ebe778f16c8ab645a15c9afcd6138480c826c Mon Sep 17 00:00:00 2001 From: Lucy Menon <168595099+syntactically@users.noreply.github.com> Date: Tue, 17 Mar 2026 03:13:56 +0000 Subject: [PATCH 3/5] Avoid copying memory on snapshot restore, when possible Whenever we are not running with certain features that still require legacy writable access to the snapshot region (currently: nanvix-unstable, gdb), map the snapshot region directly into the VM. This lays the groundwork for sharing a single snapshot memory between multiple VMs. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com> --- src/hyperlight_host/build.rs | 4 + src/hyperlight_host/src/error.rs | 5 - src/hyperlight_host/src/hypervisor/gdb/mod.rs | 187 ++++----------- .../src/hypervisor/hyperlight_vm/mod.rs | 6 +- .../src/hypervisor/hyperlight_vm/x86_64.rs | 20 +- src/hyperlight_host/src/mem/layout.rs | 195 +++++++++++++++- src/hyperlight_host/src/mem/memory_region.rs | 8 +- src/hyperlight_host/src/mem/mgr.rs | 178 +++++++-------- src/hyperlight_host/src/mem/shared_mem.rs | 175 ++++++++++---- src/hyperlight_host/src/sandbox/outb.rs | 2 - src/hyperlight_host/src/sandbox/snapshot.rs | 214 ++++++------------ .../src/sandbox/trace/mem_profile.rs | 11 +- 12 files changed, 547 insertions(+), 458 deletions(-) diff --git a/src/hyperlight_host/build.rs b/src/hyperlight_host/build.rs index d599bedc1..6f3f9587a 100644 --- a/src/hyperlight_host/build.rs +++ b/src/hyperlight_host/build.rs @@ -105,6 +105,10 @@ fn main() -> Result<()> { crashdump: { all(feature = "crashdump", target_arch = "x86_64") }, // print_debug feature is aliased with debug_assertions to make it only available in debug-builds. print_debug: { all(feature = "print_debug", debug_assertions) }, + // the nanvix-unstable and gdb features both (only + // temporarily!) need to use writable/un-shared snapshot + // memories, and so can't share + unshared_snapshot_mem: { any(feature = "nanvix-unstable", feature = "gdb") }, } #[cfg(feature = "build-metadata")] diff --git a/src/hyperlight_host/src/error.rs b/src/hyperlight_host/src/error.rs index 0a5ec25a1..3a1d0955c 100644 --- a/src/hyperlight_host/src/error.rs +++ b/src/hyperlight_host/src/error.rs @@ -240,10 +240,6 @@ pub enum HyperlightError { #[error("Failed To Convert Return Value {0:?} to {1:?}")] ReturnValueConversionFailure(ReturnValue, &'static str), - /// Attempted to process a snapshot but the snapshot size does not match the current memory size - #[error("Snapshot Size Mismatch: Memory Size {0:?} Snapshot Size {1:?}")] - SnapshotSizeMismatch(usize, usize), - /// Tried to restore snapshot to a sandbox that is not the same as the one the snapshot was taken from #[error("Snapshot was taken from a different sandbox")] SnapshotSandboxMismatch, @@ -335,7 +331,6 @@ impl HyperlightError { | HyperlightError::PoisonedSandbox | HyperlightError::ExecutionAccessViolation(_) | HyperlightError::MemoryAccessViolation(_, _, _) - | HyperlightError::SnapshotSizeMismatch(_, _) | HyperlightError::MemoryRegionSizeMismatch(_, _, _) // HyperlightVmError::Restore is already handled manually in restore(), but we mark it // as poisoning here too for defense in depth. diff --git a/src/hyperlight_host/src/hypervisor/gdb/mod.rs b/src/hyperlight_host/src/hypervisor/gdb/mod.rs index 6ef5a3322..a85df0ee2 100644 --- a/src/hyperlight_host/src/hypervisor/gdb/mod.rs +++ b/src/hyperlight_host/src/hypervisor/gdb/mod.rs @@ -21,7 +21,7 @@ mod x86_64_target; use std::io::{self, ErrorKind}; use std::net::TcpListener; use std::sync::{Arc, Mutex}; -use std::{slice, thread}; +use std::thread; use crossbeam_channel::{Receiver, Sender, TryRecvError}; use event_loop::event_loop_thread; @@ -36,10 +36,10 @@ use super::regs::CommonRegisters; use crate::HyperlightError; use crate::hypervisor::regs::CommonFpu; use crate::hypervisor::virtual_machine::{HypervisorError, RegisterError, VirtualMachine}; -use crate::mem::layout::SandboxMemoryLayout; +use crate::mem::layout::BaseGpaRegion; use crate::mem::memory_region::MemoryRegion; use crate::mem::mgr::SandboxMemoryManager; -use crate::mem::shared_mem::{HostSharedMemory, SharedMemory}; +use crate::mem::shared_mem::HostSharedMemory; #[derive(Debug, Error)] pub enum GdbTargetError { @@ -95,18 +95,11 @@ pub enum DebugMemoryAccessError { LockFailed(&'static str, u32, String), #[error("Failed to translate guest address {0:#x}")] TranslateGuestAddress(u64), + #[error("Failed to write to read-only region")] + WriteToReadOnly, } impl DebugMemoryAccess { - // TODO: There is a lot of common logic between both of these - // functions, as well as guest_page/access_gpa in snapshot.rs. It - // would be nice to factor that out at some point, but the - // snapshot versions deal with ExclusiveSharedMemory, since we - // never expect a guest to be running concurrent with a snapshot, - // and doesn't want to make unnecessary copies, since it runs over - // relatively large volumes of data, so it's not clear if it's - // terribly easy to combine them - /// Reads memory from the guest's address space with a maximum length of a PAGE_SIZE /// /// # Arguments @@ -120,74 +113,17 @@ impl DebugMemoryAccess { data: &mut [u8], gpa: u64, ) -> std::result::Result<(), DebugMemoryAccessError> { - let read_len = data.len(); - - let mem_offset = (gpa as usize) - .checked_sub(SandboxMemoryLayout::BASE_ADDRESS) - .ok_or_else(|| { - log::warn!( - "gpa={:#X} causes subtract with underflow: \"gpa - BASE_ADDRESS={:#X}-{:#X}\"", - gpa, - gpa, - SandboxMemoryLayout::BASE_ADDRESS - ); - DebugMemoryAccessError::TranslateGuestAddress(gpa) - })?; - - // First check the mapped memory regions to see if the address is within any of them - let mut region_found = false; - for reg in self.guest_mmap_regions.iter() { - if reg.guest_region.contains(&mem_offset) { - log::debug!("Found mapped region containing {:X}: {:#?}", gpa, reg); - - // Region found - calculate the offset within the region - let region_offset = mem_offset.checked_sub(reg.guest_region.start).ok_or_else(|| { - log::warn!( - "Cannot calculate offset in memory region: mem_offset={:#X}, base={:#X}", - mem_offset, - reg.guest_region.start, - ); - DebugMemoryAccessError::TranslateGuestAddress(mem_offset as u64) - })?; - - let host_start_ptr = <_ as Into>::into(reg.host_region.start); - let bytes: &[u8] = unsafe { - slice::from_raw_parts(host_start_ptr as *const u8, reg.guest_region.len()) - }; - data[..read_len].copy_from_slice(&bytes[region_offset..region_offset + read_len]); - - region_found = true; - break; - } - } - - if !region_found { - let mut mgr = self - .dbg_mem_access_fn - .try_lock() - .map_err(|e| DebugMemoryAccessError::LockFailed(file!(), line!(), e.to_string()))?; - let scratch_base = - hyperlight_common::layout::scratch_base_gpa(mgr.scratch_mem.mem_size()); - let (mem, offset, name): (&mut HostSharedMemory, _, _) = if gpa >= scratch_base { - ( - &mut mgr.scratch_mem, - (gpa - scratch_base) as usize, - "scratch", - ) - } else { - (&mut mgr.shared_mem, mem_offset, "snapshot") - }; - log::debug!( - "No mapped region found containing {:X}. Trying {} memory at offset {:X} ...", - gpa, - name, - offset - ); - mem.copy_to_slice(&mut data[..read_len], offset) - .map_err(|e| DebugMemoryAccessError::CopyFailed(Box::new(e)))?; - } - - Ok(()) + let mgr = self + .dbg_mem_access_fn + .try_lock() + .map_err(|e| DebugMemoryAccessError::LockFailed(file!(), line!(), e.to_string()))?; + + mgr.layout + .resolve_gpa(gpa, &self.guest_mmap_regions) + .ok_or(DebugMemoryAccessError::TranslateGuestAddress(gpa))? + .with_memories(&mgr.shared_mem, &mgr.scratch_mem) + .copy_to_slice(data) + .map_err(|e| DebugMemoryAccessError::CopyFailed(Box::new(e))) } /// Writes memory from the guest's address space with a maximum length of a PAGE_SIZE @@ -203,74 +139,30 @@ impl DebugMemoryAccess { data: &[u8], gpa: u64, ) -> std::result::Result<(), DebugMemoryAccessError> { - let write_len = data.len(); - - let mem_offset = (gpa as usize) - .checked_sub(SandboxMemoryLayout::BASE_ADDRESS) - .ok_or_else(|| { - log::warn!( - "gpa={:#X} causes subtract with underflow: \"gpa - BASE_ADDRESS={:#X}-{:#X}\"", - gpa, - gpa, - SandboxMemoryLayout::BASE_ADDRESS - ); - DebugMemoryAccessError::TranslateGuestAddress(gpa) - })?; - - // First check the mapped memory regions to see if the address is within any of them - let mut region_found = false; - for reg in self.guest_mmap_regions.iter() { - if reg.guest_region.contains(&mem_offset) { - log::debug!("Found mapped region containing {:X}: {:#?}", gpa, reg); - - // Region found - calculate the offset within the region - let region_offset = mem_offset.checked_sub(reg.guest_region.start).ok_or_else(|| { - log::warn!( - "Cannot calculate offset in memory region: mem_offset={:#X}, base={:#X}", - mem_offset, - reg.guest_region.start, - ); - DebugMemoryAccessError::TranslateGuestAddress(mem_offset as u64) - })?; - - let host_start_ptr = <_ as Into>::into(reg.host_region.start); - let bytes: &mut [u8] = unsafe { - slice::from_raw_parts_mut(host_start_ptr as *mut u8, reg.guest_region.len()) - }; - bytes[region_offset..region_offset + write_len].copy_from_slice(&data[..write_len]); - - region_found = true; - break; - } - } - - if !region_found { - let mut mgr = self - .dbg_mem_access_fn - .try_lock() - .map_err(|e| DebugMemoryAccessError::LockFailed(file!(), line!(), e.to_string()))?; - let scratch_base = - hyperlight_common::layout::scratch_base_gpa(mgr.scratch_mem.mem_size()); - let (mem, offset, name): (&mut HostSharedMemory, _, _) = if gpa >= scratch_base { - ( - &mut mgr.scratch_mem, - (gpa - scratch_base) as usize, - "scratch", - ) - } else { - (&mut mgr.shared_mem, mem_offset, "snapshot") - }; - log::debug!( - "No mapped region found containing {:X}. Trying {} memory at offset {:X} ...", - gpa, - name, - offset - ); - mem.copy_from_slice(&data[..write_len], offset) - .map_err(|e| DebugMemoryAccessError::CopyFailed(Box::new(e)))?; + let mgr = self + .dbg_mem_access_fn + .try_lock() + .map_err(|e| DebugMemoryAccessError::LockFailed(file!(), line!(), e.to_string()))?; + + let resolved = mgr + .layout + .resolve_gpa(gpa, &self.guest_mmap_regions) + .ok_or(DebugMemoryAccessError::TranslateGuestAddress(gpa))?; + + // We can only safely write (without causing UB in the host + // process) if the address is in the scratch region + match resolved.base { + #[cfg(unshared_snapshot_mem)] + BaseGpaRegion::Snapshot(()) => mgr + .shared_mem + .copy_from_slice(data, resolved.offset) + .map_err(|e| DebugMemoryAccessError::CopyFailed(Box::new(e))), + BaseGpaRegion::Scratch(()) => mgr + .scratch_mem + .copy_from_slice(data, resolved.offset) + .map_err(|e| DebugMemoryAccessError::CopyFailed(Box::new(e))), + _ => Err(DebugMemoryAccessError::WriteToReadOnly), } - - Ok(()) } } @@ -490,6 +382,7 @@ mod tests { use hyperlight_testing::dummy_guest_as_string; use super::*; + use crate::mem::layout::SandboxMemoryLayout; use crate::mem::memory_region::{MemoryRegionFlags, MemoryRegionType}; use crate::sandbox::UninitializedSandbox; use crate::sandbox::uninitialized::GuestBinary; diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs index 4fa9fba36..830b856c0 100644 --- a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs @@ -47,7 +47,7 @@ use crate::hypervisor::virtual_machine::{ }; use crate::hypervisor::{InterruptHandle, InterruptHandleImpl}; use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionType}; -use crate::mem::mgr::SandboxMemoryManager; +use crate::mem::mgr::{SandboxMemoryManager, SnapshotSharedMemory}; use crate::mem::shared_mem::{GuestSharedMemory, HostSharedMemory, SharedMemory}; use crate::metrics::{METRIC_ERRONEOUS_VCPU_KICKS, METRIC_GUEST_CANCELLATION}; use crate::sandbox::host_funcs::FunctionRegistry; @@ -375,7 +375,7 @@ pub(crate) struct HyperlightVm { pub(super) snapshot_slot: u32, // The current snapshot region, used to keep it alive as long as // it is used & when unmapping - pub(super) snapshot_memory: Option, + pub(super) snapshot_memory: Option>, pub(super) scratch_slot: u32, // The slot number used for the scratch region // The current scratch region, used to keep it alive as long as it // is used & when unmapping @@ -460,7 +460,7 @@ impl HyperlightVm { /// Update the snapshot mapping to point to a new GuestSharedMemory pub(crate) fn update_snapshot_mapping( &mut self, - snapshot: GuestSharedMemory, + snapshot: SnapshotSharedMemory, ) -> Result<(), UpdateRegionError> { let guest_base = crate::mem::layout::SandboxMemoryLayout::BASE_ADDRESS as u64; let rgn = snapshot.mapping_at(guest_base, MemoryRegionType::Snapshot); diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs index e8f21e119..c10883ca7 100644 --- a/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs @@ -74,7 +74,7 @@ impl HyperlightVm { #[instrument(err(Debug), skip_all, parent = Span::current(), level = "Trace")] #[allow(clippy::too_many_arguments)] pub(crate) fn new( - snapshot_mem: GuestSharedMemory, + snapshot_mem: SnapshotSharedMemory, scratch_mem: GuestSharedMemory, _pml4_addr: u64, entrypoint: NextAction, @@ -885,7 +885,7 @@ mod tests { use crate::mem::memory_region::{GuestMemoryRegion, MemoryRegionFlags}; use crate::mem::mgr::{GuestPageTableBuffer, SandboxMemoryManager}; use crate::mem::ptr::RawPtr; - use crate::mem::shared_mem::ExclusiveSharedMemory; + use crate::mem::shared_mem::{ExclusiveSharedMemory, ReadonlySharedMemory}; use crate::sandbox::SandboxConfiguration; use crate::sandbox::host_funcs::FunctionRegistry; #[cfg(any(crashdump, gdb))] @@ -1479,20 +1479,22 @@ mod tests { layout.set_pt_size(pt_bytes.len()).unwrap(); let mem_size = layout.get_memory_size().unwrap(); - let mut eshm = ExclusiveSharedMemory::new(mem_size).unwrap(); + let mut snapshot_contents = vec![0u8; mem_size]; let snapshot_pt_start = mem_size - layout.get_pt_size(); - eshm.copy_from_slice(&pt_bytes, snapshot_pt_start).unwrap(); - eshm.copy_from_slice(code, layout.get_guest_code_offset()) - .unwrap(); + snapshot_contents[snapshot_pt_start..].copy_from_slice(&pt_bytes); + snapshot_contents + [layout.get_guest_code_offset()..layout.get_guest_code_offset() + code.len()] + .copy_from_slice(code); + layout.write_peb(&mut snapshot_contents).unwrap(); + let ro_mem = ReadonlySharedMemory::from_bytes(&snapshot_contents).unwrap(); let scratch_mem = ExclusiveSharedMemory::new(config.get_scratch_size()).unwrap(); - let mut mem_mgr = SandboxMemoryManager::new( + let mem_mgr = SandboxMemoryManager::new( layout, - eshm, + ro_mem.to_mgr_snapshot_mem().unwrap(), scratch_mem, NextAction::Initialise(layout.get_guest_code_address() as u64), ); - mem_mgr.write_memory_layout().unwrap(); let (mut hshm, gshm) = mem_mgr.build().unwrap(); diff --git a/src/hyperlight_host/src/mem/layout.rs b/src/hyperlight_host/src/mem/layout.rs index f7a6c2ec1..5f1e72ff1 100644 --- a/src/hyperlight_host/src/mem/layout.rs +++ b/src/hyperlight_host/src/mem/layout.rs @@ -68,14 +68,150 @@ use tracing::{Span, instrument}; use super::memory_region::MemoryRegionType::{Code, Heap, InitData, Peb}; use super::memory_region::{ - DEFAULT_GUEST_BLOB_MEM_FLAGS, MemoryRegion_, MemoryRegionFlags, MemoryRegionKind, + DEFAULT_GUEST_BLOB_MEM_FLAGS, MemoryRegion, MemoryRegion_, MemoryRegionFlags, MemoryRegionKind, MemoryRegionVecBuilder, }; -use super::shared_mem::{ExclusiveSharedMemory, SharedMemory}; +#[cfg(any(gdb, feature = "mem_profile"))] +use super::shared_mem::HostSharedMemory; +use super::shared_mem::{ExclusiveSharedMemory, ReadonlySharedMemory}; use crate::error::HyperlightError::{MemoryRequestTooBig, MemoryRequestTooSmall}; use crate::sandbox::SandboxConfiguration; use crate::{Result, new_error}; +pub(crate) enum BaseGpaRegion { + Snapshot(Sn), + Scratch(Sc), + Mmap(MemoryRegion), +} + +// It's an invariant of this type, checked on creation, that the +// offset is in bounds for the base region. +pub(crate) struct ResolvedGpa { + pub(crate) offset: usize, + pub(crate) base: BaseGpaRegion, +} + +impl AsRef<[u8]> for ExclusiveSharedMemory { + fn as_ref(&self) -> &[u8] { + self.as_slice() + } +} +impl AsRef<[u8]> for ReadonlySharedMemory { + fn as_ref(&self) -> &[u8] { + self.as_slice() + } +} + +impl ResolvedGpa { + pub(crate) fn with_memories(self, sn: Sn2, sc: Sc2) -> ResolvedGpa { + ResolvedGpa { + offset: self.offset, + base: match self.base { + BaseGpaRegion::Snapshot(_) => BaseGpaRegion::Snapshot(sn), + BaseGpaRegion::Scratch(_) => BaseGpaRegion::Scratch(sc), + BaseGpaRegion::Mmap(r) => BaseGpaRegion::Mmap(r), + }, + } + } +} +impl<'a> BaseGpaRegion<&'a [u8], &'a [u8]> { + pub(crate) fn as_ref<'b>(&'b self) -> &'a [u8] { + match self { + BaseGpaRegion::Snapshot(sn) => sn, + BaseGpaRegion::Scratch(sc) => sc, + BaseGpaRegion::Mmap(r) => unsafe { + #[allow(clippy::useless_conversion)] + let host_region_base: usize = r.host_region.start.into(); + #[allow(clippy::useless_conversion)] + let host_region_end: usize = r.host_region.end.into(); + let len = host_region_end - host_region_base; + std::slice::from_raw_parts(host_region_base as *const u8, len) + }, + } + } +} +impl<'a> ResolvedGpa<&'a [u8], &'a [u8]> { + pub(crate) fn as_ref<'b>(&'b self) -> &'a [u8] { + let base = self.base.as_ref(); + if self.offset > base.len() { + return &[]; + } + &self.base.as_ref()[self.offset..] + } +} +#[cfg(any(gdb, feature = "mem_profile"))] +pub(crate) trait ReadableSharedMemory { + fn copy_to_slice(&self, slice: &mut [u8], offset: usize) -> Result<()>; +} +#[cfg(any(gdb, feature = "mem_profile"))] +impl ReadableSharedMemory for &HostSharedMemory { + fn copy_to_slice(&self, slice: &mut [u8], offset: usize) -> Result<()> { + HostSharedMemory::copy_to_slice(self, slice, offset) + } +} +#[cfg(any(gdb, feature = "mem_profile"))] +mod coherence_hack { + use super::{ExclusiveSharedMemory, ReadonlySharedMemory}; + #[allow(unused)] // it actually is; see the impl below + pub(super) trait SharedMemoryAsRefMarker: AsRef<[u8]> {} + impl SharedMemoryAsRefMarker for ExclusiveSharedMemory {} + impl SharedMemoryAsRefMarker for &ExclusiveSharedMemory {} + impl SharedMemoryAsRefMarker for ReadonlySharedMemory {} + impl SharedMemoryAsRefMarker for &ReadonlySharedMemory {} +} +#[cfg(any(gdb, feature = "mem_profile"))] +impl ReadableSharedMemory for T { + fn copy_to_slice(&self, slice: &mut [u8], offset: usize) -> Result<()> { + let ss: &[u8] = self.as_ref(); + let end = offset + slice.len(); + if end > ss.len() { + return Err(new_error!( + "Attempt to read up to {} in memory of size {}", + offset + slice.len(), + self.as_ref().len() + )); + } + slice.copy_from_slice(&ss[offset..end]); + Ok(()) + } +} +#[cfg(any(gdb, feature = "mem_profile"))] +impl ResolvedGpa { + #[cfg(feature = "gdb")] + pub(crate) fn copy_to_slice(&self, slice: &mut [u8]) -> Result<()> { + match &self.base { + BaseGpaRegion::Snapshot(sn) => sn.copy_to_slice(slice, self.offset), + BaseGpaRegion::Scratch(sc) => sc.copy_to_slice(slice, self.offset), + BaseGpaRegion::Mmap(r) => unsafe { + #[allow(clippy::useless_conversion)] + let host_region_base: usize = r.host_region.start.into(); + #[allow(clippy::useless_conversion)] + let host_region_end: usize = r.host_region.end.into(); + let len = host_region_end - host_region_base; + // Safety: it's a documented invariant of MemoryRegion + // that the memory must remain alive as long as the + // sandbox is alive, and the way this code is used, + // the lifetimes of the snapshot and scratch memories + // ensure that the sandbox is still alive. This could + // perhaps be cleaned up/improved/made harder to + // misuse significantly, but it would require a much + // larger rework. + let ss = std::slice::from_raw_parts(host_region_base as *const u8, len); + let end = self.offset + slice.len(); + if end > ss.len() { + return Err(new_error!( + "Attempt to read up to {} in memory of size {}", + self.offset + slice.len(), + ss.len() + )); + } + slice.copy_from_slice(&ss[self.offset..end]); + Ok(()) + }, + } + } +} + #[derive(Copy, Clone)] pub(crate) struct SandboxMemoryLayout { pub(super) sandbox_memory_config: SandboxConfiguration, @@ -107,6 +243,9 @@ pub(crate) struct SandboxMemoryLayout { // The size of the scratch region in physical memory; note that // this will appear under the top of physical memory. scratch_size: usize, + // The size of the snapshot region in physical memory; note that + // this will appear somewhere near the base of physical memory. + snapshot_size: usize, } impl Debug for SandboxMemoryLayout { @@ -237,8 +376,7 @@ impl SandboxMemoryLayout { // make sure init data starts at 4K boundary let init_data_offset = (guest_heap_buffer_offset + heap_size).next_multiple_of(PAGE_SIZE_USIZE); - - Ok(Self { + let mut ret = Self { peb_offset, heap_size, peb_input_data_offset, @@ -256,7 +394,10 @@ impl SandboxMemoryLayout { init_data_permissions, pt_size: None, scratch_size, - }) + snapshot_size: 0, + }; + ret.set_snapshot_size(ret.get_memory_size()?); + Ok(ret) } /// Get the offset in guest memory to the output data size @@ -448,10 +589,17 @@ impl SandboxMemoryLayout { if self.scratch_size < min_scratch { return Err(MemoryRequestTooSmall(self.scratch_size, min_scratch)); } + let old_pt_size = self.pt_size.unwrap_or(0); + self.snapshot_size = self.snapshot_size - old_pt_size + size; self.pt_size = Some(size); Ok(()) } + #[instrument(skip_all, parent = Span::current(), level= "Trace")] + pub(crate) fn set_snapshot_size(&mut self, new_size: usize) { + self.snapshot_size = new_size; + } + /// Get the size of the memory region used for page tables #[instrument(skip_all, parent = Span::current(), level= "Trace")] pub(crate) fn get_pt_size(&self) -> usize { @@ -560,10 +708,10 @@ impl SandboxMemoryLayout { Ok(()) } - /// Write the finished memory layout to `shared_mem` and return - /// `Ok` if successful. + /// Write the finished memory layout to `mem` and return `Ok` if + /// successful. /// - /// Note: `shared_mem` may have been modified, even if `Err` was returned + /// Note: `mem` may have been modified, even if `Err` was returned /// from this function. #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")] pub(crate) fn write_peb(&self, mem: &mut [u8]) -> Result<()> { @@ -652,6 +800,37 @@ impl SandboxMemoryLayout { Ok(()) } + + /// Determine what region this gpa is in, and its offset into that region + pub(crate) fn resolve_gpa( + &self, + gpa: u64, + mmap_regions: &[MemoryRegion], + ) -> Option> { + let scratch_base = hyperlight_common::layout::scratch_base_gpa(self.scratch_size); + if gpa >= scratch_base && gpa < scratch_base + self.scratch_size as u64 { + return Some(ResolvedGpa { + offset: (gpa - scratch_base) as usize, + base: BaseGpaRegion::Scratch(()), + }); + } else if gpa >= SandboxMemoryLayout::BASE_ADDRESS as u64 + && gpa < SandboxMemoryLayout::BASE_ADDRESS as u64 + self.snapshot_size as u64 + { + return Some(ResolvedGpa { + offset: gpa as usize - SandboxMemoryLayout::BASE_ADDRESS, + base: BaseGpaRegion::Snapshot(()), + }); + } + for rgn in mmap_regions { + if gpa >= rgn.guest_region.start as u64 && gpa < rgn.guest_region.end as u64 { + return Some(ResolvedGpa { + offset: gpa as usize - rgn.guest_region.start, + base: BaseGpaRegion::Mmap(rgn.clone()), + }); + } + } + None + } } #[cfg(test)] diff --git a/src/hyperlight_host/src/mem/memory_region.rs b/src/hyperlight_host/src/mem/memory_region.rs index 4fc15c2ba..979b260dd 100644 --- a/src/hyperlight_host/src/mem/memory_region.rs +++ b/src/hyperlight_host/src/mem/memory_region.rs @@ -185,6 +185,12 @@ pub trait MemoryRegionKind { /// Type for memory regions that track both host and guest addresses. /// +/// When one of these is created, it always ends up in a sandbox +/// quickly. It's an invariant of this type that as long as one of +/// these is associated with a sandbox, it's always acceptable to read +/// from it, since a lot of the debug/crashdump/snapshot code +/// does. (Note: this means that _writable_ HostGuestMemoryRegions are +/// not possible to support at the moment). #[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] pub struct HostGuestMemoryRegion {} @@ -323,7 +329,7 @@ impl MemoryRegionKind for CrashDumpMemoryRegion { #[cfg(crashdump)] pub(crate) type CrashDumpRegion = MemoryRegion_; -#[cfg(crashdump)] +#[cfg(all(crashdump, feature = "nanvix-unstable"))] impl HostGuestMemoryRegion { /// Extract the raw `usize` host address from the platform-specific /// host base type. diff --git a/src/hyperlight_host/src/mem/mgr.rs b/src/hyperlight_host/src/mem/mgr.rs index 6fc192506..8c67ed6e2 100644 --- a/src/hyperlight_host/src/mem/mgr.rs +++ b/src/hyperlight_host/src/mem/mgr.rs @@ -27,13 +27,13 @@ use hyperlight_common::vmem::{BasicMapping, MappingKind}; use tracing::{Span, instrument}; use super::layout::SandboxMemoryLayout; -use super::shared_mem::{ExclusiveSharedMemory, GuestSharedMemory, HostSharedMemory, SharedMemory}; +use super::shared_mem::{ + ExclusiveSharedMemory, GuestSharedMemory, HostSharedMemory, ReadonlySharedMemory, SharedMemory, +}; use crate::hypervisor::regs::CommonSpecialRegisters; use crate::mem::memory_region::MemoryRegion; #[cfg(crashdump)] -use crate::mem::memory_region::{ - CrashDumpRegion, HostGuestMemoryRegion, MemoryRegionFlags, MemoryRegionType, -}; +use crate::mem::memory_region::{CrashDumpRegion, MemoryRegionFlags, MemoryRegionType}; use crate::sandbox::snapshot::{NextAction, Snapshot}; use crate::{Result, new_error}; @@ -94,46 +94,48 @@ fn try_coalesce_region( false } -/// Check dynamic mmap regions for a GPA that wasn't found in snapshot/scratch, -/// and push matching regions. -#[cfg(all(feature = "crashdump", not(feature = "nanvix-unstable")))] -fn resolve_from_mmap_regions( - regions: &mut Vec, - mapping: &hyperlight_common::vmem::Mapping, - virt_base: usize, - virt_end: usize, - mmap_regions: &[MemoryRegion], -) { - let phys_start = mapping.phys_base as usize; - let phys_end = (mapping.phys_base + mapping.len) as usize; - - for rgn in mmap_regions { - if phys_start >= rgn.guest_region.start && phys_end <= rgn.guest_region.end { - let offset = phys_start - rgn.guest_region.start; - let host_base = HostGuestMemoryRegion::to_addr(rgn.host_region.start) + offset; - let host_end = host_base + mapping.len as usize; - let flags = rgn.flags; - - if try_coalesce_region(regions, virt_base, virt_end, host_base, flags) { - continue; - } - - regions.push(CrashDumpRegion { - guest_region: virt_base..virt_end, - host_region: host_base..host_end, - flags, - region_type: rgn.region_type, - }); - } +// It would be nice to have a simple type alias +// `SnapshotSharedMemory` that abstracts over the +// fact that the snapshot shared memory is `ReadonlySharedMemory` +// normally, but there is (temporary) support for writable +// `GuestSharedMemory` with `#[cfg(feature = +// "nanvix-unstable")]`. Unfortunately, rustc gets annoyed about an +// unused type parameter, unless one goes to a little bit of effort to +// trick it... +mod unused_hack { + #[cfg(not(unshared_snapshot_mem))] + use crate::mem::shared_mem::ReadonlySharedMemory; + use crate::mem::shared_mem::SharedMemory; + pub trait SnapshotSharedMemoryT { + type T; + } + pub struct SnapshotSharedMemory_; + impl SnapshotSharedMemoryT for SnapshotSharedMemory_ { + #[cfg(not(unshared_snapshot_mem))] + type T = ReadonlySharedMemory; + #[cfg(unshared_snapshot_mem)] + type T = S; + } + pub type SnapshotSharedMemory = ::T; +} +impl ReadonlySharedMemory { + pub(crate) fn to_mgr_snapshot_mem( + &self, + ) -> Result> { + #[cfg(not(unshared_snapshot_mem))] + let ret = self.clone(); + #[cfg(unshared_snapshot_mem)] + let ret = self.copy_to_writable()?; + Ok(ret) } } - +pub(crate) use unused_hack::SnapshotSharedMemory; /// A struct that is responsible for laying out and managing the memory /// for a given `Sandbox`. #[derive(Clone)] -pub(crate) struct SandboxMemoryManager { +pub(crate) struct SandboxMemoryManager { /// Shared memory for the Sandbox - pub(crate) shared_mem: S, + pub(crate) shared_mem: SnapshotSharedMemory, /// Scratch memory for the Sandbox pub(crate) scratch_mem: S, /// The memory layout of the underlying shared memory @@ -242,7 +244,7 @@ where #[instrument(skip_all, parent = Span::current(), level= "Trace")] pub(crate) fn new( layout: SandboxMemoryLayout, - shared_mem: S, + shared_mem: SnapshotSharedMemory, scratch_mem: S, entrypoint: NextAction, ) -> Self { @@ -261,12 +263,6 @@ where &mut self.abort_buffer } - /// Get `SharedMemory` in `self` as a mutable reference - #[cfg(test)] - pub(crate) fn get_shared_mem_mut(&mut self) -> &mut S { - &mut self.shared_mem - } - /// Create a snapshot with the given mapped regions pub(crate) fn snapshot( &mut self, @@ -295,8 +291,7 @@ where impl SandboxMemoryManager { pub(crate) fn from_snapshot(s: &Snapshot) -> Result { let layout = *s.layout(); - let mut shared_mem = ExclusiveSharedMemory::new(s.mem_size())?; - shared_mem.copy_from_slice(s.memory(), 0)?; + let shared_mem = s.memory().to_mgr_snapshot_mem()?; let scratch_mem = ExclusiveSharedMemory::new(s.layout().get_scratch_size())?; let entrypoint = s.entrypoint(); Ok(Self::new(layout, shared_mem, scratch_mem, entrypoint)) @@ -489,16 +484,25 @@ impl SandboxMemoryManager { pub(crate) fn restore_snapshot( &mut self, snapshot: &Snapshot, - ) -> Result<(Option, Option)> { - let gsnapshot = if self.shared_mem.mem_size() == snapshot.mem_size() { + ) -> Result<( + Option>, + Option, + )> { + let gsnapshot = if *snapshot.memory() == self.shared_mem { + // If the snapshot memory is already the correct memory, + // which is readonly, don't bother with restoring it, + // since its contents must be the same. Note that in the + // #[cfg(unshared_snapshot_mem)] case, this condition will + // never be true, since even immediately after a restore, + // self.shared_mem is a (writable) copy, not the original + // shared_mem. None } else { - let new_snapshot_mem = ExclusiveSharedMemory::new(snapshot.mem_size())?; + let new_snapshot_mem = snapshot.memory().to_mgr_snapshot_mem()?; let (hsnapshot, gsnapshot) = new_snapshot_mem.build(); self.shared_mem = hsnapshot; Some(gsnapshot) }; - self.shared_mem.restore_from_snapshot(snapshot)?; let new_scratch_size = snapshot.layout().get_scratch_size(); let gscratch = if new_scratch_size == self.scratch_mem.mem_size() { self.scratch_mem.zero()?; @@ -549,13 +553,21 @@ impl SandboxMemoryManager { // Copy the page tables into the scratch region let snapshot_pt_end = self.shared_mem.mem_size(); - let snapshot_pt_start = snapshot_pt_end - self.layout.get_pt_size(); - self.shared_mem.with_exclusivity(|snap| { - self.scratch_mem.with_exclusivity(|scratch| { - let bytes = &snap.as_slice()[snapshot_pt_start..snapshot_pt_end]; - scratch.copy_from_slice(bytes, self.layout.get_pt_base_scratch_offset()) - }) - })???; + let snapshot_pt_size = self.layout.get_pt_size(); + let snapshot_pt_start = snapshot_pt_end - snapshot_pt_size; + self.scratch_mem.with_exclusivity(|scratch| { + #[cfg(not(unshared_snapshot_mem))] + let bytes = &self.shared_mem.as_slice()[snapshot_pt_start..snapshot_pt_end]; + #[cfg(unshared_snapshot_mem)] + let bytes = { + let mut bytes = vec![0u8; snapshot_pt_size]; + self.shared_mem + .copy_to_slice(&mut bytes, snapshot_pt_start)?; + bytes + }; + #[allow(clippy::needless_borrow)] + scratch.copy_from_slice(&bytes, self.layout.get_pt_base_scratch_offset()) + })??; Ok(()) } @@ -570,24 +582,21 @@ impl SandboxMemoryManager { root_pt: u64, mmap_regions: &[MemoryRegion], ) -> Result> { - use crate::sandbox::snapshot::{SharedMemoryPageTableBuffer, access_gpa}; + use crate::sandbox::snapshot::SharedMemoryPageTableBuffer; - let scratch_size = self.scratch_mem.mem_size(); let len = hyperlight_common::layout::MAX_GVA; - let regions = self.shared_mem.with_exclusivity(|snapshot| { - self.scratch_mem.with_exclusivity(|scratch| { + let regions = self.shared_mem.with_contents(|snapshot| { + self.scratch_mem.with_contents(|scratch| { let pt_buf = - SharedMemoryPageTableBuffer::new(snapshot, scratch, scratch_size, root_pt); + SharedMemoryPageTableBuffer::new(snapshot, scratch, self.layout, root_pt); let mappings: Vec<_> = unsafe { hyperlight_common::vmem::virt_to_phys(&pt_buf, 0, len as u64) } .collect(); if mappings.is_empty() { - return Err(new_error!( - "No page table mappings found (len {len})", - )); + return Err(new_error!("No page table mappings found (len {len})",)); } let mut regions: Vec = Vec::new(); @@ -595,22 +604,13 @@ impl SandboxMemoryManager { let virt_base = mapping.virt_base as usize; let virt_end = (mapping.virt_base + mapping.len) as usize; - if let Some((mem, offset)) = - access_gpa(snapshot, scratch, scratch_size, mapping.phys_base) + if let Some(resolved) = self.layout.resolve_gpa(mapping.phys_base, mmap_regions) { let (flags, region_type) = mapping_kind_to_flags(&mapping.kind); - - if offset >= mem.mem_size() { - tracing::error!( - "Mapping for GPA {:#x} offset {:#x} out of bounds (size {:#x}), skipping", - mapping.phys_base, offset, mem.mem_size(), - ); - continue; - } - - let host_base = mem.base_addr() + offset; - let host_len = - (mapping.len as usize).min(mem.mem_size().saturating_sub(offset)); + let resolved = resolved.with_memories(snapshot, scratch); + let contents = resolved.as_ref(); + let host_base = contents.as_ptr() as usize; + let host_len = (mapping.len as usize).min(contents.len()); if try_coalesce_region(&mut regions, virt_base, virt_end, host_base, flags) { @@ -623,11 +623,6 @@ impl SandboxMemoryManager { flags, region_type, }); - } else { - // GPA not in snapshot/scratch — check dynamic mmap regions - resolve_from_mmap_regions( - &mut regions, mapping, virt_base, virt_end, mmap_regions, - ); } } @@ -649,6 +644,8 @@ impl SandboxMemoryManager { _root_pt: u64, mmap_regions: &[MemoryRegion], ) -> Result> { + use crate::mem::memory_region::HostGuestMemoryRegion; + let snapshot_base = SandboxMemoryLayout::BASE_ADDRESS; let snapshot_size = self.shared_mem.mem_size(); let snapshot_host = self.shared_mem.base_addr(); @@ -709,11 +706,9 @@ impl SandboxMemoryManager { use crate::sandbox::snapshot::{SharedMemoryPageTableBuffer, access_gpa}; - let scratch_size = self.scratch_mem.mem_size(); - - self.shared_mem.with_exclusivity(|snap| { - self.scratch_mem.with_exclusivity(|scratch| { - let pt_buf = SharedMemoryPageTableBuffer::new(snap, scratch, scratch_size, root_pt); + self.shared_mem.with_contents(|snap| { + self.scratch_mem.with_contents(|scratch| { + let pt_buf = SharedMemoryPageTableBuffer::new(snap, scratch, self.layout, root_pt); // Walk page tables to get all mappings that cover the GVA range let mappings: Vec<_> = unsafe { @@ -753,7 +748,7 @@ impl SandboxMemoryManager { // Translate the GPA to host memory let gpa = mapping.phys_base + page_offset as u64; - let (mem, offset) = access_gpa(snap, scratch, scratch_size, gpa) + let (mem, offset) = access_gpa(snap, scratch, self.layout, gpa) .ok_or_else(|| { new_error!( "Failed to resolve GPA {:#x} to host memory (GVA {:#x})", @@ -763,7 +758,6 @@ impl SandboxMemoryManager { })?; let slice = mem - .as_slice() .get(offset..offset + bytes_to_copy) .ok_or_else(|| { new_error!( diff --git a/src/hyperlight_host/src/mem/shared_mem.rs b/src/hyperlight_host/src/mem/shared_mem.rs index 5c4a1a63f..527322cc9 100644 --- a/src/hyperlight_host/src/mem/shared_mem.rs +++ b/src/hyperlight_host/src/mem/shared_mem.rs @@ -39,10 +39,8 @@ use windows::core::PCSTR; use super::memory_region::{ HostGuestMemoryRegion, MemoryRegion, MemoryRegionFlags, MemoryRegionKind, MemoryRegionType, }; -use crate::HyperlightError::SnapshotSizeMismatch; #[cfg(target_os = "windows")] use crate::HyperlightError::WindowsAPIError; -use crate::sandbox::snapshot::Snapshot; use crate::{HyperlightError, Result, log_then_return, new_error}; /// Makes sure that the given `offset` and `size` are within the bounds of the memory with size `mem_size`. @@ -680,6 +678,28 @@ impl ExclusiveSharedMemory { } } +fn mapping_at( + s: &impl SharedMemory, + gpa: u64, + region_type: MemoryRegionType, + flags: MemoryRegionFlags, +) -> MemoryRegion { + let guest_base = gpa as usize; + + #[cfg(not(windows))] + let host_base = s.base_addr(); + #[cfg(windows)] + let host_base = s.host_region_base(); + let host_end = ::add(host_base, s.mem_size()); + + MemoryRegion { + guest_region: guest_base..(guest_base + s.mem_size()), + host_region: host_base..host_end, + region_type, + flags, + } +} + impl GuestSharedMemory { /// Create a [`super::memory_region::MemoryRegion`] structure /// suitable for mapping this region into a VM @@ -692,46 +712,24 @@ impl GuestSharedMemory { MemoryRegionType::Scratch => { MemoryRegionFlags::READ | MemoryRegionFlags::WRITE | MemoryRegionFlags::EXECUTE } - // Without nanvix-unstable (default), the snapshot is read-only - // because guest page tables provide CoW semantics for writable - // pages. With nanvix-unstable there are no guest page tables, - // so the snapshot must be writable — otherwise writes (including - // the CPU setting the "Accessed" bit in GDT descriptors during - // segment loads) cause EPT violations that KVM retries forever. + #[cfg(unshared_snapshot_mem)] MemoryRegionType::Snapshot => { - #[cfg(not(feature = "nanvix-unstable"))] - { - MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE - } - #[cfg(feature = "nanvix-unstable")] - { - MemoryRegionFlags::READ | MemoryRegionFlags::WRITE | MemoryRegionFlags::EXECUTE - } + MemoryRegionFlags::READ | MemoryRegionFlags::WRITE | MemoryRegionFlags::EXECUTE } #[allow(clippy::panic)] - // In the future, all the host side knowledge about memory - // region types should collapse down to Snapshot vs - // Scratch, at which time this panicking case will be - // unnecessary. For now, we will panic if one of the - // legacy regions ends up in this function, which very - // much should be impossible (since the only callers of it - // directly pass a literal new-style region type). + // This will not ever actually panic: the only places this + // is called are HyperlightVm::update_snapshot_mapping and + // HyperlightVm::update_scratch_mapping. The latter + // statically uses the Scratch region type, and the former + // does not use this at all when the unshared_snapshot_mem + // feature is not set, since in that case the scratch + // mapping type is ReadonlySharedMemory, not + // GuestSharedMemory. _ => panic!( "GuestSharedMemory::mapping_at should only be used for Scratch or Snapshot regions" ), }; - let guest_base = guest_base as usize; - #[cfg(not(windows))] - let host_base = self.base_addr(); - #[cfg(windows)] - let host_base = self.host_region_base(); - let host_end = ::add(host_base, self.mem_size()); - MemoryRegion { - guest_region: guest_base..(guest_base + self.mem_size()), - host_region: host_base..host_end, - region_type, - flags, - } + mapping_at(self, guest_base, region_type, flags) } } @@ -800,12 +798,13 @@ pub trait SharedMemory { f: F, ) -> Result; - /// Restore a SharedMemory from a snapshot with matching size - fn restore_from_snapshot(&mut self, snapshot: &Snapshot) -> Result<()> { - if snapshot.memory().len() != self.mem_size() { - return Err(SnapshotSizeMismatch(self.mem_size(), snapshot.mem_size())); - } - self.with_exclusivity(|e| e.copy_from_slice(snapshot.memory(), 0))? + /// Run some code that is allowed to access the contents of the + /// SharedMemory as if it is a normal slice. By default, this is + /// implemented via [`SharedMemory::with_exclusivity`], which is + /// the correct implementation for a memory that can be mutated, + /// but a [`ReadonlySharedMemory`], can support this. + fn with_contents T>(&mut self, f: F) -> Result { + self.with_exclusivity(|m| f(m.as_slice())) } /// Zero a shared memory region @@ -1990,3 +1989,97 @@ mod tests { } } } + +/// A ReadonlySharedMemory is a different kind of shared memory, +/// separate from the exclusive/host/guest lifecycle, used to +/// represent read-only mappings of snapshot pages into the guest +/// efficiently. +#[derive(Clone, Debug)] +pub struct ReadonlySharedMemory { + region: Arc, +} +// Safety: HostMapping is only non-Send/Sync (causing +// ReadonlySharedMemory to not be automatically Send/Sync) because raw +// pointers are not ("as a lint", as the Rust docs say). We don't want +// to mark HostMapping Send/Sync immediately, because that could +// socially imply that it's "safe" to use unsafe accesses from +// multiple threads at once in more cases, including ones that don't +// actually ensure immutability/synchronisation. Since +// ReadonlySharedMemory can only be accessed by reading, and reading +// concurrently from multiple threads is not racy, +// ReadonlySharedMemory can be Send and Sync. +unsafe impl Send for ReadonlySharedMemory {} +unsafe impl Sync for ReadonlySharedMemory {} + +impl ReadonlySharedMemory { + pub(crate) fn from_bytes(contents: &[u8]) -> Result { + let mut anon = ExclusiveSharedMemory::new(contents.len())?; + anon.copy_from_slice(contents, 0)?; + Ok(ReadonlySharedMemory { + region: anon.region, + }) + } + + pub(crate) fn as_slice(&self) -> &[u8] { + unsafe { std::slice::from_raw_parts(self.base_ptr(), self.mem_size()) } + } + + #[cfg(unshared_snapshot_mem)] + pub(crate) fn copy_to_writable(&self) -> Result { + let mut writable = ExclusiveSharedMemory::new(self.mem_size())?; + writable.copy_from_slice(self.as_slice(), 0)?; + Ok(writable) + } + + #[cfg(not(unshared_snapshot_mem))] + pub(crate) fn build(self) -> (Self, Self) { + (self.clone(), self) + } + + #[cfg(not(unshared_snapshot_mem))] + pub(crate) fn mapping_at( + &self, + guest_base: u64, + region_type: MemoryRegionType, + ) -> MemoryRegion { + #[allow(clippy::panic)] + // This will not ever actually panic: the only place this is + // called is HyperlightVm::update_snapshot_mapping, which + // always calls it with the Snapshot region type. + if region_type != MemoryRegionType::Snapshot { + panic!("ReadonlySharedMemory::mapping_at should only be used for Snapshot regions"); + } + mapping_at( + self, + guest_base, + region_type, + MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, + ) + } +} + +impl SharedMemory for ReadonlySharedMemory { + fn region(&self) -> &HostMapping { + &self.region + } + // There's no way to get exclusive (and therefore writable) access + // to a ReadonlySharedMemory. + fn with_exclusivity T>( + &mut self, + _: F, + ) -> Result { + Err(new_error!( + "Cannot take exclusive access to a ReadonlySharedMemory" + )) + } + // However, just access to the contents as a slice is doable + fn with_contents T>(&mut self, f: F) -> Result { + Ok(f(self.as_slice())) + } +} + +impl PartialEq for ReadonlySharedMemory { + fn eq(&self, other: &S) -> bool { + self.raw_ptr() == other.raw_ptr() + } +} diff --git a/src/hyperlight_host/src/sandbox/outb.rs b/src/hyperlight_host/src/sandbox/outb.rs index 868b57e19..9704a1fe3 100644 --- a/src/hyperlight_host/src/sandbox/outb.rs +++ b/src/hyperlight_host/src/sandbox/outb.rs @@ -245,9 +245,7 @@ mod tests { use super::outb_log; use crate::GuestBinary; - use crate::mem::layout::SandboxMemoryLayout; use crate::mem::mgr::SandboxMemoryManager; - use crate::mem::shared_mem::SharedMemory; use crate::sandbox::SandboxConfiguration; use crate::sandbox::outb::GuestLogData; use crate::testing::log_values::test_value_as_str; diff --git a/src/hyperlight_host/src/sandbox/snapshot.rs b/src/hyperlight_host/src/sandbox/snapshot.rs index 24912be8d..dbd3be47a 100644 --- a/src/hyperlight_host/src/sandbox/snapshot.rs +++ b/src/hyperlight_host/src/sandbox/snapshot.rs @@ -26,8 +26,8 @@ use crate::hypervisor::regs::CommonSpecialRegisters; use crate::mem::exe::LoadInfo; use crate::mem::layout::SandboxMemoryLayout; use crate::mem::memory_region::MemoryRegion; -use crate::mem::mgr::GuestPageTableBuffer; -use crate::mem::shared_mem::{ExclusiveSharedMemory, SharedMemory}; +use crate::mem::mgr::{GuestPageTableBuffer, SnapshotSharedMemory}; +use crate::mem::shared_mem::{ReadonlySharedMemory, SharedMemory}; use crate::sandbox::SandboxConfiguration; use crate::sandbox::uninitialized::{GuestBinary, GuestEnvironment}; @@ -70,7 +70,7 @@ pub struct Snapshot { /// configuration id will share the same layout layout: crate::mem::layout::SandboxMemoryLayout, /// Memory of the sandbox at the time this snapshot was taken - memory: Vec, + memory: ReadonlySharedMemory, /// The memory regions that were mapped when this snapshot was /// taken (excluding initial sandbox regions) regions: Vec, @@ -175,40 +175,32 @@ fn hash(memory: &[u8], regions: &[MemoryRegion]) -> Result<[u8; 32]> { } pub(crate) fn access_gpa<'a>( - snap: &'a ExclusiveSharedMemory, - scratch: &'a ExclusiveSharedMemory, - scratch_size: usize, + snap: &'a [u8], + scratch: &'a [u8], + layout: SandboxMemoryLayout, gpa: u64, -) -> Option<(&'a ExclusiveSharedMemory, usize)> { - let scratch_base = scratch_base_gpa(scratch_size); - if gpa >= scratch_base && gpa < scratch_base + scratch_size as u64 { - Some((scratch, (gpa - scratch_base) as usize)) - } else if gpa >= SandboxMemoryLayout::BASE_ADDRESS as u64 - && gpa < SandboxMemoryLayout::BASE_ADDRESS as u64 + snap.mem_size() as u64 - { - Some((snap, gpa as usize - SandboxMemoryLayout::BASE_ADDRESS)) - } else { - None - } +) -> Option<(&'a [u8], usize)> { + let resolved = layout.resolve_gpa(gpa, &[])?.with_memories(snap, scratch); + Some((resolved.base.as_ref(), resolved.offset)) } pub(crate) struct SharedMemoryPageTableBuffer<'a> { - snap: &'a ExclusiveSharedMemory, - scratch: &'a ExclusiveSharedMemory, - scratch_size: usize, + snap: &'a [u8], + scratch: &'a [u8], + layout: SandboxMemoryLayout, root: u64, } impl<'a> SharedMemoryPageTableBuffer<'a> { pub(crate) fn new( - snap: &'a ExclusiveSharedMemory, - scratch: &'a ExclusiveSharedMemory, - scratch_size: usize, + snap: &'a [u8], + scratch: &'a [u8], + layout: SandboxMemoryLayout, root: u64, ) -> Self { Self { snap, scratch, - scratch_size, + layout, root, } } @@ -219,8 +211,8 @@ impl<'a> hyperlight_common::vmem::TableReadOps for SharedMemoryPageTableBuffer<' addr + offset } unsafe fn read_entry(&self, addr: u64) -> u64 { - let memoff = access_gpa(self.snap, self.scratch, self.scratch_size, addr); - let Some(pte_bytes) = memoff.and_then(|(mem, off)| mem.as_slice().get(off..off + 8)) else { + let memoff = access_gpa(self.snap, self.scratch, self.layout, addr); + let Some(pte_bytes) = memoff.and_then(|(mem, off)| mem.get(off..off + 8)) else { // Attacker-controlled data pointed out-of-bounds. We'll // default to returning 0 in this case, which, for most // architectures (including x86-64 and arm64, the ones we @@ -249,19 +241,19 @@ impl<'a> core::convert::AsRef> for SharedMemoryP } } fn filtered_mappings<'a>( - snap: &'a ExclusiveSharedMemory, - scratch: &'a ExclusiveSharedMemory, + snap: &'a [u8], + scratch: &'a [u8], regions: &[MemoryRegion], - scratch_size: usize, + layout: SandboxMemoryLayout, root_pt: u64, ) -> Vec<(Mapping, &'a [u8])> { - let op = SharedMemoryPageTableBuffer::new(snap, scratch, scratch_size, root_pt); + let op = SharedMemoryPageTableBuffer::new(snap, scratch, layout, root_pt); unsafe { hyperlight_common::vmem::virt_to_phys(&op, 0, hyperlight_common::layout::MAX_GVA as u64) } .filter_map(move |mapping| { // the scratch map doesn't count - if mapping.virt_base >= scratch_base_gva(scratch_size) { + if mapping.virt_base >= scratch_base_gva(layout.get_scratch_size()) { return None; } // neither does the mapping of the snapshot's own page tables @@ -272,8 +264,7 @@ fn filtered_mappings<'a>( return None; } // todo: is it useful to warn if we can't resolve this? - let contents = - unsafe { guest_page(snap, scratch, regions, scratch_size, mapping.phys_base) }?; + let contents = unsafe { guest_page(snap, scratch, regions, layout, mapping.phys_base) }?; Some((mapping, contents)) }) .collect() @@ -287,32 +278,19 @@ fn filtered_mappings<'a>( /// alive and must not be mutated by any other thread: references to /// these regions may be created and live for `'a`. unsafe fn guest_page<'a>( - snap: &'a ExclusiveSharedMemory, - scratch: &'a ExclusiveSharedMemory, + snap: &'a [u8], + scratch: &'a [u8], regions: &[MemoryRegion], - scratch_size: usize, + layout: SandboxMemoryLayout, gpa: u64, ) -> Option<&'a [u8]> { - if !gpa.is_multiple_of(PAGE_SIZE as u64) { + let resolved = layout + .resolve_gpa(gpa, regions)? + .with_memories(snap, scratch); + if resolved.as_ref().len() < PAGE_SIZE { return None; } - let gpa_u = gpa as usize; - for rgn in regions { - if gpa_u >= rgn.guest_region.start && gpa_u + PAGE_SIZE <= rgn.guest_region.end { - let off = gpa_u - rgn.guest_region.start; - #[allow(clippy::useless_conversion)] - let host_region_base: usize = rgn.host_region.start.into(); - return Some(unsafe { - std::slice::from_raw_parts((host_region_base + off) as *const u8, PAGE_SIZE) - }); - } - } - let (mem, off) = access_gpa(snap, scratch, scratch_size, gpa)?; - if off + PAGE_SIZE <= mem.as_slice().len() { - Some(&mem.as_slice()[off..off + PAGE_SIZE]) - } else { - None - } + Some(&resolved.as_ref()[..PAGE_SIZE]) } fn map_specials(pt_buf: &GuestPageTableBuffer, scratch_size: usize) { @@ -437,7 +415,7 @@ impl Snapshot { Ok(Self { sandbox_id: SANDBOX_CONFIGURATION_COUNTER.fetch_add(1, Ordering::Relaxed), - memory, + memory: ReadonlySharedMemory::from_bytes(&memory)?, layout, regions: extra_regions, load_info, @@ -458,7 +436,7 @@ impl Snapshot { /// instance of `Self` with the snapshot stored therein. #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")] pub(crate) fn new( - shared_mem: &mut S, + shared_mem: &mut SnapshotSharedMemory, scratch_mem: &mut S, sandbox_id: u64, mut layout: SandboxMemoryLayout, @@ -469,13 +447,11 @@ impl Snapshot { sregs: CommonSpecialRegisters, entrypoint: NextAction, ) -> Result { - let memory = shared_mem.with_exclusivity(|snap_e| { - scratch_mem.with_exclusivity(|scratch_e| { - let scratch_size = layout.get_scratch_size(); - + let memory = shared_mem.with_contents(|snap_c| { + scratch_mem.with_contents(|scratch_c| { // Pass 1: count how many pages need to live let live_pages = - filtered_mappings(snap_e, scratch_e, ®ions, scratch_size, root_pt_gpa); + filtered_mappings(snap_c, scratch_c, ®ions, layout, root_pt_gpa); // Pass 2: copy them, and map them // TODO: Look for opportunities to hugepage map @@ -513,6 +489,7 @@ impl Snapshot { Ok::, crate::HyperlightError>(snapshot_memory) }) })???; + layout.set_snapshot_size(memory.len()); // We do not need the original regions anymore, as any uses of // them in the guest have been incorporated into the snapshot @@ -523,7 +500,7 @@ impl Snapshot { Ok(Self { sandbox_id, layout, - memory, + memory: ReadonlySharedMemory::from_bytes(&memory)?, regions, load_info, hash, @@ -543,15 +520,9 @@ impl Snapshot { &self.regions } - /// Return the size of the snapshot in bytes. - #[instrument(skip_all, parent = Span::current(), level= "Trace")] - pub(crate) fn mem_size(&self) -> usize { - self.memory.len() - } - /// Return the main memory contents of the snapshot #[instrument(skip_all, parent = Span::current(), level= "Trace")] - pub(crate) fn memory(&self) -> &[u8] { + pub(crate) fn memory(&self) -> &ReadonlySharedMemory { &self.memory } @@ -599,18 +570,19 @@ mod tests { use crate::hypervisor::regs::CommonSpecialRegisters; use crate::mem::exe::LoadInfo; use crate::mem::layout::SandboxMemoryLayout; - use crate::mem::mgr::{GuestPageTableBuffer, SandboxMemoryManager}; - use crate::mem::shared_mem::{ExclusiveSharedMemory, HostSharedMemory, SharedMemory}; + use crate::mem::mgr::{GuestPageTableBuffer, SandboxMemoryManager, SnapshotSharedMemory}; + use crate::mem::shared_mem::{ + ExclusiveSharedMemory, HostSharedMemory, ReadonlySharedMemory, SharedMemory, + }; fn default_sregs() -> CommonSpecialRegisters { CommonSpecialRegisters::default() } - fn make_simple_pt_mems() -> (SandboxMemoryManager, u64) { - let cfg = crate::sandbox::SandboxConfiguration::default(); - let scratch_mem = ExclusiveSharedMemory::new(cfg.get_scratch_size()).unwrap(); - let pt_base = PAGE_SIZE + SandboxMemoryLayout::BASE_ADDRESS; - let pt_buf = GuestPageTableBuffer::new(pt_base); + const SIMPLE_PT_BASE: usize = PAGE_SIZE + SandboxMemoryLayout::BASE_ADDRESS; + + fn make_simple_pt_mem(contents: &[u8]) -> SnapshotSharedMemory { + let pt_buf = GuestPageTableBuffer::new(SIMPLE_PT_BASE); let mapping = Mapping { phys_base: SandboxMemoryLayout::BASE_ADDRESS as u64, virt_base: SandboxMemoryLayout::BASE_ADDRESS as u64, @@ -625,86 +597,36 @@ mod tests { super::map_specials(&pt_buf, PAGE_SIZE); let pt_bytes = pt_buf.into_bytes(); - let mut snapshot_mem = ExclusiveSharedMemory::new(PAGE_SIZE + pt_bytes.len()).unwrap(); + let mut snapshot_mem = vec![0u8; PAGE_SIZE + pt_bytes.len()]; + snapshot_mem[0..PAGE_SIZE].copy_from_slice(contents); + snapshot_mem[PAGE_SIZE..].copy_from_slice(&pt_bytes); + ReadonlySharedMemory::from_bytes(&snapshot_mem) + .unwrap() + .to_mgr_snapshot_mem() + .unwrap() + } - snapshot_mem.copy_from_slice(&pt_bytes, PAGE_SIZE).unwrap(); + fn make_simple_pt_mgr() -> (SandboxMemoryManager, u64) { + let cfg = crate::sandbox::SandboxConfiguration::default(); + let scratch_mem = ExclusiveSharedMemory::new(cfg.get_scratch_size()).unwrap(); let mgr = SandboxMemoryManager::new( SandboxMemoryLayout::new(cfg, 4096, 0x3000, None).unwrap(), - snapshot_mem, + make_simple_pt_mem(&[0u8; PAGE_SIZE]), scratch_mem, super::NextAction::None, ); let (mgr, _) = mgr.build().unwrap(); - (mgr, pt_base as u64) - } - - #[test] - fn restore() { - // Simplified version of the original test - let data1 = vec![b'a'; PAGE_SIZE]; - let data2 = vec![b'b'; PAGE_SIZE]; - - let (mut mgr, pt_base) = make_simple_pt_mems(); - mgr.shared_mem.copy_from_slice(&data1, 0).unwrap(); - - // Take snapshot of data1 - let snapshot = super::Snapshot::new( - &mut mgr.shared_mem, - &mut mgr.scratch_mem, - 0, - mgr.layout, - LoadInfo::dummy(), - Vec::new(), - pt_base, - 0, - default_sregs(), - super::NextAction::None, - ) - .unwrap(); - - // Modify memory to data2 - mgr.shared_mem.copy_from_slice(&data2, 0).unwrap(); - mgr.shared_mem - .with_exclusivity(|e| assert_eq!(&e.as_slice()[0..data2.len()], &data2[..])) - .unwrap(); - - // Restore should bring back data1 - let _ = mgr.restore_snapshot(&snapshot).unwrap(); - mgr.shared_mem - .with_exclusivity(|e| assert_eq!(&e.as_slice()[0..data1.len()], &data1[..])) - .unwrap(); - } - - #[test] - fn snapshot_mem_size() { - let (mut mgr, pt_base) = make_simple_pt_mems(); - let size = mgr.shared_mem.mem_size(); - - let snapshot = super::Snapshot::new( - &mut mgr.shared_mem, - &mut mgr.scratch_mem, - 0, - mgr.layout, - LoadInfo::dummy(), - Vec::new(), - pt_base, - 0, - default_sregs(), - super::NextAction::None, - ) - .unwrap(); - assert_eq!(snapshot.mem_size(), size); + (mgr, SIMPLE_PT_BASE as u64) } #[test] fn multiple_snapshots_independent() { - let (mut mgr, pt_base) = make_simple_pt_mems(); + let (mut mgr, pt_base) = make_simple_pt_mgr(); // Create first snapshot with pattern A let pattern_a = vec![0xAA; PAGE_SIZE]; - mgr.shared_mem.copy_from_slice(&pattern_a, 0).unwrap(); let snapshot_a = super::Snapshot::new( - &mut mgr.shared_mem, + &mut make_simple_pt_mem(&pattern_a).build().0, &mut mgr.scratch_mem, 1, mgr.layout, @@ -719,9 +641,8 @@ mod tests { // Create second snapshot with pattern B let pattern_b = vec![0xBB; PAGE_SIZE]; - mgr.shared_mem.copy_from_slice(&pattern_b, 0).unwrap(); let snapshot_b = super::Snapshot::new( - &mut mgr.shared_mem, + &mut make_simple_pt_mem(&pattern_b).build().0, &mut mgr.scratch_mem, 2, mgr.layout, @@ -734,19 +655,16 @@ mod tests { ) .unwrap(); - // Clear memory - mgr.shared_mem.copy_from_slice(&[0; PAGE_SIZE], 0).unwrap(); - // Restore snapshot A mgr.restore_snapshot(&snapshot_a).unwrap(); mgr.shared_mem - .with_exclusivity(|e| assert_eq!(&e.as_slice()[0..pattern_a.len()], &pattern_a[..])) + .with_contents(|contents| assert_eq!(&contents[0..pattern_a.len()], &pattern_a[..])) .unwrap(); // Restore snapshot B mgr.restore_snapshot(&snapshot_b).unwrap(); mgr.shared_mem - .with_exclusivity(|e| assert_eq!(&e.as_slice()[0..pattern_b.len()], &pattern_b[..])) + .with_contents(|contents| assert_eq!(&contents[0..pattern_b.len()], &pattern_b[..])) .unwrap(); } } diff --git a/src/hyperlight_host/src/sandbox/trace/mem_profile.rs b/src/hyperlight_host/src/sandbox/trace/mem_profile.rs index 54905b272..62cb281fa 100644 --- a/src/hyperlight_host/src/sandbox/trace/mem_profile.rs +++ b/src/hyperlight_host/src/sandbox/trace/mem_profile.rs @@ -20,6 +20,8 @@ use fallible_iterator::FallibleIterator; use framehop::Unwinder; use crate::hypervisor::regs::CommonRegisters; +#[cfg(not(unshared_snapshot_mem))] +use crate::mem::layout::ReadableSharedMemory; use crate::mem::layout::SandboxMemoryLayout; use crate::mem::mgr::SandboxMemoryManager; use crate::mem::shared_mem::HostSharedMemory; @@ -96,10 +98,15 @@ impl MemTraceInfo { mem_mgr: &SandboxMemoryManager, ) -> Result> { let mut read_stack = |addr| { + let mut buf: [u8; 8] = [0u8; 8]; mem_mgr .shared_mem - .read::((addr - SandboxMemoryLayout::BASE_ADDRESS as u64) as usize) - .map_err(|_| ()) + .copy_to_slice( + &mut buf, + (addr - SandboxMemoryLayout::BASE_ADDRESS as u64) as usize, + ) + .map_err(|_| ())?; + Ok(u64::from_ne_bytes(buf)) }; let mut cache = self .unwind_cache From 6a5064db5be61672120d0a17580edcdc90024ed4 Mon Sep 17 00:00:00 2001 From: Lucy Menon <168595099+syntactically@users.noreply.github.com> Date: Tue, 17 Mar 2026 03:40:30 +0000 Subject: [PATCH 4/5] snapshot: avoid duplicating pages mapped multiple times Previously, if a sandbox mapped one physical page to multiple virtual addresses, taking a snapshot would duplicate the page. This commit changes the logic around snapshots to keep track of the physical pages that have been encountered during the virtual memory traversal so far, and ensure that the same page is not copied multiple times. Note that this does not (yet) track the contents of pages to allow de-duplicating them---it merely ensures that two mappings of the same physical address do not result in needless duplication. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com> --- src/hyperlight_host/src/sandbox/snapshot.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/hyperlight_host/src/sandbox/snapshot.rs b/src/hyperlight_host/src/sandbox/snapshot.rs index dbd3be47a..3f62da0bf 100644 --- a/src/hyperlight_host/src/sandbox/snapshot.rs +++ b/src/hyperlight_host/src/sandbox/snapshot.rs @@ -447,6 +447,8 @@ impl Snapshot { sregs: CommonSpecialRegisters, entrypoint: NextAction, ) -> Result { + use std::collections::HashMap; + let mut phys_seen = HashMap::::new(); let memory = shared_mem.with_contents(|snap_c| { scratch_mem.with_contents(|scratch_c| { // Pass 1: count how many pages need to live @@ -458,9 +460,6 @@ impl Snapshot { let pt_buf = GuestPageTableBuffer::new(layout.get_pt_base_gpa() as usize); let mut snapshot_memory: Vec = Vec::new(); for (mapping, contents) in live_pages { - let new_offset = snapshot_memory.len(); - snapshot_memory.extend(contents); - let new_gpa = new_offset + SandboxMemoryLayout::BASE_ADDRESS; let kind = match mapping.kind { MappingKind::Cow(cm) => MappingKind::Cow(cm), MappingKind::Basic(bm) if bm.writable => MappingKind::Cow(CowMapping { @@ -473,8 +472,13 @@ impl Snapshot { executable: bm.executable, }), }; + let new_gpa = phys_seen.entry(mapping.phys_base).or_insert_with(|| { + let new_offset = snapshot_memory.len(); + snapshot_memory.extend(contents); + new_offset + SandboxMemoryLayout::BASE_ADDRESS + }); let mapping = Mapping { - phys_base: new_gpa as u64, + phys_base: *new_gpa as u64, virt_base: mapping.virt_base, len: PAGE_SIZE as u64, kind, From 85f3e193a27674e65f6346166f635084b21ce7fb Mon Sep 17 00:00:00 2001 From: Lucy Menon <168595099+syntactically@users.noreply.github.com> Date: Tue, 17 Mar 2026 03:44:48 +0000 Subject: [PATCH 5/5] Allow unmapping existing mapped ranges in the guest Previously, the page table manipulation interfaces exported for the use of guests did not permit unmapping an existing mapped range. This commit improves that situation, by adding an "unmapped" mapping type, which code can use to request that a remap operation make a region not-present. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com> --- src/hyperlight_common/src/arch/amd64/vmem.rs | 1 + src/hyperlight_common/src/vmem.rs | 1 + src/hyperlight_host/src/mem/layout.rs | 3 ++- src/hyperlight_host/src/mem/mgr.rs | 1 + src/hyperlight_host/src/sandbox/snapshot.rs | 1 + 5 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/hyperlight_common/src/arch/amd64/vmem.rs b/src/hyperlight_common/src/arch/amd64/vmem.rs index 618c56660..9505dda16 100644 --- a/src/hyperlight_common/src/arch/amd64/vmem.rs +++ b/src/hyperlight_common/src/arch/amd64/vmem.rs @@ -456,6 +456,7 @@ unsafe fn map_page< 0 | // R/W - Cow page is never writable PAGE_PRESENT // P - this entry is present } + MappingKind::Unmapped => 0, }; unsafe { write_entry_updating(op, r.update_parent, r.entry_ptr, pte); diff --git a/src/hyperlight_common/src/vmem.rs b/src/hyperlight_common/src/vmem.rs index c72a6b9af..96de9f334 100644 --- a/src/hyperlight_common/src/vmem.rs +++ b/src/hyperlight_common/src/vmem.rs @@ -196,6 +196,7 @@ pub struct CowMapping { #[derive(Debug, PartialEq, Clone, Copy)] pub enum MappingKind { + Unmapped, Basic(BasicMapping), Cow(CowMapping), /* TODO: What useful things other than basic mappings actually diff --git a/src/hyperlight_host/src/mem/layout.rs b/src/hyperlight_host/src/mem/layout.rs index 5f1e72ff1..94f21057f 100644 --- a/src/hyperlight_host/src/mem/layout.rs +++ b/src/hyperlight_host/src/mem/layout.rs @@ -140,6 +140,7 @@ impl<'a> ResolvedGpa<&'a [u8], &'a [u8]> { } } #[cfg(any(gdb, feature = "mem_profile"))] +#[allow(unused)] // may be unused when nanvix-unstable is also enabled pub(crate) trait ReadableSharedMemory { fn copy_to_slice(&self, slice: &mut [u8], offset: usize) -> Result<()>; } @@ -177,7 +178,7 @@ impl ReadableSharedMemory for T { } #[cfg(any(gdb, feature = "mem_profile"))] impl ResolvedGpa { - #[cfg(feature = "gdb")] + #[allow(unused)] // may be unused when nanvix-unstable is also enabled pub(crate) fn copy_to_slice(&self, slice: &mut [u8]) -> Result<()> { match &self.base { BaseGpaRegion::Snapshot(sn) => sn.copy_to_slice(slice, self.offset), diff --git a/src/hyperlight_host/src/mem/mgr.rs b/src/hyperlight_host/src/mem/mgr.rs index 8c67ed6e2..1a44b822e 100644 --- a/src/hyperlight_host/src/mem/mgr.rs +++ b/src/hyperlight_host/src/mem/mgr.rs @@ -67,6 +67,7 @@ fn mapping_kind_to_flags(kind: &MappingKind) -> (MemoryRegionFlags, MemoryRegion } (flags, MemoryRegionType::Scratch) } + MappingKind::Unmapped => (MemoryRegionFlags::empty(), MemoryRegionType::Snapshot), } } diff --git a/src/hyperlight_host/src/sandbox/snapshot.rs b/src/hyperlight_host/src/sandbox/snapshot.rs index 3f62da0bf..c5f0520a6 100644 --- a/src/hyperlight_host/src/sandbox/snapshot.rs +++ b/src/hyperlight_host/src/sandbox/snapshot.rs @@ -471,6 +471,7 @@ impl Snapshot { writable: false, executable: bm.executable, }), + MappingKind::Unmapped => continue, }; let new_gpa = phys_seen.entry(mapping.phys_base).or_insert_with(|| { let new_offset = snapshot_memory.len();