-
Notifications
You must be signed in to change notification settings - Fork 16
munmap: consult vmmap before PROT_NONE to avoid unlinking adjacent mappings #1075
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 27 commits
096b55a
6a31a83
cfa6e5d
43146da
ecf13d1
bf88c29
0299dff
4b751a2
33a0502
55fed1e
d82a6f2
2fb3910
4bf8155
bde723c
fa60b13
0c58b90
0261ef6
2102768
801cfa0
d5ef87c
37065dc
e5aaf29
74aa480
ce33759
0937a39
021ec3e
4b60fd4
ee023e5
244324d
3d30361
101f31f
7f68ccd
abc7cbe
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 |
|---|---|---|
|
|
@@ -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 | ||
|
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. this is worth refactoring into a function in the vmmap i think. |
||
| .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!( | ||
|
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. I think these should be debug panics if they should never happen? if its just an errno we want to return errno tho
Contributor
Author
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. @rennergade could you please explain why they should be debug panics? I didn't understand the comment perfectly
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. I don't really understand what's happening here, in what situations are panics being triggered?
Contributor
Author
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. I actually copy pasted both of these error scenarios from before, that is, I modified the overlapping processing of regions logic but didn't think through the error scenarios coming out of that. But reading through the man page,
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. we basically should only panic if state is corrupted in some unrecoverable way, but we should use lind_debug_panic for that and not use the generic panic, if its just an errno thats normal we want to return that. |
||
| "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,25 @@ 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; | ||
| 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 | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| // Regression test: munmap with unaligned length must not destroy adjacent shm segment. | ||
| // Layout: [anon 2P][shm 1P], munmap(anon, 2P+1) rounds to 3P covering shm. | ||
| #include <sys/ipc.h> | ||
| #include <sys/shm.h> | ||
| #include <sys/mman.h> | ||
| #include <stdio.h> | ||
| #include <string.h> | ||
| #include <stdint.h> | ||
| #include <errno.h> | ||
|
|
||
| #define PAGE_SIZE 4096 | ||
|
|
||
| int main(void) { | ||
| // Create and attach shm | ||
| int shmid = shmget(4242, PAGE_SIZE, 0666 | IPC_CREAT); | ||
| if (shmid == -1) { | ||
| perror("shmget"); | ||
| return 1; | ||
| } | ||
|
|
||
| char *shm = (char *)shmat(shmid, NULL, 0); | ||
| if (shm == (char *)-1) { | ||
| perror("shmat"); | ||
| shmctl(shmid, IPC_RMID, NULL); | ||
| return 1; | ||
| } | ||
| memset(shm, 0xAB, PAGE_SIZE); | ||
|
|
||
| // Place anon immediately before shm using MAP_FIXED | ||
| char *anon = (char *)mmap(shm - 2 * PAGE_SIZE, 2 * PAGE_SIZE, | ||
| PROT_READ | PROT_WRITE, | ||
| MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); | ||
| if (anon == MAP_FAILED) { | ||
| printf("SKIP: MAP_FIXED failed (errno=%d)\n", errno); | ||
| shmdt(shm); | ||
| shmctl(shmid, IPC_RMID, NULL); | ||
| return 77; | ||
| } | ||
| memset(anon, 0xCD, 2 * PAGE_SIZE); | ||
|
|
||
| printf("Layout: anon=[%p-%p) shm=[%p-%p)\n", | ||
| (void *)anon, (void *)(anon + 2 * PAGE_SIZE), | ||
| (void *)shm, (void *)(shm + PAGE_SIZE)); | ||
|
|
||
| // munmap with unaligned length - rounds up to cover shm | ||
| if (munmap(anon, 2 * PAGE_SIZE + 1) != 0) { | ||
| perror("munmap"); | ||
| shmdt(shm); | ||
| shmctl(shmid, IPC_RMID, NULL); | ||
| return 1; | ||
| } | ||
|
|
||
| // Verify shm segment still exists | ||
| struct shmid_ds stat; | ||
| if (shmctl(shmid, IPC_STAT, &stat) == -1) { | ||
| printf("FAIL: shm segment destroyed\n"); | ||
| return 1; | ||
| } | ||
|
|
||
| // Re-attach and verify data | ||
| char *shm2 = (char *)shmat(shmid, NULL, 0); | ||
| if (shm2 == (char *)-1) { | ||
| perror("shmat re-attach"); | ||
| shmctl(shmid, IPC_RMID, NULL); | ||
| return 1; | ||
| } | ||
|
|
||
| for (int i = 0; i < PAGE_SIZE; i++) { | ||
| if ((unsigned char)shm2[i] != 0xAB) { | ||
| printf("FAIL: shm[%d]=0x%02x, expected 0xAB\n", i, (unsigned char)shm2[i]); | ||
| shmdt(shm2); | ||
| shmctl(shmid, IPC_RMID, NULL); | ||
| return 1; | ||
| } | ||
| } | ||
|
|
||
| printf("PASS: shm intact after unaligned munmap\n"); | ||
|
|
||
| // Cleanup disabled: lind-wasm shmdt has null pointer bug | ||
| // shmdt(shm2); | ||
| // shmctl(shmid, IPC_RMID, NULL); | ||
|
|
||
| return 0; | ||
| } |
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.
this probably needs a bit more context in the comments that this is handled in rawposix and why