From 9ce9bffcf6dc75bd087685e23db85075317938fe Mon Sep 17 00:00:00 2001 From: Brooks Davis Date: Tue, 7 Jan 2025 18:43:37 +0000 Subject: [PATCH 1/4] vm_map_stack_locked: fix stack within a reservation If we're trying to install a stack within a reservation (uncommon and likely not sensible), we need to use the reservation's address not the address of the bottom of the stack. Fixes: #2252 --- bin/cheribsdtest/cheribsdtest_vm.c | 34 ++++++++++++++++++++++++++++++ sys/vm/vm_map.c | 14 +++++++++--- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/bin/cheribsdtest/cheribsdtest_vm.c b/bin/cheribsdtest/cheribsdtest_vm.c index a2db3ef3ac09..dd3a7092c9a8 100644 --- a/bin/cheribsdtest/cheribsdtest_vm.c +++ b/bin/cheribsdtest/cheribsdtest_vm.c @@ -2844,4 +2844,38 @@ CHERIBSDTEST(cheri_revoke_shm_anon_hoard_closed, #endif /* CHERIBSDTEST_CHERI_REVOKE_TESTS */ +/* + * This test is derived from a syskiller panic. Bugs in + * vm_map_stack_locked() when the stack was being inserted into an + * existing reservation (why would anyone do this in the real world?) + * caused a panic. + * https://github.com/CTSRD-CHERI/cheribsd/issues/2252 + */ +CHERIBSDTEST(mmap_insert_stack, "insert a stack mapping in a reservation") +{ + void *p; + + p = CHERIBSDTEST_CHECK_SYSCALL(mmap((void *)(intptr_t)0x20000000, + 0x1000000, PROT_WRITE | PROT_READ, + MAP_FIXED | MAP_ANONYMOUS | MAP_PRIVATE, -1, 0)); + + /* + * This would fail, but leave the map in a broken state due to + * trying to insert a reservation inside an existing one. + * + * We don't check it because it is needed to set up the panic. + */ + mmap(cheri_setaddress(p, 0x20ffc000), 0x2000, + PROT_WRITE | PROT_READ, MAP_STACK | MAP_FIXED, -1, 0); + + /* + * This would trigger a panic by trying to remove an unmapped + * entry left by the previous mmap. + */ + CHERIBSDTEST_CHECK_SYSCALL(munmap(cheri_setaddress(p, 0x20ffc000), + 0x3000)); + + cheribsdtest_success(); +} + #endif /* __CHERI_PURE_CAPABILITY__ */ diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c index 177a92493172..cc06e2f12416 100644 --- a/sys/vm/vm_map.c +++ b/sys/vm/vm_map.c @@ -5437,6 +5437,7 @@ vm_map_stack_locked(vm_map_t map, vm_pointer_t addrbos, vm_size_t max_ssize, vm_pointer_t bot, gap_bot, gap_top, top; vm_size_t init_ssize, sgp; int orient, rv; + vm_offset_t reservation; /* * The stack orientation is piggybacked with the cow argument. @@ -5462,13 +5463,18 @@ vm_map_stack_locked(vm_map_t map, vm_pointer_t addrbos, vm_size_t max_ssize, init_ssize = max_ssize - sgp; if (map->flags & MAP_RESERVATIONS) { - /* Check reservation exists */ + /* + * If this map enables reservations, a reservation must have + * already been created for us. + */ if (vm_map_lookup_entry(map, addrbos, &prev_entry) == 0 || (prev_entry->eflags & MAP_ENTRY_UNMAPPED) == 0) return (KERN_PROTECTION_FAILURE); /* If reservation can't accommodate max_ssize, no go. */ if (prev_entry->end - (vm_offset_t)addrbos < max_ssize) return (KERN_NO_SPACE); + + reservation = prev_entry->reservation; } else { /* If addr is already mapped, no go */ if (vm_map_lookup_entry(map, addrbos, &prev_entry)) @@ -5478,6 +5484,8 @@ vm_map_stack_locked(vm_map_t map, vm_pointer_t addrbos, vm_size_t max_ssize, */ if (vm_map_entry_succ(prev_entry)->start < addrbos + max_ssize) return (KERN_NO_SPACE); + + reservation = addrbos; } /* @@ -5501,7 +5509,7 @@ vm_map_stack_locked(vm_map_t map, vm_pointer_t addrbos, vm_size_t max_ssize, gap_bot = top; gap_top = addrbos + max_ssize; } - rv = vm_map_insert1(map, NULL, 0, bot, top, prot, max, cow, addrbos, + rv = vm_map_insert1(map, NULL, 0, bot, top, prot, max, cow, reservation, &new_entry); if (rv != KERN_SUCCESS) return (rv); @@ -5517,7 +5525,7 @@ vm_map_stack_locked(vm_map_t map, vm_pointer_t addrbos, vm_size_t max_ssize, return (KERN_SUCCESS); rv = vm_map_insert1(map, NULL, 0, gap_bot, gap_top, VM_PROT_NONE, VM_PROT_NONE, MAP_CREATE_GUARD | (orient == MAP_STACK_GROWS_DOWN ? - MAP_CREATE_STACK_GAP_DN : MAP_CREATE_STACK_GAP_UP), addrbos, + MAP_CREATE_STACK_GAP_DN : MAP_CREATE_STACK_GAP_UP), reservation, &gap_entry); if (rv == KERN_SUCCESS) { KASSERT((gap_entry->eflags & MAP_ENTRY_GUARD) != 0, From 9aa1ae1c9e7755f3f6a58e11006bbd9e33079bd9 Mon Sep 17 00:00:00 2001 From: Brooks Davis Date: Tue, 7 Jan 2025 19:32:34 +0000 Subject: [PATCH 2/4] cheribsdtest: rewrap CHERIBSDTEST_CHECK_CALL_ERROR Put backslashes in the 73nd column not 81st. --- bin/cheribsdtest/cheribsdtest.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/bin/cheribsdtest/cheribsdtest.h b/bin/cheribsdtest/cheribsdtest.h index a071fd67197f..5136283d23c9 100644 --- a/bin/cheribsdtest/cheribsdtest.h +++ b/bin/cheribsdtest/cheribsdtest.h @@ -308,14 +308,15 @@ _cheribsdtest_check_errno(const char *context, int actual, int expected) } /** Check that @p call fails and errno is set to @p expected_errno */ -#define CHERIBSDTEST_CHECK_CALL_ERROR(call, expected_errno) \ - do { \ - errno = 0; \ - int __ret = call; \ - int call_errno = errno; \ - CHERIBSDTEST_VERIFY2(__ret == -1, \ - #call " unexpectedly returned %d", __ret); \ - _cheribsdtest_check_errno(#call, call_errno, expected_errno); \ +#define CHERIBSDTEST_CHECK_CALL_ERROR(call, expected_errno) \ + do { \ + errno = 0; \ + int __ret = call; \ + int call_errno = errno; \ + CHERIBSDTEST_VERIFY2(__ret == -1, \ + #call " unexpectedly returned %d", __ret); \ + _cheribsdtest_check_errno(#call, call_errno, \ + expected_errno); \ } while (0) /* For libc_memcpy and libc_memset tests and the unaligned copy tests: */ From 29da12c5ec0cb9012fdf94b32f3735be29d0f5e6 Mon Sep 17 00:00:00 2001 From: Brooks Davis Date: Tue, 7 Jan 2025 19:21:09 +0000 Subject: [PATCH 3/4] cheribsdtest: improve CHERIBSDTEST_CHECK_CALL_ERROR Use typeof() to make CHERIBSDTEST_CHECK_CALL_ERROR compatible with functions that return pointers (e.g., mmap()). Print unexpected return values with %#p to support most integer values. --- bin/cheribsdtest/cheribsdtest.h | 15 ++++++++++++--- bin/cheribsdtest/cheribsdtest_vm.c | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/bin/cheribsdtest/cheribsdtest.h b/bin/cheribsdtest/cheribsdtest.h index 5136283d23c9..9e06b91d52fc 100644 --- a/bin/cheribsdtest/cheribsdtest.h +++ b/bin/cheribsdtest/cheribsdtest.h @@ -307,14 +307,23 @@ _cheribsdtest_check_errno(const char *context, int actual, int expected) context, actual, actual_str, expected, expected_str); } +#ifdef __CHERI_PURE_CAPABILITY__ +#define __CHERIBSDTEST_PTR_FMT "%#p" +#else +#define __CHERIBSDTEST_PTR_FMT "%p" +#endif + /** Check that @p call fails and errno is set to @p expected_errno */ #define CHERIBSDTEST_CHECK_CALL_ERROR(call, expected_errno) \ do { \ errno = 0; \ - int __ret = call; \ + __typeof(call) __ret = call; \ int call_errno = errno; \ - CHERIBSDTEST_VERIFY2(__ret == -1, \ - #call " unexpectedly returned %d", __ret); \ + CHERIBSDTEST_VERIFY2(__ret == (__typeof(__ret))-1, \ + _Generic((__ret), \ + void *: #call " unexpectedly returned " __CHERIBSDTEST_PTR_FMT, \ + default: #call " unexpectedly returned %d"), \ + __ret); \ _cheribsdtest_check_errno(#call, call_errno, \ expected_errno); \ } while (0) diff --git a/bin/cheribsdtest/cheribsdtest_vm.c b/bin/cheribsdtest/cheribsdtest_vm.c index dd3a7092c9a8..7dfe96cf6dcf 100644 --- a/bin/cheribsdtest/cheribsdtest_vm.c +++ b/bin/cheribsdtest/cheribsdtest_vm.c @@ -168,7 +168,7 @@ CHERIBSDTEST(vm_notag_mprotect_no_cap, static void mmap_check_bad_protections(int prot, int expected_errno) { - CHERIBSDTEST_CHECK_CALL_ERROR((int)(intptr_t)mmap(NULL, getpagesize(), + CHERIBSDTEST_CHECK_CALL_ERROR(mmap(NULL, getpagesize(), prot, MAP_ANON, -1, 0), expected_errno); } From 9426ade562c31bced4ccdb53dc27d2f51d79fb6a Mon Sep 17 00:00:00 2001 From: Brooks Davis Date: Tue, 7 Jan 2025 19:27:39 +0000 Subject: [PATCH 4/4] mmap: disallow MAP_STACK into a reservation It doesn't make sense to insert a stack into a reservation so disallow MAP_FIXED|MAP_STACK with a valid capability in addr. It's still allowed to MAP_FIXED|MAP_STACK, but only if there's nothing in that location (and you shouldn't be doing that). --- bin/cheribsdtest/cheribsdtest_vm.c | 15 ++++++++------- lib/libsys/mmap.2 | 8 ++++++++ sys/vm/vm_mmap.c | 2 ++ 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/bin/cheribsdtest/cheribsdtest_vm.c b/bin/cheribsdtest/cheribsdtest_vm.c index 7dfe96cf6dcf..021a3f79ccd3 100644 --- a/bin/cheribsdtest/cheribsdtest_vm.c +++ b/bin/cheribsdtest/cheribsdtest_vm.c @@ -2851,7 +2851,8 @@ CHERIBSDTEST(cheri_revoke_shm_anon_hoard_closed, * caused a panic. * https://github.com/CTSRD-CHERI/cheribsd/issues/2252 */ -CHERIBSDTEST(mmap_insert_stack, "insert a stack mapping in a reservation") +CHERIBSDTEST(mmap_insert_stack, + "try to insert a stack mapping in a reservation") { void *p; @@ -2860,13 +2861,13 @@ CHERIBSDTEST(mmap_insert_stack, "insert a stack mapping in a reservation") MAP_FIXED | MAP_ANONYMOUS | MAP_PRIVATE, -1, 0)); /* - * This would fail, but leave the map in a broken state due to - * trying to insert a reservation inside an existing one. - * - * We don't check it because it is needed to set up the panic. + * Historically would fail, but leave the map in a broken state + * due to trying to insert a reservation inside an existing one. + * This is now rejected outright. */ - mmap(cheri_setaddress(p, 0x20ffc000), 0x2000, - PROT_WRITE | PROT_READ, MAP_STACK | MAP_FIXED, -1, 0); + CHERIBSDTEST_CHECK_CALL_ERROR(mmap(cheri_setaddress(p, 0x20ffc000), + 0x2000, PROT_WRITE | PROT_READ, MAP_STACK | MAP_FIXED, -1, 0), + ENOMEM); /* * This would trigger a panic by trying to remove an unmapped diff --git a/lib/libsys/mmap.2 b/lib/libsys/mmap.2 index 54c8750f8925..b532302e860e 100644 --- a/lib/libsys/mmap.2 +++ b/lib/libsys/mmap.2 @@ -409,6 +409,14 @@ and .Dv PROT_WRITE . The size of the guard, in pages, is specified by sysctl .Dv security.bsd.stack_guard_page . +.Pp +Under CheriABI, +.Dv MAP_STACK +may not be combined with +.Dv MAP_FIXED +and an +.Fa addr +argument that is a valid pointer. .El .Pp The diff --git a/sys/vm/vm_mmap.c b/sys/vm/vm_mmap.c index 4cbd76031c1e..0104d1bc792a 100644 --- a/sys/vm/vm_mmap.c +++ b/sys/vm/vm_mmap.c @@ -335,6 +335,8 @@ sys_mmap(struct thread *td, struct mmap_args *uap) if (cheri_gettag(uap->addr)) { if ((flags & MAP_FIXED) == 0) return (EPROT); + else if ((flags & MAP_STACK) != 0) + return (ENOMEM); else if ((cheri_getperm(uap->addr) & CHERI_PERM_SW_VMEM)) source_cap = uap->addr; else {