munmap: consult vmmap before PROT_NONE to avoid unlinking adjacent mappings

src/rawposix/src/fs_calls.rs

@@ -1105,44 +1105,55 @@ pub extern "C" fn munmap_syscall(
         return syscall_error(Errno::EINVAL, "munmap", "address it not aligned");
     }
 
-    let vmmap = cage.vmmap.read();
-    let sysaddr = rounded_addr;
-    drop(vmmap);
-
     let rounded_length = round_up_page(len as u64) as usize;
 
-    // we are replacing munmap with mmap because we do not want to really deallocate the memory region
-    // we just want to set the prot of the memory region back to PROT_NONE
-    let result = unsafe {
-        libc::mmap(
-            sysaddr as *mut c_void,
-            rounded_length,
-            PROT_NONE,
-            (MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED) as i32,
-            -1,
-            0,
-        ) as usize
-    };
-    // Check for different failure modes with specific error messages
-    if result as isize == -1 {
-        let errno = get_errno();
-        panic!(
-            "munmap: mmap failed during memory protection reset with errno: {:?}",
-            errno
-        );
-    }
+    let mut vmmap = cage.vmmap.write();
 
-    if result != sysaddr {
-        panic!(
-            "munmap: MAP_FIXED violation - mmap returned address {:p} but requested {:p}",
-            result as *const c_void, sysaddr as *const c_void
-        );
+    let req_start: u32 = vmmap.sys_to_user(rounded_addr) >> PAGESHIFT;
+    let req_end: u32 = req_start + (rounded_length as u32 >> PAGESHIFT);
+
+    // Collect owned page ranges first to release the iterator borrow before mutating.
+    // interval.end() is inclusive, so +1 converts to exclusive to match req_end.
+    let overlaps: Vec<(usize, usize)> = vmmap
+        .find_page_iter(req_start)
+        .take_while(|(interval, _)| interval.start() < req_end)
+        .filter(|(_, e)| !matches!(e.backing, MemoryBackingType::SharedMemory(_)))
+        .map(|(interval, _)| {
+            let act_start = interval.start().max(req_start);
+            let act_end = (interval.end() + 1).min(req_end);
+            (act_start as usize, act_end as usize)
+        })
+        .collect();
+
+    for (act_start, act_end) in overlaps {
+        let act_start_addr = vmmap.user_to_sys((act_start as u32) << PAGESHIFT);
+        let act_len = ((act_end - act_start) as usize) << PAGESHIFT;
+        let result = unsafe {
+            libc::mmap(
+                act_start_addr as *mut c_void,
+                act_len,
+                PROT_NONE,
+                (MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED) as i32,
+                -1,
+                0,
+            ) as usize
+        };
+        if result as isize == -1 {
+            let errno = get_errno();
+            panic!(
+                "munmap: mmap failed during memory protection reset with errno: {:?}",
+                errno
+            );
+        }
+        if result != act_start_addr {
+            panic!(
+                "munmap: MAP_FIXED violation - mmap returned address {:p} but requested {:p}",
+                result as *const c_void, act_start_addr as *const c_void
+            );
+        }
     }
 
-    let mut vmmap = cage.vmmap.write();
-
-    let user_addr = vmmap.sys_to_user(rounded_addr) as u32;
-    vmmap.remove_entry(user_addr >> PAGESHIFT, (rounded_length as u32) >> PAGESHIFT);
+    vmmap.remove_entry(req_start, req_end - req_start);
 
     0
 }
