diff --git a/src/hyperlight_host/src/mem/memory_region.rs b/src/hyperlight_host/src/mem/memory_region.rs index b46426c3b..f4c38b168 100644 --- a/src/hyperlight_host/src/mem/memory_region.rs +++ b/src/hyperlight_host/src/mem/memory_region.rs @@ -324,9 +324,11 @@ impl From for kvm_bindings::kvm_userspace_memory_region { guest_phys_addr: region.guest_region.start as u64, memory_size: (region.guest_region.end - region.guest_region.start) as u64, userspace_addr: region.host_region.start as u64, - flags: match perm_flags { - MemoryRegionFlags::READ => KVM_MEM_READONLY, - _ => 0, // normal, RWX + flags: if perm_flags.contains(MemoryRegionFlags::WRITE) { + 0 // RWX + } else { + // Note: KVM_MEM_READONLY is executable + KVM_MEM_READONLY // RX }, } } diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index ada0ea174..a660e7c57 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -597,8 +597,12 @@ mod tests { let guest_base = 0x1_0000_0000; // Arbitrary guest base address unsafe { - sbox.map_region(®ion_for_memory(&map_mem, guest_base)) - .unwrap(); + sbox.map_region(®ion_for_memory( + &map_mem, + guest_base, + MemoryRegionFlags::READ, + )) + .unwrap(); } let _guard = map_mem.lock.try_read().unwrap(); @@ -612,6 +616,56 @@ mod tests { assert_eq!(actual, expected); } + // Makes sure MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE executable but not writable + #[cfg(target_os = "linux")] + #[test] + fn test_mmap_write_exec() { + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let expected = &[0x90, 0x90, 0x90, 0xC3]; // NOOP slide to RET + let map_mem = page_aligned_memory(expected); + let guest_base = 0x1_0000_0000; // Arbitrary guest base address + + unsafe { + sbox.map_region(®ion_for_memory( + &map_mem, + guest_base, + MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, + )) + .unwrap(); + } + + let _guard = map_mem.lock.try_read().unwrap(); + + // Execute should pass since memory is executable + let succeed = sbox + .call_guest_function_by_name::( + "ExecMappedBuffer", + (guest_base as u64, expected.len() as u64), + ) + .unwrap(); + assert!(succeed, "Expected execution of mapped buffer to succeed"); + + // write should fail because the memory is mapped as read-only + let err = sbox + .call_guest_function_by_name::( + "WriteMappedBuffer", + (guest_base as u64, expected.len() as u64), + ) + .unwrap_err(); + + match err { + HyperlightError::MemoryAccessViolation(addr, ..) if addr == guest_base as u64 => {} + _ => panic!("Expected MemoryAccessViolation error"), + }; + } + #[cfg(target_os = "linux")] fn page_aligned_memory(src: &[u8]) -> GuestSharedMemory { use hyperlight_common::mem::PAGE_SIZE_USIZE; @@ -627,13 +681,17 @@ mod tests { } #[cfg(target_os = "linux")] - fn region_for_memory(mem: &GuestSharedMemory, guest_base: usize) -> MemoryRegion { + fn region_for_memory( + mem: &GuestSharedMemory, + guest_base: usize, + flags: MemoryRegionFlags, + ) -> MemoryRegion { let ptr = mem.base_addr(); let len = mem.mem_size(); MemoryRegion { host_region: ptr..(ptr + len), guest_region: guest_base..(guest_base + len), - flags: MemoryRegionFlags::READ, + flags, region_type: MemoryRegionType::Heap, } } diff --git a/src/tests/rust_guests/simpleguest/src/main.rs b/src/tests/rust_guests/simpleguest/src/main.rs index 3bf093665..405087e25 100644 --- a/src/tests/rust_guests/simpleguest/src/main.rs +++ b/src/tests/rust_guests/simpleguest/src/main.rs @@ -798,6 +798,60 @@ fn read_mapped_buffer(function_call: &FunctionCall) -> Result> { } } +fn write_mapped_buffer(function_call: &FunctionCall) -> Result> { + if let (ParameterValue::ULong(base), ParameterValue::ULong(len)) = ( + function_call.parameters.clone().unwrap()[0].clone(), + function_call.parameters.clone().unwrap()[1].clone(), + ) { + let base = base as usize as *mut u8; + let len = len as usize; + + unsafe { + hyperlight_guest_bin::paging::map_region(base as _, base as _, len as u64 + 4096) + }; + + let data = unsafe { core::slice::from_raw_parts_mut(base, len) }; + + // should fail + data[0] = 0x42; + + // should never reach this + Ok(get_flatbuffer_result(true)) + } else { + Err(HyperlightGuestError::new( + ErrorCode::GuestFunctionParameterTypeMismatch, + "Invalid parameters passed to read_mapped_buffer".to_string(), + )) + } +} + +fn exec_mapped_buffer(function_call: &FunctionCall) -> Result> { + if let (ParameterValue::ULong(base), ParameterValue::ULong(len)) = ( + function_call.parameters.clone().unwrap()[0].clone(), + function_call.parameters.clone().unwrap()[1].clone(), + ) { + let base = base as usize as *mut u8; + let len = len as usize; + + unsafe { + hyperlight_guest_bin::paging::map_region(base as _, base as _, len as u64 + 4096) + }; + + let data = unsafe { core::slice::from_raw_parts(base, len) }; + + // Should be safe as long as data is something like a NOOP followed by a RET + let func: fn() = unsafe { core::mem::transmute(data.as_ptr()) }; + func(); + + Ok(get_flatbuffer_result(true)) + } else { + Err(HyperlightGuestError::new( + ErrorCode::GuestFunctionParameterTypeMismatch, + "Invalid parameters passed to read_mapped_buffer".to_string(), + )) + } +} + #[no_mangle] pub extern "C" fn hyperlight_main() { let read_from_user_memory_def = GuestFunctionDefinition::new( @@ -818,6 +872,24 @@ pub extern "C" fn hyperlight_main() { register_function(read_mapped_buffer_def); + let write_mapped_buffer_def = GuestFunctionDefinition::new( + "WriteMappedBuffer".to_string(), + Vec::from(&[ParameterType::ULong, ParameterType::ULong]), + ReturnType::Bool, + write_mapped_buffer as usize, + ); + + register_function(write_mapped_buffer_def); + + let exec_mapped_buffer_def = GuestFunctionDefinition::new( + "ExecMappedBuffer".to_string(), + Vec::from(&[ParameterType::ULong, ParameterType::ULong]), + ReturnType::Bool, + exec_mapped_buffer as usize, + ); + + register_function(exec_mapped_buffer_def); + let set_static_def = GuestFunctionDefinition::new( "SetStatic".to_string(), Vec::new(), diff --git a/src/tests/rust_guests/witguest/Cargo.lock b/src/tests/rust_guests/witguest/Cargo.lock index ddd6f659e..ea2537ea3 100644 --- a/src/tests/rust_guests/witguest/Cargo.lock +++ b/src/tests/rust_guests/witguest/Cargo.lock @@ -359,9 +359,9 @@ dependencies = [ [[package]] name = "prettyplease" -version = "0.2.35" +version = "0.2.36" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "061c1221631e079b26479d25bbf2275bfe5917ae8419cd7e34f13bfc2aa7539a" +checksum = "ff24dfcda44452b9816fff4cd4227e1bb73ff5a2f1bc1105aa92fb8565ce44d2" dependencies = [ "proc-macro2", "syn", @@ -522,9 +522,9 @@ checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" [[package]] name = "wasmparser" -version = "0.235.0" +version = "0.236.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "161296c618fa2d63f6ed5fffd1112937e803cb9ec71b32b01a76321555660917" +checksum = "16d1eee846a705f6f3cb9d7b9f79b54583810f1fb57a1e3aea76d1742db2e3d2" dependencies = [ "bitflags", "hashbrown",