-
Notifications
You must be signed in to change notification settings - Fork 172
Validate Guest Address Ranges for Overlapping Regions in map_region #1464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -243,6 +243,8 @@ pub enum MapRegionError { | |||||||||||||||||||||||
| MapMemory(#[from] MapMemoryError), | ||||||||||||||||||||||||
| #[error("Region is not page-aligned (page size: {0:#x})")] | ||||||||||||||||||||||||
| NotPageAligned(usize), | ||||||||||||||||||||||||
| #[error("Region [{0:#x}..{1:#x}) overlaps existing region [{2:#x}..{3:#x})")] | ||||||||||||||||||||||||
| Overlapping(usize, usize, usize, usize), | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Errors that can occur when unmapping a memory region | ||||||||||||||||||||||||
|
|
@@ -420,6 +422,50 @@ impl HyperlightVm { | |||||||||||||||||||||||
| return Err(MapRegionError::NotPageAligned(self.page_size)); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let new_start = region.guest_region.start; | ||||||||||||||||||||||||
| let new_end = region.guest_region.end; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Check against existing dynamically mapped regions | ||||||||||||||||||||||||
| for (_, existing) in &self.mmap_regions { | ||||||||||||||||||||||||
| if new_start < existing.guest_region.end && new_end > existing.guest_region.start { | ||||||||||||||||||||||||
| return Err(MapRegionError::Overlapping( | ||||||||||||||||||||||||
| new_start, | ||||||||||||||||||||||||
| new_end, | ||||||||||||||||||||||||
| existing.guest_region.start, | ||||||||||||||||||||||||
| existing.guest_region.end, | ||||||||||||||||||||||||
| )); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+429
to
+438
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think w/ this we can now remove: hyperlight/src/hyperlight_host/src/sandbox/initialized_multi_use.rs Lines 645 to 655 in 29bf180
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Check against the snapshot region | ||||||||||||||||||||||||
| if let Some(ref snapshot) = self.snapshot_memory { | ||||||||||||||||||||||||
| let snap_start = crate::mem::layout::SandboxMemoryLayout::BASE_ADDRESS; | ||||||||||||||||||||||||
| #[cfg(not(unshared_snapshot_mem))] | ||||||||||||||||||||||||
| let snap_end = snap_start + snapshot.guest_mapped_size(); | ||||||||||||||||||||||||
| #[cfg(unshared_snapshot_mem)] | ||||||||||||||||||||||||
| let snap_end = snap_start + snapshot.mem_size(); | ||||||||||||||||||||||||
| if new_start < snap_end && new_end > snap_start { | ||||||||||||||||||||||||
| return Err(MapRegionError::Overlapping( | ||||||||||||||||||||||||
| new_start, new_end, snap_start, snap_end, | ||||||||||||||||||||||||
| )); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Check against the scratch region | ||||||||||||||||||||||||
| if let Some(ref scratch) = self.scratch_memory { | ||||||||||||||||||||||||
| let scratch_start = | ||||||||||||||||||||||||
| hyperlight_common::layout::scratch_base_gpa(scratch.mem_size()) as usize; | ||||||||||||||||||||||||
| let scratch_end = scratch_start + scratch.mem_size(); | ||||||||||||||||||||||||
| if new_start < scratch_end && new_end > scratch_start { | ||||||||||||||||||||||||
| return Err(MapRegionError::Overlapping( | ||||||||||||||||||||||||
| new_start, | ||||||||||||||||||||||||
| new_end, | ||||||||||||||||||||||||
| scratch_start, | ||||||||||||||||||||||||
| scratch_end, | ||||||||||||||||||||||||
| )); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Try to reuse a freed slot first, otherwise use next_slot | ||||||||||||||||||||||||
| let slot = if let Some(freed_slot) = self.freed_slots.pop() { | ||||||||||||||||||||||||
| freed_slot | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -568,9 +568,10 @@ impl MultiUseSandbox { | |
| // writes can be rolled back when necessary. | ||
| log_then_return!("TODO: Writable mappings not yet supported"); | ||
| } | ||
| // Reset snapshot since we are mutating the sandbox state | ||
| self.snapshot = None; | ||
|
|
||
| // Map first so overlaps are rejected before resetting the snapshot | ||
| unsafe { self.vm.map_region(rgn) }.map_err(HyperlightVmError::MapRegion)?; | ||
| self.snapshot = None; | ||
| self.mem_mgr.mapped_rgns += 1; | ||
| Ok(()) | ||
| } | ||
|
|
@@ -654,13 +655,12 @@ impl MultiUseSandbox { | |
| } | ||
| } | ||
|
|
||
| // Reset snapshot since we are mutating the sandbox state | ||
| self.snapshot = None; | ||
|
|
||
| unsafe { self.vm.map_region(®ion) } | ||
| .map_err(HyperlightVmError::MapRegion) | ||
| .map_err(crate::HyperlightError::HyperlightVmError)?; | ||
|
|
||
| self.snapshot = None; | ||
|
|
||
| let size = prepared.size as u64; | ||
|
|
||
| // Mark consumed immediately after map_region succeeds. | ||
|
|
@@ -2588,4 +2588,99 @@ mod tests { | |
| } | ||
| let _ = std::fs::remove_file(&path); | ||
| } | ||
|
|
||
| #[test] | ||
| fn map_region_rejects_overlapping_regions() { | ||
| 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 mem1 = allocate_guest_memory(); | ||
| let mem2 = allocate_guest_memory(); | ||
| let guest_base: usize = 0x200000000; | ||
| let region1 = region_for_memory(&mem1, guest_base, MemoryRegionFlags::READ); | ||
|
|
||
| // First mapping should succeed | ||
| unsafe { sbox.map_region(®ion1).unwrap() }; | ||
|
|
||
| // Exact same range should fail | ||
| let region2 = region_for_memory(&mem2, guest_base, MemoryRegionFlags::READ); | ||
| let err = unsafe { sbox.map_region(®ion2) }.unwrap_err(); | ||
| assert!( | ||
| format!("{err:?}").contains("Overlapping"), | ||
| "Expected Overlapping error, got: {err:?}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn map_region_rejects_partial_overlap() { | ||
| 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() | ||
| }; | ||
|
|
||
| // Use multi-page regions so partial overlap is geometrically possible | ||
| let mem1 = page_aligned_memory(&[0xAA; 8192]); // 2 pages | ||
| let mem2 = page_aligned_memory(&[0xBB; 8192]); // 2 pages | ||
| let guest_base: usize = 0x200000000; | ||
| let region1 = region_for_memory(&mem1, guest_base, MemoryRegionFlags::READ); | ||
|
|
||
| unsafe { sbox.map_region(®ion1).unwrap() }; | ||
|
|
||
| // region2 starts one page before region1, overlapping by one page | ||
| let overlap_base = guest_base - 0x1000; | ||
| let region2 = region_for_memory(&mem2, overlap_base, MemoryRegionFlags::READ); | ||
| let err = unsafe { sbox.map_region(®ion2) }.unwrap_err(); | ||
| assert!( | ||
| format!("{err:?}").contains("verlap"), | ||
| "Expected overlap error for partial overlap, got: {err:?}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn map_region_allows_adjacent_non_overlapping() { | ||
| 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 mem1 = allocate_guest_memory(); | ||
| let mem2 = allocate_guest_memory(); | ||
| let guest_base: usize = 0x200000000; | ||
| let region1 = region_for_memory(&mem1, guest_base, MemoryRegionFlags::READ); | ||
| let region_size = mem1.mem_size(); | ||
|
|
||
| unsafe { sbox.map_region(®ion1).unwrap() }; | ||
|
|
||
| // Adjacent region (starts right after the first one ends) should succeed | ||
| let adjacent_base = guest_base + region_size; | ||
| let region2 = region_for_memory(&mem2, adjacent_base, MemoryRegionFlags::READ); | ||
| unsafe { sbox.map_region(®ion2).unwrap() }; | ||
| } | ||
|
|
||
| #[test] | ||
| fn map_region_rejects_overlap_with_snapshot() { | ||
| 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() | ||
| }; | ||
|
|
||
| // Try to map at BASE_ADDRESS (0x1000) which overlaps the snapshot region | ||
| let mem = allocate_guest_memory(); | ||
| let region = region_for_memory( | ||
| &mem, | ||
| crate::mem::layout::SandboxMemoryLayout::BASE_ADDRESS, | ||
| MemoryRegionFlags::READ, | ||
| ); | ||
| let err = unsafe { sbox.map_region(®ion) }.unwrap_err(); | ||
| assert!( | ||
| format!("{err:?}").contains("Overlapping"), | ||
| "Expected Overlapping error for snapshot overlap, got: {err:?}" | ||
| ); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iiuc, we have tests covering overlaps on the mmap_regions as well as snapshots--yes? Would be nice to add a test checking for overlap on scratch too. |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe make these named fields so you don't have
usize, usize, usize, usize?