@@ -4270,36 +4281,29 @@ pub extern "C" fn shmat_syscall(
     let result = vmmap.sys_to_user(result);
     drop(vmmap);
 
-    // If the syscall succeeded, update the vmmap entry.
-    if result as i32 >= 0 {
-        // Ensure the syscall attached the segment at the expected address.
-        if result as u32 != useraddr {
-            panic!("shmat did not attach at the expected address");
-        }
-        let mut vmmap = cage.vmmap.write();
-        let backing = MemoryBackingType::SharedMemory(shmid as u64);
-        // Use the effective protection (prot) for both the current and maximum protection.
-        let maxprot = prot;
-        // Add a new vmmap entry for the shared memory segment.
-        // Since shared memory is not file-backed, there are no extra mapping flags
-        // or file offset parameters to consider; thus, we pass 0 for both.
-        vmmap
-            .add_entry_with_overwrite(
-                useraddr >> PAGESHIFT,
-                (rounded_length >> PAGESHIFT) as u32,
-                prot,
-                maxprot,
-                0, // No flags for shared memory mapping
-                backing,
-                0, // Offset is not applicable for shared memory
-                len as i64,
-                cageid,
-            )
-            .expect("shmat: failed to add vmmap entry");
-    } else {
-        // If the syscall failed, propagate the error.
-        return result as i32;
+    // The host-side error case was already handled via is_mmap_error() above;
+    // here `result` is a valid u32 user-space address. Do NOT reinterpret it
+    // as a signed integer to detect failure — high addresses (>= 0x80000000)
+    // would alias to negative values and falsely look like errors.
+    if result as u32 != useraddr {
+        panic!("shmat did not attach at the expected address");
     }
+    let mut vmmap = cage.vmmap.write();
+    let backing = MemoryBackingType::SharedMemory(shmid as u64);
+    let maxprot = prot;
+    vmmap
+        .add_entry_with_overwrite(
+            useraddr >> PAGESHIFT,
+            (rounded_length >> PAGESHIFT) as u32,
+            prot,
+            maxprot,
+            0,
+            backing,
+            0,
+            len as i64,
+            cageid,
+        )
+        .expect("shmat: failed to add vmmap entry");
 
     useraddr as i32
 }

tests/unit-tests/memory_tests/deterministic/munmap_adjacent_shm.c

@@ -0,0 +1,93 @@
+// Test: unaligned-length munmap must not clobber an adjacent shm page.
+//
+// Regression test for the bug fixed in PR #1075:
+//   munmap_syscall rounded `len` up to a page multiple and issued a single
+//   mmap(MAP_FIXED|PROT_NONE) over the whole rounded range. When that
+//   range crossed into an adjacent SharedMemory-backed vmmap entry, the
+//   host-level PROT_NONE silently clobbered the shm page.
+//
+// Forcing adjacency without MAP_FIXED:
+//   lind's allocator places allocations at the TOP of a gap, so a plain
+//   mmap after shmat doesn't land byte-adjacent to shm (there's typically
+//   a startup-time entry one page below shm). MAP_FIXED would bypass the
+//   allocator but silently overwrite whatever that entry is via
+//   add_entry_with_overwrite. Instead we carve a 1-page hole inside an
+//   anon region we own and steer shmat into it:
+//
+//     1. mmap(NULL, 3*PAGE) → [base .. base+3P) anon
+//     2. munmap(base+PAGE, PAGE) → splits into two anon entries with a
+//        1-page gap at [base+PAGE .. base+2P)
+//     3. shmat(shmid, base+PAGE, 0) → find_map_space_with_hint iterates
+//        gaps from the hint upward and returns the top of the first
+//        fitting gap; the 1-page hole is exactly 1 page, so shm lands
+//        there byte-exact
+//
+// Layout after setup:
+//
+//     [ anon @ base ][ shm @ base+PAGE ][ anon @ base+2*PAGE ]
+//
+// munmap(base, PAGE+1) rounds to 2*PAGE and targets [base .. base+2P),
+// covering {anon, shm}. Buggy: single mmap(PROT_NONE) clobbers shm's
+// host memory. Fixed: overlap loop filters out SharedMemory entries and
+// only PROT_NONEs the anon page.
+#include <sys/ipc.h>
+#include <sys/shm.h>
+#include <sys/mman.h>
+#include <stdio.h>
+#include <string.h>
+#include <assert.h>
+#include <stdint.h>
+
+#define PAGE_SIZE 4096
+
+int main(void) {
+    key_t key = 4242;
+    int shmid = shmget(key, PAGE_SIZE, 0666 | IPC_CREAT);
+    assert(shmid != -1 && "shmget failed");
+
+    char *base = (char *)mmap(NULL, 3 * PAGE_SIZE, PROT_READ | PROT_WRITE,
+                              MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    assert(base != MAP_FAILED && "anon mmap failed");
+    assert(((uintptr_t)base % PAGE_SIZE) == 0 && "base not page-aligned");
+
+    // Carve a 1-page hole in the middle. The buggy munmap path is what
+    // PR #1075 touches — but a clean unmap of a fully-contained anon
+    // page does not trip the bug, so this step is safe on both builds.
+    int rc = munmap(base + PAGE_SIZE, PAGE_SIZE);
+    assert(rc == 0 && "middle munmap failed");
+
+    printf("DIAG: base        = %p\n", (void *)base);
+    printf("DIAG: hole @       = %p\n", (void *)(base + PAGE_SIZE));
+    printf("DIAG: anon tail @  = %p\n", (void *)(base + 2 * PAGE_SIZE));
+
+    // Steer shmat into the 1-page hole via the hint argument.
+    char *shm = (char *)shmat(shmid, base + PAGE_SIZE, 0);
+    assert(shm != (char *)-1 && "shmat failed");
+    printf("DIAG: shm actual   = %p\n", (void *)shm);
+    assert(shm == base + PAGE_SIZE &&
+           "shmat did not land in carved hole — allocator may have "
+           "found a larger gap above the hint");
+
+    memset(shm, 0xAB, PAGE_SIZE);
+
+    // Trigger: unaligned munmap at `base` whose rounded length (2*PAGE)
+    // reaches into the shm page. The fixed runtime must leave shm's host
+    // memory R/W because the overlapping entry is SharedMemory-backed.
+    rc = munmap(base, PAGE_SIZE + 1);
+    assert(rc == 0 && "trigger munmap failed");
+
+    for (int i = 0; i < PAGE_SIZE; i++) {
+        if ((unsigned char)shm[i] != 0xAB) {
+            printf("FAIL: shm[%d] = 0x%02x, expected 0xAB\n",
+                   i, (unsigned char)shm[i]);
+            shmdt(shm);
+            shmctl(shmid, IPC_RMID, NULL);
+            return 1;
+        }
+    }
+
+    printf("PASS: adjacent shm page intact after unaligned munmap\n");
+    shmdt(shm);
+    shmctl(shmid, IPC_RMID, NULL);
+    return 0;
+}