MAP_STACK fixes

bin/cheribsdtest/cheribsdtest.h

@@ -307,15 +307,25 @@
 	    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;						\
-		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;						\
+		__typeof(call) __ret = call;				\
+		int call_errno = errno;					\
+		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)
 
 /* For libc_memcpy and libc_memset tests and the unaligned copy tests: */

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);
 }
 
@@ -2844,4 +2844,39 @@ 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,
+    "try to 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));
+
+	/*
+	 * 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.
+	 */
+	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
+	 * entry left by the previous mmap.
+	 */
+	CHERIBSDTEST_CHECK_SYSCALL(munmap(cheri_setaddress(p, 0x20ffc000),
+	    0x3000));
+
+	cheribsdtest_success();
+}
+
 #endif /* __CHERI_PURE_CAPABILITY__ */

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

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,

sys/vm/vm_mmap.c

@@ -335,9 +335,11 @@
 	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 {
 			SYSERRCAUSE("MAP_FIXED without CHERI_PERM_SW_VMEM");
 			return (EACCES);
 		}