[fix] Make sure memory is not mapped writeable into sandbox in kvm (#740)

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

Author: ludfjig

src/hyperlight_host/src/mem/memory_region.rs

@@ -324,9 +324,11 @@ impl From<MemoryRegion> 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 
             },
         }
     }

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

@@ -596,8 +596,12 @@ mod tests {
         let guest_base = 0x1_0000_0000; // Arbitrary guest base address
 
         unsafe {
-            sbox.map_region(&region_for_memory(&map_mem, guest_base))
-                .unwrap();
+            sbox.map_region(&region_for_memory(
+                &map_mem,
+                guest_base,
+                MemoryRegionFlags::READ,
+            ))
+            .unwrap();
         }
 
         let _guard = map_mem.lock.try_read().unwrap();
@@ -611,6 +615,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(&region_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::<bool>(
+                "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::<bool>(
+                "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;
@@ -626,13 +680,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,
         }
     }

src/tests/rust_guests/simpleguest/src/main.rs

@@ -798,6 +798,60 @@ fn read_mapped_buffer(function_call: &FunctionCall) -> Result<Vec<u8>> {
     }
 }
 
+fn write_mapped_buffer(function_call: &FunctionCall) -> Result<Vec<u8>> {
+    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<Vec<u8>> {
+    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(),