[fix] Make sure memory is not mapped writeable into sandbox in kvm

Signed-off-by: Ludvig Liljenberg <[email protected]>

Author: ludfjig

src/hyperlight_host/src/mem/memory_region.rs

@@ -314,6 +314,8 @@ impl From<MemoryRegion> for mshv_user_mem_region {
 #[cfg(kvm)]
 impl From<MemoryRegion> for kvm_bindings::kvm_userspace_memory_region {
     fn from(region: MemoryRegion) -> Self {
+        use bitflags::bitflags_match;
+
         let perm_flags =
             MemoryRegionFlags::READ | MemoryRegionFlags::WRITE | MemoryRegionFlags::EXECUTE;
 
@@ -324,10 +326,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 {
+            flags: bitflags_match!(perm_flags, {
                 MemoryRegionFlags::READ => KVM_MEM_READONLY,
+                MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE => KVM_MEM_READONLY, // KVM_MEM_READONLY is executable
                 _ => 0, // normal, RWX
-            },
+            }),
         }
     }
 }

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

@@ -605,8 +605,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();
@@ -620,6 +624,47 @@ mod tests {
         assert_eq!(actual, expected);
     }
 
+    // Makes sure MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE is not writable
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn test_mmap_write() {
+        let mut sbox = UninitializedSandbox::new(
+            GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")),
+            None,
+        )
+        .unwrap()
+        .evolve()
+        .unwrap();
+
+        let expected = b"hello world";
+        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();
+
+        // 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;
@@ -635,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,33 @@ 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(),
+        ))
+    }
+}
+
 #[no_mangle]
 pub extern "C" fn hyperlight_main() {
     let read_from_user_memory_def = GuestFunctionDefinition::new(
@@ -818,6 +845,15 @@ 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 set_static_def = GuestFunctionDefinition::new(
         "SetStatic".to_string(),
         Vec::new(),