Syscalls operating on memory mappings manage their address space via owning capabilities. They must adhere to a certain set of rules[1] in order to ensure memory safety. Address space management syscalls are only allowed to manipulate mappings that are within the range of the owning capability and have the appropriate permissions. Tests to vailidate the parameters being passed to the syscall, check its bounds, range as well as permissions have been added. Additionally, a signal handler has been registered to handle invalid memory access. Finally, as certain flags and syscalls conflict with the reservation model or lack implementation, a check to verify appropriate handling of the same has also been added.
Review branch: https://git.morello-project.org/chaitanya_prakash/linux/-/tree/review/mmap_t...
This patch series has been tested on: https://git.morello-project.org/amitdaniel/linux/-/tree/review/extern_reserv...
[1] https://git.morello-project.org/morello/kernel/linux/-/wikis/Morello-pure-ca...
Changes in V2: - Added link to the review branch - Removed unnecessary whitespace
Changes in V1: https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...
Chaitanya S Prakash (8): kselftests/arm64/morello: Add necessary support for mmap testcases kselftests/arm64/morello: Add MAP_GROWSDOWN testcase kselftests/arm64/morello: Add parameter check testcases kselftests/arm64/morello: Add capability range testcases kselftests/arm64/morello: Add mmap() bounds check testcases kselftests/arm64/morello: Add mremap() bounds check testcases kselftests/arm64/morello: Add mremap() permission testcases kselftests/arm64/morello: Add brk() testcase
.../testing/selftests/arm64/morello/Makefile | 1 + .../selftests/arm64/morello/freestanding.h | 62 ++- tools/testing/selftests/arm64/morello/mmap.c | 479 +++++++++++++++++- 3 files changed, 535 insertions(+), 7 deletions(-)
Wrapper functions for system calls that are frequently invoked are added in order to improve ease of usability. Making use of the newly added support, mmap.c can be re-written in a manner that avoids directly interacting with syscalls. Required datatypes and constants have also been defined. Additionally, dependency required to utilise signal support has also been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com --- .../testing/selftests/arm64/morello/Makefile | 1 + .../selftests/arm64/morello/freestanding.h | 62 ++++++++++++++++++- tools/testing/selftests/arm64/morello/mmap.c | 11 ++-- 3 files changed, 67 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/Makefile b/tools/testing/selftests/arm64/morello/Makefile index ecfa6fc5e989..d2e100a5aadc 100644 --- a/tools/testing/selftests/arm64/morello/Makefile +++ b/tools/testing/selftests/arm64/morello/Makefile @@ -43,3 +43,4 @@ $(OUTPUT)/%: $(OUTPUT)/%.o $(OUTPUT)/freestanding_start.o $(OUTPUT)/freestanding
$(OUTPUT)/signal: $(OUTPUT)/signal_common.o $(OUTPUT)/clone: $(OUTPUT)/signal_common.o +$(OUTPUT)/mmap: $(OUTPUT)/signal_common.o diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 2beb52eafae1..3badf163569c 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -21,6 +21,9 @@ typedef __kernel_timer_t timer_t; typedef __kernel_clockid_t clockid_t; typedef __kernel_uid_t uid_t; typedef __kernel_mode_t mode_t; +typedef __kernel_off_t off_t; +typedef __kernel_key_t key_t; + #ifndef __clang__ typedef __uintcap_t uintcap_t; #endif @@ -29,6 +32,7 @@ typedef __uintcap_t uintcap_t; #define INT_MAX __INT_MAX__ #endif
+#define PAGE_SIZE 4096 #define EXIT_SUCCESS 0
#ifndef WIFEXITED @@ -160,6 +164,16 @@ static inline ssize_t write(int fd, const void *buf, size_t count) return syscall(__NR_write, fd, buf, count); }
+static inline off_t lseek(int fd, off_t offset, int whence) +{ + return syscall(__NR_lseek, fd, offset, whence); +} + +static inline int openat(int dirfd, const char *pathname, int flags, mode_t mode) +{ + return syscall(__NR_openat, dirfd, pathname, flags, mode); +} + long __clone(int (*fn)(void *), uintcap_t stack, int flags, void *arg, pid_t *parent_tid, void *tls, pid_t *child_tid);
@@ -174,8 +188,49 @@ static inline int munmap(void *addr, size_t length) return syscall(__NR_munmap, addr, length); }
+static inline int madvise(void *addr, size_t length, int advise) +{ + return syscall(__NR_madvise, addr, length, advise); +} + +static inline int mincore(void *addr, size_t length, unsigned char *vec) +{ + return syscall(__NR_mincore, addr, length, vec); +} + +static inline int mlock(const void *addr, size_t len) +{ + return syscall(__NR_mlock, addr, len); +} + +static inline int mlock2(const void *addr, size_t len, unsigned int flags) +{ + return syscall(__NR_mlock2, addr, len, flags); +} + +static inline int munlock(const void *addr, size_t len) +{ + return syscall(__NR_munlock, addr, len); +} + +static inline int msync(void *addr, size_t length, int flags) +{ + return syscall(__NR_msync, addr, length, flags); +} + +static inline int mprotect(void *addr, size_t length, int prot) +{ + return syscall(__NR_mprotect, addr, length, prot); +} + +static inline void *mremap(void *old_address, size_t old_size, size_t new_size, + int flags, void *new_address) +{ + return (void *)syscall(__NR_mremap, old_address, old_size, new_size, flags, new_address); +} + static inline void *mmap_verified(void *addr, size_t length, int prot, int flags, - int fd, int offset, unsigned int perms) + int fd, int offset, unsigned int perms) { void *__addr = mmap(addr, length, prot, flags, fd, offset);
@@ -200,6 +255,11 @@ static inline void *mmap_verified(void *addr, size_t length, int prot, int flags return NULL; }
+static inline int brk(void *addr) +{ + return syscall(__NR_brk, addr); +} + static inline int close(int fd) { return syscall(__NR_close, fd); diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 5d259c336f55..2dd4ccdb0d2a 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -71,11 +71,11 @@ void syscall_mmap2(void) int retval;
/* create a sample file to map onto with mmap */ - fd = syscall(__NR_openat, 0, sample_file, O_RDWR | O_CREAT, FILE_PERM); + fd = openat(0, sample_file, O_RDWR | O_CREAT, FILE_PERM);
ASSERT_GE(fd, 0);
- retval = syscall(__NR_lseek, fd, MMAP_SIZE, SEEK_SET); + retval = lseek(fd, MMAP_SIZE, SEEK_SET); ASSERT_EQ(retval, MMAP_SIZE);
/* attempt to write arbitrary data to file */ @@ -92,18 +92,17 @@ void syscall_mmap2(void) PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));
/* Attempt to change bounds of memory mapping, shrink by factor of 2 */ - addr = (void *)syscall(__NR_mremap, addr, MMAP_SIZE, - MMAP_SIZE_REDUCED, 0, 0); + addr = mremap(addr, MMAP_SIZE, MMAP_SIZE_REDUCED, 0, 0);
ASSERT_FALSE(IS_ERR_VALUE(addr)); /* advise kernel about how to handle paging of mapped memory.*/ - retval = syscall(__NR_madvise, addr, MMAP_SIZE_REDUCED, MADV_WILLNEED); + retval = madvise(addr, MMAP_SIZE_REDUCED, MADV_WILLNEED); ASSERT_EQ(retval, 0);
EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED, PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); /* An attempt to change permissions to RO */ - retval = syscall(__NR_mprotect, addr, MMAP_SIZE_REDUCED, PROT_READ); + retval = mprotect(addr, MMAP_SIZE_REDUCED, PROT_READ); ASSERT_EQ(retval, 0); /* Write permission should be revoked - verify mode only */ /* To be extended when signals are fully supported */
The mmap() system call is expected to fail with -EOPNOTSUPP when the MAP_GROWSDOWN flag is passed. A testcase to verify this behaviour has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com --- tools/testing/selftests/arm64/morello/mmap.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 2dd4ccdb0d2a..00d4c5ea9703 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -117,6 +117,17 @@ void syscall_mmap2(void) close(fd); }
+/* test to verify mmap() behaviour when MAP_GROWSDOWN flag is specified */ +static void purecap_map_growsdown(void) +{ + void *addr; + int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_GROWSDOWN; + + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); +} + TEST(test_syscall_mmap) { syscall_mmap(); @@ -127,9 +138,15 @@ TEST(test_syscall_mmap2) syscall_mmap2(); }
+TEST(test_purecap_map_growsdown) +{ + purecap_map_growsdown(); +} + int main(void) { test_syscall_mmap(); test_syscall_mmap2(); + test_purecap_map_growsdown(); return 0; }
Only valid owning capabilities have the ability to modify an address space by making use of the appropriate syscalls, any deviation from this method must result in immediate failure of the syscall. When a capability is created it is assigned the maximum permissions it may ever have by passing PROT_MAX(max_prot) as one of the prot flags. Any attempt to increase the permissions beyond this would result in failure of the syscall.
A test to validate the parmeters passed to address space management syscalls has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com --- tools/testing/selftests/arm64/morello/mmap.c | 76 ++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 00d4c5ea9703..184dcf4ddc92 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -128,6 +128,76 @@ static void purecap_map_growsdown(void) ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); }
+ +/* test to validate parameters passed to address space management syscalls */ +static inline void purecap_param_check(void) +{ + void *addr, *addr_cap, *null_cap_addr; + ptraddr_t address; + int retval; + int max_prot = PROT_READ | PROT_WRITE | PROT_EXEC; + int prot = max_prot; + int flags = MAP_PRIVATE | MAP_ANONYMOUS; + + /* generate null-derived capability to be used later*/ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + address = cheri_address_get(addr); + null_cap_addr = (void *)(uintptr_t)address; + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* passing invalid capability to sycall */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + addr_cap = cheri_tag_clear(addr); + retval = munmap(addr_cap, MMAP_SIZE); + ASSERT_EQ(retval, -EINVAL); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* increase permission beyond the maximum prot specified for the mapping */ + addr = mmap(NULL, MMAP_SIZE, PROT_MAX(PROT_READ | PROT_WRITE) | + PROT_READ | PROT_WRITE, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mprotect(addr, MMAP_SIZE, PROT_EXEC); + ASSERT_EQ(retval, -EINVAL); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* max_prot has fewer permissions than prot */ + max_prot = (prot & ~(PROT_READ)); + addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0); + ASSERT_EQ((unsigned long)addr, (unsigned long)-EINVAL); + + /* max_prot has more permissions than prot */ + max_prot = prot; + prot = (prot & ~(PROT_WRITE)); + addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + + retval = mprotect(addr, MMAP_SIZE, PROT_WRITE); + ASSERT_NE(retval, -EINVAL); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* repeat positive max_prot test with null-derived capability */ + addr = mmap(null_cap_addr, MMAP_SIZE, PROT_MAX(max_prot) | prot, + flags | MAP_FIXED, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + + retval = mprotect(addr, MMAP_SIZE, PROT_WRITE); + ASSERT_EQ(retval, 0); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); +} + TEST(test_syscall_mmap) { syscall_mmap(); @@ -143,10 +213,16 @@ TEST(test_purecap_map_growsdown) purecap_map_growsdown(); }
+TEST(test_purecap_param_check) +{ + purecap_param_check(); +} + int main(void) { test_syscall_mmap(); test_syscall_mmap2(); test_purecap_map_growsdown(); + test_purecap_param_check(); return 0; }
The length of the initial mapping using an mmap() call has the ability to be reduced in a subsequent mmap() call without first being unmapped. Whereas, trying to increase the length of the initial mapping would result in failure as the capability is not permitted to access a range that it does not own.
Address space management syscalls that manipulate a given mapping are restricted to the range owned by the capability. Any attempt to modify or access memory beyond this range will result in failure of syscall.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com --- tools/testing/selftests/arm64/morello/mmap.c | 132 +++++++++++++++++++ 1 file changed, 132 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 184dcf4ddc92..4a2a552d47dc 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -198,6 +198,132 @@ static inline void purecap_param_check(void) ASSERT_EQ(retval, 0); }
+/* test to verify address space management syscall behaviour when capability + * range is modified */ +static inline void purecap_range_check(void) +{ + void *addr, *ret; + int retval; + int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS; + unsigned char vec[MMAP_SIZE / PAGE_SIZE]; + + /* mapping a smaller range at prev mmap addr in a subsequent mmap() + * call without first unmapping */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + ret = mmap(addr, MMAP_SIZE_REDUCED, prot, flags | MAP_FIXED, -1, 0); + ASSERT_GT((unsigned long)ret, 0); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); + + /* mapping a larger range at prev mmap addr in a subsequent mmap() + * call without first unmapping */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + ret = mmap(addr, 2 * MMAP_SIZE, prot, flags | MAP_FIXED, -1, 0); + ASSERT_EQ((unsigned long)ret, (unsigned long)-EINVAL); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* positive madvise() range test */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = madvise(addr, MMAP_SIZE, MADV_WILLNEED); + ASSERT_NE(retval, -EINVAL); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* negative madvise() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = madvise(addr, MMAP_SIZE, MADV_NORMAL); + ASSERT_EQ(retval, -EINVAL); + + retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); + + /* positive mincore() range test */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mincore(addr, MMAP_SIZE, vec); + ASSERT_EQ(retval, 0); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* negative mincore() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mincore(addr, MMAP_SIZE, vec); + ASSERT_EQ(retval, -EINVAL); + + retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); + + /* positive mlock() range test */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mlock(addr, MMAP_SIZE); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + retval = munlock(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* negative mlock() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mlock(addr, MMAP_SIZE); + ASSERT_EQ(retval, -EINVAL); + + retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); + + /* negative munlock() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + EXPECT_EQ(0, mlock2(addr, MMAP_SIZE_REDUCED, MLOCK_ONFAULT)); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + + retval = munlock(addr, MMAP_SIZE); + ASSERT_EQ(retval, -EINVAL); + + retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); + + /* positive msync() range test */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = msync(addr, MMAP_SIZE, MS_SYNC); + ASSERT_EQ(retval, 0); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* negative msync() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = msync(addr, MMAP_SIZE, MS_SYNC); + ASSERT_EQ(retval, -EINVAL); + + retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); +} + TEST(test_syscall_mmap) { syscall_mmap(); @@ -218,11 +344,17 @@ TEST(test_purecap_param_check) purecap_param_check(); }
+TEST(test_purecap_range_check) +{ + purecap_range_check(); +} + int main(void) { test_syscall_mmap(); test_syscall_mmap2(); test_purecap_map_growsdown(); test_purecap_param_check(); + test_purecap_range_check(); return 0; }
Reservations are contiguous ranges of virtual addresses that exactly match the bounds of an owning capability which is provided by the kernel. When an owning capability is passed to a syscall, its bounds are first verified against the existing reservations. If the bounds of the capability are found to not match any of the existing reservations, the syscall is expected to fail with a -ERESERVATION error code. Tests to verify this behaviour has been added.
An additional check to verify whether the representable length returned by the capability is greater than or equal to the bounds set for the capability has been added. Finally, signal handler has also been registered to handle invalid memory access.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com --- tools/testing/selftests/arm64/morello/mmap.c | 116 +++++++++++++++++++ 1 file changed, 116 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 4a2a552d47dc..c7f1ddb395c5 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -8,8 +8,13 @@ #include <linux/errno.h> #include <linux/fs.h> #include <linux/fcntl.h> +#include <linux/signal.h> +#include <asm/sigcontext.h> +#include <asm/siginfo.h> +#include <asm/ucontext.h> #include <cheriintrin.h> #include "freestanding.h" +#include "signal_common.h"
#define MMAP_SIZE ((1ULL << 16) << 1) /* 64k x 2 */ @@ -19,6 +24,7 @@ #define PROBE_MODE_TOUCH 0x01 #define PROBE_MODE_VERIFY 0x02
+static volatile unsigned int signal_status;
static inline int probe_mem_range(void *addr, size_t size, int mode) { @@ -37,6 +43,37 @@ static inline int probe_mem_range(void *addr, size_t size, int mode) return 0; }
+static void purecap_sigsegv_handler(int n, siginfo_t *si, void *data) +{ + struct ucontext *uc = (struct ucontext *)data; + + ASSERT_EQ(si->si_signo, n); + TH_LOG("Signal (%d) occurred", n); + uc->uc_mcontext.pc += 4; + signal_status = 1; +} + +static void raise_sigsegv(void) +{ + void *addr; + long value; + int retval; + unsigned int *invalid_addr; + int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS; + + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + invalid_addr = (unsigned int *)addr; + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + invalid_addr = (unsigned int *)(addr + MMAP_SIZE); + asm volatile("ldr %w0, [%1]" : "=r" (value) : "C" (invalid_addr) : "memory"); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); +} + /* Simple test to check our ability to create a new anonymous mapping * in the virtual address space of the calling process */ @@ -324,6 +361,79 @@ static inline void purecap_range_check(void) ASSERT_EQ(retval, 0); }
+/* test to verify mmap() behaviour when capability bounds are modified */ +static inline void purecap_mmap_bounds_check(void) +{ + int retval; + ptraddr_t address; + size_t res_length; + void *addr, *addr_cap, *ddc_cap, *ddc_value, *new_addr, *null_cap_addr; + int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS; + + struct sigaction sa; + + /* generate a null-derived capability to be used later */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + + address = cheri_address_get(addr); + null_cap_addr = (void *)(uintptr_t)address; + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + + /* bounds of capability is increased by using root capability of the DDC + * register */ + ddc_value = cheri_ddc_get(); + ddc_cap = cheri_address_set(ddc_value, cheri_address_get(addr)); + ddc_cap = cheri_bounds_set(ddc_value, 2 * MMAP_SIZE); + ddc_cap = cheri_perms_and(ddc_value, cheri_perms_get(addr)); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + retval = munmap(ddc_cap, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* bounds of capability returned by mmap() is reduced, followed by a + * representable length check */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + addr_cap = cheri_bounds_set(addr, MMAP_SIZE_REDUCED); + res_length = cheri_representable_length((unsigned long)addr_cap); + EXPECT_GE(res_length, MMAP_SIZE_REDUCED); + + retval = munmap(addr_cap, MMAP_SIZE); + ASSERT_EQ(retval, -EINVAL); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* overlapping mappings produce a reservation error */ + addr = mmap(null_cap_addr, MMAP_SIZE, prot, flags | MAP_FIXED, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + new_addr = mmap(null_cap_addr + MMAP_SIZE_REDUCED, MMAP_SIZE_REDUCED, + prot, flags | MAP_FIXED, -1, 0); + ASSERT_EQ((unsigned long)new_addr, (unsigned long)-ERESERVATION); + + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* register a signal handler to catch invalid memory access */ + sigemptyset(&sa.sa_mask); + sa.sa_handler = (sighandler_t)(void *)purecap_sigsegv_handler; + sa.sa_flags = SA_SIGINFO; + sigaction(SIGSEGV, &sa, NULL); + signal_status = 0; + raise_sigsegv(); + ASSERT_TRUE(signal_status); + TH_LOG("Segmenation fault caused by invalid memory access has been handled."); +} + TEST(test_syscall_mmap) { syscall_mmap(); @@ -349,6 +459,11 @@ TEST(test_purecap_range_check) purecap_range_check(); }
+TEST(test_purecap_mmap_bounds_check) +{ + purecap_mmap_bounds_check(); +} + int main(void) { test_syscall_mmap(); @@ -356,5 +471,6 @@ int main(void) test_purecap_map_growsdown(); test_purecap_param_check(); test_purecap_range_check(); + test_purecap_mmap_bounds_check(); return 0; }
Attempting to remap a range larger than what is owned by the capability trigger an -EINVAL error. Additionally, mappings that have to be moved in order to satisfy the new constraints, expect the MREMAP_MAYMOVE flag to be specified. Failure to do so also triggers the -ERESERVATION error. Tests to verify this behaviour has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com --- tools/testing/selftests/arm64/morello/mmap.c | 44 ++++++++++++++++++++ 1 file changed, 44 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index c7f1ddb395c5..aa552c8affba 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -434,6 +434,44 @@ static inline void purecap_mmap_bounds_check(void) TH_LOG("Segmenation fault caused by invalid memory access has been handled."); }
+/* test to verify mremap() behaviour when capability bounds are modified */ +static inline void purecap_mremap_bounds_check(void) +{ + void *addr, *new_addr; + int retval; + int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS; + + /* moving a mapping with MREMAP_MAYMOVE flag specified */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + new_addr = mremap(addr, MMAP_SIZE_REDUCED, MMAP_SIZE, MREMAP_MAYMOVE, 0); + ASSERT_GT((unsigned long)new_addr, 0); + EXPECT_EQ(0, probe_mem_range(new_addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + + retval = munmap(new_addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* moving a mapping without MREMAP_MAYMOVE flag triggers a reservation error */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + new_addr = mremap(addr, MMAP_SIZE_REDUCED, MMAP_SIZE, 0, 0); + ASSERT_EQ((unsigned long)new_addr, (unsigned long)-ERESERVATION); + + retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); + + /* attempt to resize a mapping range greater than what the capability owns */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + new_addr = mremap(addr, MMAP_SIZE, MMAP_SIZE, MREMAP_MAYMOVE, 0); + ASSERT_EQ((unsigned long)new_addr, (unsigned long)-EINVAL); + + retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); +} + TEST(test_syscall_mmap) { syscall_mmap(); @@ -464,6 +502,11 @@ TEST(test_purecap_mmap_bounds_check) purecap_mmap_bounds_check(); }
+TEST(test_purecap_mremap_bounds_check) +{ + purecap_mremap_bounds_check(); +} + int main(void) { test_syscall_mmap(); @@ -472,5 +515,6 @@ int main(void) test_purecap_param_check(); test_purecap_range_check(); test_purecap_mmap_bounds_check(); + test_purecap_mremap_bounds_check(); return 0; }
As mremap() doesn't take prot flags into consideration, the permissions of the original address are retained. If the permissions of the new address do not match the set of permissions of the old address, the syscall fails with a -EINVAL error code. A test to verify this behaviour has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com --- tools/testing/selftests/arm64/morello/mmap.c | 68 ++++++++++++++++++++ 1 file changed, 68 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index aa552c8affba..3b8e99aafedc 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -165,6 +165,68 @@ static void purecap_map_growsdown(void) ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); }
+/* test to verify mremap() behaviour when permissions are modified */ +static void purecap_mremap_perms_check(void) +{ + void *old_addr, *new_addr, *ret; + int retval; + int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS; + size_t old_perms, new_perms; + + /* permissions of capability returned by mremap must match the + * permissions returned by the original mapping */ + old_addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)old_addr, 0); + old_perms = cheri_perms_get(old_addr); + new_addr = mremap(old_addr, MMAP_SIZE_REDUCED, MMAP_SIZE, + MREMAP_MAYMOVE, 0); + ASSERT_GT((unsigned long)new_addr, 0); + new_perms = cheri_perms_get(new_addr); + ASSERT_EQ(old_perms, new_perms); + EXPECT_EQ(0, probe_mem_range(new_addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + + retval = munmap(new_addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* remapping to a new_addr having reduced permissions from old_addr */ + old_addr = mmap(NULL, MMAP_SIZE_REDUCED, PROT_MAX(prot | PROT_EXEC) | + prot | PROT_EXEC, flags, -1, 0); + ASSERT_GT((unsigned long)old_addr, 0); + + new_addr = mmap(NULL, MMAP_SIZE, PROT_MAX(prot) | prot, flags, -1, 0); + ASSERT_GT((unsigned long)new_addr, 0); + retval = munmap(new_addr, MMAP_SIZE); + ASSERT_EQ(0, retval); + + ret = mremap(old_addr, MMAP_SIZE_REDUCED, MMAP_SIZE, + MREMAP_MAYMOVE | MREMAP_FIXED, new_addr); + + EXPECT_EQ(0, probe_mem_range(ret, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + + retval = munmap(ret, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + /* remapping to new_addr having increased permissions from old_addr */ + old_addr = mmap(NULL, MMAP_SIZE_REDUCED, PROT_MAX(prot) | prot, flags, + -1, 0); + ASSERT_GT((unsigned long)old_addr, 0); + + new_addr = mmap(NULL, MMAP_SIZE, PROT_MAX(prot | PROT_EXEC) | prot | + PROT_EXEC, flags, -1, 0); + ASSERT_GT((unsigned long)new_addr, 0); + retval = munmap(new_addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); + + ret = mremap(old_addr, MMAP_SIZE_REDUCED, MMAP_SIZE, + MREMAP_MAYMOVE | MREMAP_FIXED, new_addr); + ASSERT_EQ((unsigned long)ret, (unsigned long)-EINVAL); + + retval = munmap(old_addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); +}
/* test to validate parameters passed to address space management syscalls */ static inline void purecap_param_check(void) @@ -507,6 +569,11 @@ TEST(test_purecap_mremap_bounds_check) purecap_mremap_bounds_check(); }
+TEST(test_purecap_mremap_perms_check) +{ + purecap_mremap_perms_check(); +} + int main(void) { test_syscall_mmap(); @@ -516,5 +583,6 @@ int main(void) test_purecap_range_check(); test_purecap_mmap_bounds_check(); test_purecap_mremap_bounds_check(); + test_purecap_mremap_perms_check(); return 0; }
As the mechanism of brk() depends on implicit address space reservation by moving the program break, it is unfavourable to the capability model. Hence an assumption is made that brk() is unnecessary and allocators making use of it can use mmap() instead. If used, it returns -ENOSYS. A test to verify this behaviour has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com --- tools/testing/selftests/arm64/morello/mmap.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 3b8e99aafedc..118bb4fe6334 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -534,6 +534,15 @@ static inline void purecap_mremap_bounds_check(void) ASSERT_EQ(retval, 0); }
+/* test to verify that using brk() results syscall failure */ +static inline void purecap_brk_check(void) +{ + int retval; + + retval = brk(NULL); + ASSERT_EQ(retval, -ENOSYS); +} + TEST(test_syscall_mmap) { syscall_mmap(); @@ -574,6 +583,11 @@ TEST(test_purecap_mremap_perms_check) purecap_mremap_perms_check(); }
+TEST(test_purecap_brk_check) +{ + purecap_brk_check(); +} + int main(void) { test_syscall_mmap(); @@ -584,5 +598,6 @@ int main(void) test_purecap_mmap_bounds_check(); test_purecap_mremap_bounds_check(); test_purecap_mremap_perms_check(); + test_purecap_brk_check(); return 0; }
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
Syscalls operating on memory mappings manage their address space via owning capabilities. They must adhere to a certain set of rules[1] in order to ensure memory safety. Address space management syscalls are only allowed to manipulate mappings that are within the range of the owning capability and have the appropriate permissions. Tests to vailidate the parameters being passed to the syscall, check its bounds, range as well as permissions have been added. Additionally, a signal handler has been registered to handle invalid memory access. Finally, as certain flags and syscalls conflict with the reservation model or lack implementation, a check to verify appropriate handling of the same has also been added.
Great series! I am very impressed by the comprehensiveness of the tests and the quality of the code is generally very good! Well done!
I have only a few notes/comments. If you see a comment, it might apply to multiple patches.
Thanks, Tudor
Review branch: https://git.morello-project.org/chaitanya_prakash/linux/-/tree/review/mmap_t...
This patch series has been tested on: https://git.morello-project.org/amitdaniel/linux/-/tree/review/extern_reserv...
[1] https://git.morello-project.org/morello/kernel/linux/-/wikis/Morello-pure-ca...
Changes in V2:
- Added link to the review branch
- Removed unnecessary whitespace
Changes in V1: https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...
Chaitanya S Prakash (8): kselftests/arm64/morello: Add necessary support for mmap testcases kselftests/arm64/morello: Add MAP_GROWSDOWN testcase kselftests/arm64/morello: Add parameter check testcases kselftests/arm64/morello: Add capability range testcases kselftests/arm64/morello: Add mmap() bounds check testcases kselftests/arm64/morello: Add mremap() bounds check testcases kselftests/arm64/morello: Add mremap() permission testcases kselftests/arm64/morello: Add brk() testcase
.../testing/selftests/arm64/morello/Makefile | 1 + .../selftests/arm64/morello/freestanding.h | 62 ++- tools/testing/selftests/arm64/morello/mmap.c | 479 +++++++++++++++++- 3 files changed, 535 insertions(+), 7 deletions(-)
Similarly to the previous commits in Morello kselftests, the tag should be: "kselftests/arm64: morello:"
Commits specific for a specific syscall or system, could even have: "kselftests/arm64: morello: mmap:"
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
Wrapper functions for system calls that are frequently invoked are added in order to improve ease of usability. Making use of the newly added support, mmap.c can be re-written in a manner that avoids directly interacting with syscalls. Required datatypes and constants have also been defined.
Additionally, dependency required to utilise signal > support has also been added.
Just a suggestion. This patch adds necessary support for mmap testcases, but doesn't specify what kind of testcases need this kind of support. The added dependency to utilise signal support looks a bit unrelated to creating the wrapper functions for syscalls. I would say leave this patch with only creating the wrapper functions, change the commit title to say specifically what the patch does (i.e. create wrapper functions for frequently invoked syscalls), optionally use the wrapper functions created in all the Morello kselftests (not only mmap.c), and move the dependency to the signal support to the patch that uses the signal support, which is "Add mmap() bounds check testcases" AFAICT.
Tudor
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
.../testing/selftests/arm64/morello/Makefile | 1 + .../selftests/arm64/morello/freestanding.h | 62 ++++++++++++++++++- tools/testing/selftests/arm64/morello/mmap.c | 11 ++-- 3 files changed, 67 insertions(+), 7 deletions(-)
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
The mmap() system call is expected to fail with -EOPNOTSUPP when the MAP_GROWSDOWN flag is passed. A testcase to verify this behaviour has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 2dd4ccdb0d2a..00d4c5ea9703 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -117,6 +117,17 @@ void syscall_mmap2(void) close(fd); } +/* test to verify mmap() behaviour when MAP_GROWSDOWN flag is specified */ +static void purecap_map_growsdown(void)
These tests are under arm64/morello, so they're expected to be purecap. No need to add "purecap" in the function names.
Tudor
+{
- void *addr;
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_GROWSDOWN;
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP);
+}
- TEST(test_syscall_mmap) { syscall_mmap();
@@ -127,9 +138,15 @@ TEST(test_syscall_mmap2) syscall_mmap2(); } +TEST(test_purecap_map_growsdown) +{
- purecap_map_growsdown();
+}
- int main(void) { test_syscall_mmap(); test_syscall_mmap2();
- test_purecap_map_growsdown(); return 0; }
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
Only valid owning capabilities have the ability to modify an address space by making use of the appropriate syscalls, any deviation from this method must result in immediate failure of the syscall. When a capability is created it is assigned the maximum permissions it may ever have by passing PROT_MAX(max_prot) as one of the prot flags. Any attempt to increase the permissions beyond this would result in failure of the syscall.
A test to validate the parmeters passed to address space management syscalls has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 76 ++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 00d4c5ea9703..184dcf4ddc92 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -128,6 +128,76 @@ static void purecap_map_growsdown(void) ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); }
nit: extra newline.
+/* test to validate parameters passed to address space management syscalls */ +static inline void purecap_param_check(void) +{
- void *addr, *addr_cap, *null_cap_addr;
- ptraddr_t address;
- int retval;
- int max_prot = PROT_READ | PROT_WRITE | PROT_EXEC;
- int prot = max_prot;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- /* generate null-derived capability to be used later*/
Space before /*.
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
mmap returns MAP_FAILED on error, not 0. If you want to check that mmap return value is valid, use: ASSERT_FALSE(IS_ERR_VALUE(addr))
- address = cheri_address_get(addr);
- null_cap_addr = (void *)(uintptr_t)address;
Having both variables "addr" and "address" is a bit confusing, and you don't need "address" as an intermediary. I'd say just do: null_cap_addr = (void *)(uintptr_t)cheri_address_get(addr);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* passing invalid capability to sycall */
typo in syscall.
Tudor
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- addr_cap = cheri_tag_clear(addr);
- retval = munmap(addr_cap, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* increase permission beyond the maximum prot specified for the mapping */
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(PROT_READ | PROT_WRITE) |
PROT_READ | PROT_WRITE, flags, -1, 0);- ASSERT_GT((unsigned long)addr, 0);
- retval = mprotect(addr, MMAP_SIZE, PROT_EXEC);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* max_prot has fewer permissions than prot */
- max_prot = (prot & ~(PROT_READ));
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0);
- ASSERT_EQ((unsigned long)addr, (unsigned long)-EINVAL);
- /* max_prot has more permissions than prot */
- max_prot = prot;
- prot = (prot & ~(PROT_WRITE));
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mprotect(addr, MMAP_SIZE, PROT_WRITE);
- ASSERT_NE(retval, -EINVAL);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* repeat positive max_prot test with null-derived capability */
- addr = mmap(null_cap_addr, MMAP_SIZE, PROT_MAX(max_prot) | prot,
flags | MAP_FIXED, -1, 0);- ASSERT_GT((unsigned long)addr, 0);
- retval = mprotect(addr, MMAP_SIZE, PROT_WRITE);
- ASSERT_EQ(retval, 0);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
+}
- TEST(test_syscall_mmap) { syscall_mmap();
@@ -143,10 +213,16 @@ TEST(test_purecap_map_growsdown) purecap_map_growsdown(); } +TEST(test_purecap_param_check) +{
- purecap_param_check();
+}
- int main(void) { test_syscall_mmap(); test_syscall_mmap2(); test_purecap_map_growsdown();
- test_purecap_param_check(); return 0; }
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
The length of the initial mapping using an mmap() call has the ability to be reduced in a subsequent mmap() call without first being unmapped. Whereas, trying to increase the length of the initial mapping would result in failure as the capability is not permitted to access a range that it does not own.
Address space management syscalls that manipulate a given mapping are restricted to the range owned by the capability. Any attempt to modify or access memory beyond this range will result in failure of syscall.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 132 +++++++++++++++++++ 1 file changed, 132 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 184dcf4ddc92..4a2a552d47dc 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -198,6 +198,132 @@ static inline void purecap_param_check(void) ASSERT_EQ(retval, 0); } +/* test to verify address space management syscall behaviour when capability
- range is modified */
Block comments use a trailing */ on a separate line. You have a few other instances in the following patches.
+static inline void purecap_range_check(void) +{
- void *addr, *ret;
- int retval;
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- unsigned char vec[MMAP_SIZE / PAGE_SIZE];
- /* mapping a smaller range at prev mmap addr in a subsequent mmap()
* call without first unmapping */- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- ret = mmap(addr, MMAP_SIZE_REDUCED, prot, flags | MAP_FIXED, -1, 0);
- ASSERT_GT((unsigned long)ret, 0);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE_REDUCED);
You probably want to munmap the whole MMAP_SIZE here. Otherwise there's a memory leak.
Tudor
- ASSERT_EQ(retval, 0);
- /* mapping a larger range at prev mmap addr in a subsequent mmap()
* call without first unmapping */- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- ret = mmap(addr, 2 * MMAP_SIZE, prot, flags | MAP_FIXED, -1, 0);
- ASSERT_EQ((unsigned long)ret, (unsigned long)-EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* positive madvise() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = madvise(addr, MMAP_SIZE, MADV_WILLNEED);
- ASSERT_NE(retval, -EINVAL);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative madvise() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = madvise(addr, MMAP_SIZE, MADV_NORMAL);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* positive mincore() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mincore(addr, MMAP_SIZE, vec);
- ASSERT_EQ(retval, 0);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative mincore() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mincore(addr, MMAP_SIZE, vec);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* positive mlock() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mlock(addr, MMAP_SIZE);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munlock(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative mlock() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mlock(addr, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* negative munlock() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- EXPECT_EQ(0, mlock2(addr, MMAP_SIZE_REDUCED, MLOCK_ONFAULT));
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munlock(addr, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* positive msync() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = msync(addr, MMAP_SIZE, MS_SYNC);
- ASSERT_EQ(retval, 0);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative msync() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = msync(addr, MMAP_SIZE, MS_SYNC);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
+}
- TEST(test_syscall_mmap) { syscall_mmap();
@@ -218,11 +344,17 @@ TEST(test_purecap_param_check) purecap_param_check(); } +TEST(test_purecap_range_check) +{
- purecap_range_check();
+}
- int main(void) { test_syscall_mmap(); test_syscall_mmap2(); test_purecap_map_growsdown(); test_purecap_param_check();
- test_purecap_range_check(); return 0; }
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
Reservations are contiguous ranges of virtual addresses that exactly match the bounds of an owning capability which is provided by the kernel. When an owning capability is passed to a syscall, its bounds are first verified against the existing reservations. If the bounds of the capability are found to not match any of the existing reservations, the syscall is expected to fail with a -ERESERVATION error code. Tests to verify this behaviour has been added.
An additional check to verify whether the representable length returned by the capability is greater than or equal to the bounds set for the capability has been added. Finally, signal handler has also been registered to handle invalid memory access.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 116 +++++++++++++++++++ 1 file changed, 116 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 4a2a552d47dc..c7f1ddb395c5 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -8,8 +8,13 @@ #include <linux/errno.h> #include <linux/fs.h> #include <linux/fcntl.h> +#include <linux/signal.h> +#include <asm/sigcontext.h> +#include <asm/siginfo.h> +#include <asm/ucontext.h> #include <cheriintrin.h> #include "freestanding.h" +#include "signal_common.h" #define MMAP_SIZE ((1ULL << 16) << 1) /* 64k x 2 */ @@ -19,6 +24,7 @@ #define PROBE_MODE_TOUCH 0x01 #define PROBE_MODE_VERIFY 0x02 +static volatile unsigned int signal_status; static inline int probe_mem_range(void *addr, size_t size, int mode) { @@ -37,6 +43,37 @@ static inline int probe_mem_range(void *addr, size_t size, int mode) return 0; } +static void purecap_sigsegv_handler(int n, siginfo_t *si, void *data) +{
- struct ucontext *uc = (struct ucontext *)data;
- ASSERT_EQ(si->si_signo, n);
- TH_LOG("Signal (%d) occurred", n);
- uc->uc_mcontext.pc += 4;
- signal_status = 1;
+}
+static void raise_sigsegv(void) +{
- void *addr;
- long value;
- int retval;
- unsigned int *invalid_addr;
You can leave it "void *addr", so that you don't need to do a cast below.
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- invalid_addr = (unsigned int *)addr;
You don't need this assignment I think.
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- invalid_addr = (unsigned int *)(addr + MMAP_SIZE);
- asm volatile("ldr %w0, [%1]" : "=r" (value) : "C" (invalid_addr) : "memory");
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
+}
- /* Simple test to check our ability to create a new anonymous mapping
*/
- in the virtual address space of the calling process
@@ -324,6 +361,79 @@ static inline void purecap_range_check(void) ASSERT_EQ(retval, 0); } +/* test to verify mmap() behaviour when capability bounds are modified */ +static inline void purecap_mmap_bounds_check(void) +{
- int retval;
- ptraddr_t address;
- size_t res_length;
- void *addr, *addr_cap, *ddc_cap, *ddc_value, *new_addr, *null_cap_addr;
addr is a valid capability throughout this test, so having both addr and addr_cap is a bit confusing.
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- struct sigaction sa;
- /* generate a null-derived capability to be used later */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- address = cheri_address_get(addr);
- null_cap_addr = (void *)(uintptr_t)address;
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- /* bounds of capability is increased by using root capability of the DDC
* register */- ddc_value = cheri_ddc_get();
Don't need ddc_value and ddc_cap. Just use only one variable: ddc_cap.
- ddc_cap = cheri_address_set(ddc_value, cheri_address_get(addr));
- ddc_cap = cheri_bounds_set(ddc_value, 2 * MMAP_SIZE);
Probably you meant cheri_bounds_set(ddc_cap,...) 🙂. Same on the line below.
- ddc_cap = cheri_perms_and(ddc_value, cheri_perms_get(addr));
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(ddc_cap, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
Is there a particular reason why you munmap the same region twice?
- /* bounds of capability returned by mmap() is reduced, followed by a
* representable length check */- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- addr_cap = cheri_bounds_set(addr, MMAP_SIZE_REDUCED);
- res_length = cheri_representable_length((unsigned long)addr_cap);
cheri_representable_length(size_t len) returns the length that a capability would have after using cheri_bounds_set to set the length to len (assuming appropriate alignment of the base). It's not addr_cap the argument that you want to pass to it AFAICT, maybe like this:
res_length = cheri_representable_length(cheri_length_get(addr_cap));
- EXPECT_GE(res_length, MMAP_SIZE_REDUCED);
- retval = munmap(addr_cap, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* overlapping mappings produce a reservation error */
- addr = mmap(null_cap_addr, MMAP_SIZE, prot, flags | MAP_FIXED, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- new_addr = mmap(null_cap_addr + MMAP_SIZE_REDUCED, MMAP_SIZE_REDUCED,
prot, flags | MAP_FIXED, -1, 0);- ASSERT_EQ((unsigned long)new_addr, (unsigned long)-ERESERVATION);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* register a signal handler to catch invalid memory access */
I really like your comments for each mechanism that you're testing. I would suggest adding to this comment what the code below actually tests, and then why you need to register a signal handler.
- sigemptyset(&sa.sa_mask);
- sa.sa_handler = (sighandler_t)(void *)purecap_sigsegv_handler;
- sa.sa_flags = SA_SIGINFO;
- sigaction(SIGSEGV, &sa, NULL);
- signal_status = 0;
- raise_sigsegv();
- ASSERT_TRUE(signal_status);
- TH_LOG("Segmenation fault caused by invalid memory access has been handled.");
Typo in Segmentation.
Tudor
+}
- TEST(test_syscall_mmap) { syscall_mmap();
@@ -349,6 +459,11 @@ TEST(test_purecap_range_check) purecap_range_check(); } +TEST(test_purecap_mmap_bounds_check) +{
- purecap_mmap_bounds_check();
+}
- int main(void) { test_syscall_mmap();
@@ -356,5 +471,6 @@ int main(void) test_purecap_map_growsdown(); test_purecap_param_check(); test_purecap_range_check();
- test_purecap_mmap_bounds_check(); return 0; }
On 18/07/2023 17:43, Tudor Cretu wrote:
+ addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0);
mmap returns MAP_FAILED on error, not 0. If you want to check that mmap return value is valid, use: ASSERT_FALSE(IS_ERR_VALUE(addr))
To be clear: the libc mmap() wrapper returns MAP_FAILED (-1), but our wrappers just make a raw syscall, so the return value is directly the error (if one occurs).
Tudor is right that there is a problem with this approach: the return value interpreted as *unsigned* long is always going to be greater than 0. Using IS_ERR_VALUE() like in mmap_verified() is a good option.
In fact, I wonder if we shouldn't simply use mmap_verified() whenever we expect mmap() to succeed, as that function already takes care of the standard checks (return value, tag, permissions). Maybe we could add another helper (in this file) for the cases where we expect mmap() to fail, adding an argument specifying the expected error. I'm suggesting this as the casts in ASSERT_EQ() in that case are rather cumbersome.
Kevin
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
Wrapper functions for system calls that are frequently invoked are added in order to improve ease of usability. Making use of the newly added
Nit: either "ease of use", or "usability" :)
support, mmap.c can be re-written in a manner that avoids directly interacting with syscalls. Required datatypes and constants have also been defined. Additionally, dependency required to utilise signal support has also been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
.../testing/selftests/arm64/morello/Makefile | 1 + .../selftests/arm64/morello/freestanding.h | 62 ++++++++++++++++++- tools/testing/selftests/arm64/morello/mmap.c | 11 ++-- 3 files changed, 67 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/Makefile b/tools/testing/selftests/arm64/morello/Makefile index ecfa6fc5e989..d2e100a5aadc 100644 --- a/tools/testing/selftests/arm64/morello/Makefile +++ b/tools/testing/selftests/arm64/morello/Makefile @@ -43,3 +43,4 @@ $(OUTPUT)/%: $(OUTPUT)/%.o $(OUTPUT)/freestanding_start.o $(OUTPUT)/freestanding $(OUTPUT)/signal: $(OUTPUT)/signal_common.o $(OUTPUT)/clone: $(OUTPUT)/signal_common.o +$(OUTPUT)/mmap: $(OUTPUT)/signal_common.o
If I understand correctly, this is required due to patch 5. I agree with Tudor, I would rather move this diff to that patch, as this new dependency is otherwise rather mysterious.
diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 2beb52eafae1..3badf163569c 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -21,6 +21,9 @@ typedef __kernel_timer_t timer_t; typedef __kernel_clockid_t clockid_t; typedef __kernel_uid_t uid_t; typedef __kernel_mode_t mode_t; +typedef __kernel_off_t off_t; +typedef __kernel_key_t key_t;
I don't see key_t getting used in any patch.
#ifndef __clang__ typedef __uintcap_t uintcap_t; #endif @@ -29,6 +32,7 @@ typedef __uintcap_t uintcap_t; #define INT_MAX __INT_MAX__ #endif +#define PAGE_SIZE 4096
We should avoid hardcoding the page size to 4K. We only fully support what is in the transitional defconfig at the moment, i.e. 4K pages, but in principle other page sizes should just work too. The initial mmap tests do hardcode sizes, but this is safe as they are set to multiples of 64K (the maximum page size).
We could either set PAGE_SIZE as 64K, which is safe but may be limiting to test certain edge cases for the mmap syscalls. Or we could use the real page size - it does not seem to be necessary right now, but maybe later.
The canonical way to obtain the page size is sysconf(_SC_PAGESIZE). That said it may not be worth implementing sysconf() just for that. The way sysconf() itself obtains the page size is by reading the AT_PAGESZ auxiliary value. We could call some function in _start that finds this value and sets a global variable to it. The function could be written either in C or assembly, in that case both are about as simple.
Kevin
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
+TEST(test_purecap_map_growsdown) +{
- purecap_map_growsdown();
I don't think there's much point in having every testcase call its own separate helper and do nothing else. I see that the two existing testcases are written like that, I'm not sure why. I'd favour removing the indirection in the existing testcases (separate patch), and writing the new test code directly in the testcase functions.
Something else that would be nice to change in the existing code and the new tests is the use of "addr" to name pointer variables. Unfortunately it is very common in the kernel and the "official" prototypes for mmap syscalls, but it should be avoided for new code, especially when manipulating both addresses and pointers.
Kevin
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
Only valid owning capabilities have the ability to modify an address space by making use of the appropriate syscalls, any deviation from this method must result in immediate failure of the syscall. When a capability is created it is assigned the maximum permissions it may ever have by passing PROT_MAX(max_prot) as one of the prot flags. Any attempt to increase the permissions beyond this would result in failure of the syscall.
A test to validate the parmeters passed to address space management syscalls has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 76 ++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 00d4c5ea9703..184dcf4ddc92 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -128,6 +128,76 @@ static void purecap_map_growsdown(void) ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); }
+/* test to validate parameters passed to address space management syscalls */ +static inline void purecap_param_check(void) +{
- void *addr, *addr_cap, *null_cap_addr;
- ptraddr_t address;
- int retval;
- int max_prot = PROT_READ | PROT_WRITE | PROT_EXEC;
- int prot = max_prot;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- /* generate null-derived capability to be used later*/
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- address = cheri_address_get(addr);
- null_cap_addr = (void *)(uintptr_t)address;
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* passing invalid capability to sycall */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- addr_cap = cheri_tag_clear(addr);
addr_cap is a strange name, invalid_cap maybe? Alternatively you don't really need a temporary at all here, you could directly pass cheri_tag_clear(addr) to munmap().
- retval = munmap(addr_cap, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* increase permission beyond the maximum prot specified for the mapping */
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(PROT_READ | PROT_WRITE) |
PROT_READ | PROT_WRITE, flags, -1, 0);
Passing R|W as prot already implies PROT_MAX(R|W), so there is no need to use PROT_MAX() for that test.
- ASSERT_GT((unsigned long)addr, 0);
- retval = mprotect(addr, MMAP_SIZE, PROT_EXEC);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* max_prot has fewer permissions than prot */
- max_prot = (prot & ~(PROT_READ));
Reusing the value of prot and/or prot_max set by previous cases (or at the beginning) is not very readable. I would rather have every case set prot and prot_max explicitly. Feel free to define a macro set to R|W|X to save some typing.
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0);
- ASSERT_EQ((unsigned long)addr, (unsigned long)-EINVAL);
- /* max_prot has more permissions than prot */
- max_prot = prot;
- prot = (prot & ~(PROT_WRITE));
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mprotect(addr, MMAP_SIZE, PROT_WRITE);
- ASSERT_NE(retval, -EINVAL);
Why wouldn't we check that the call actually succeeded? I think I've noticed this pattern in other patches too. In general it is not enough to check that the return value is not a particular error, we need to check that it is valid (or a particular error).
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* repeat positive max_prot test with null-derived capability */
- addr = mmap(null_cap_addr, MMAP_SIZE, PROT_MAX(max_prot) | prot,
flags | MAP_FIXED, -1, 0);
We should avoid relying on this address that has been previously obtained from mmap() being available. That's because it is unspecified when reservations are destroyed (see the "Lifetime of reservations" note in the spec), meaning that the reservation for that initial mmap() may still exist. The MAP_FIXED + address use-case is pretty limited, but I suppose it's still worth testing. What we could do is use a hardcoded address that is not going to be ever mapped. That's not so easy to come by, but I think the very beginning of the address space is quite safe (not 0x0 though, which cannot be mapped). Maybe at address == page_size?
Kevin
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
The length of the initial mapping using an mmap() call has the ability to be reduced in a subsequent mmap() call without first being unmapped. Whereas, trying to increase the length of the initial mapping would
I have trouble making sense of this: mappings can only be enlarged or shrunk using mremap(), and these tests are not about mremap(). Rather using mmap(MAP_FIXED) in the middle of an existing mapping overwrites part of it.
result in failure as the capability is not permitted to access a range that it does not own.
Address space management syscalls that manipulate a given mapping are restricted to the range owned by the capability. Any attempt to modify or access memory beyond this range will result in failure of syscall.
It would be good to clarify that the mm syscalls do not access memory, but rather manage mappings.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 132 +++++++++++++++++++ 1 file changed, 132 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 184dcf4ddc92..4a2a552d47dc 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -198,6 +198,132 @@ static inline void purecap_param_check(void) ASSERT_EQ(retval, 0); } +/* test to verify address space management syscall behaviour when capability
- range is modified */
+static inline void purecap_range_check(void) +{
- void *addr, *ret;
- int retval;
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- unsigned char vec[MMAP_SIZE / PAGE_SIZE];
- /* mapping a smaller range at prev mmap addr in a subsequent mmap()
* call without first unmapping */- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- ret = mmap(addr, MMAP_SIZE_REDUCED, prot, flags | MAP_FIXED, -1, 0);
- ASSERT_GT((unsigned long)ret, 0);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* mapping a larger range at prev mmap addr in a subsequent mmap()
* call without first unmapping */- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- ret = mmap(addr, 2 * MMAP_SIZE, prot, flags | MAP_FIXED, -1, 0);
- ASSERT_EQ((unsigned long)ret, (unsigned long)-EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* positive madvise() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = madvise(addr, MMAP_SIZE, MADV_WILLNEED);
- ASSERT_NE(retval, -EINVAL);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative madvise() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = madvise(addr, MMAP_SIZE, MADV_NORMAL);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* positive mincore() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mincore(addr, MMAP_SIZE, vec);
- ASSERT_EQ(retval, 0);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative mincore() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mincore(addr, MMAP_SIZE, vec);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* positive mlock() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mlock(addr, MMAP_SIZE);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munlock(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative mlock() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mlock(addr, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* negative munlock() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- EXPECT_EQ(0, mlock2(addr, MMAP_SIZE_REDUCED, MLOCK_ONFAULT));
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munlock(addr, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* positive msync() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = msync(addr, MMAP_SIZE, MS_SYNC);
- ASSERT_EQ(retval, 0);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative msync() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = msync(addr, MMAP_SIZE, MS_SYNC);
- ASSERT_EQ(retval, -EINVAL);
These madvise/mincore/mlock/munlock/msync testcases will indeed fail in PCuABI (and only in PCuABI), but that is purely because the length they are passed is greater than the length of addr, not because the mapping is smaller. It would be clearer to reduce the bounds of addr instead of passing a length that is larger than the mapping. Also, given that the tests are the same regardless of the particular syscall, it would be good to refactor them to reduce duplication.
Kevin
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
Reservations are contiguous ranges of virtual addresses that exactly match the bounds of an owning capability which is provided by the kernel. When an owning capability is passed to a syscall, its bounds are first verified against the existing reservations. If the bounds of the capability are found to not match any of the existing reservations, the syscall is expected to fail with a -ERESERVATION error code. Tests to verify this behaviour has been added.
An additional check to verify whether the representable length returned by the capability is greater than or equal to the bounds set for the capability has been added. Finally, signal handler has also been registered to handle invalid memory access.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 116 +++++++++++++++++++ 1 file changed, 116 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 4a2a552d47dc..c7f1ddb395c5 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -8,8 +8,13 @@ #include <linux/errno.h> #include <linux/fs.h> #include <linux/fcntl.h> +#include <linux/signal.h> +#include <asm/sigcontext.h> +#include <asm/siginfo.h>
Not needed as it is already included from <linux/signal.h>.
+#include <asm/ucontext.h> #include <cheriintrin.h> #include "freestanding.h" +#include "signal_common.h" #define MMAP_SIZE ((1ULL << 16) << 1) /* 64k x 2 */ @@ -19,6 +24,7 @@ #define PROBE_MODE_TOUCH 0x01 #define PROBE_MODE_VERIFY 0x02 +static volatile unsigned int signal_status; static inline int probe_mem_range(void *addr, size_t size, int mode) { @@ -37,6 +43,37 @@ static inline int probe_mem_range(void *addr, size_t size, int mode) return 0; } +static void purecap_sigsegv_handler(int n, siginfo_t *si, void *data) +{
- struct ucontext *uc = (struct ucontext *)data;
- ASSERT_EQ(si->si_signo, n);
- TH_LOG("Signal (%d) occurred", n);
- uc->uc_mcontext.pc += 4;
- signal_status = 1;
+}
+static void raise_sigsegv(void) +{
- void *addr;
- long value;
- int retval;
- unsigned int *invalid_addr;
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- invalid_addr = (unsigned int *)addr;
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- invalid_addr = (unsigned int *)(addr + MMAP_SIZE);
- asm volatile("ldr %w0, [%1]" : "=r" (value) : "C" (invalid_addr) : "memory");
This test will fail in the same way regardless of the ABI - trying to access unmapped memory always results in a segfault.
It might be worth highlighting that the kselftests we are adding in this directory should all be Morello-specific. Generic Linux ABI tests belong mostly to LTP.
One PCuABI behaviour that is however worth testing is that LoadCap / StoreCap are not provided when creating a shared mapping. No need to trigger a segfault though, as we can simply check the capability permissions.
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
+}
/* Simple test to check our ability to create a new anonymous mapping
- in the virtual address space of the calling process
*/ @@ -324,6 +361,79 @@ static inline void purecap_range_check(void) ASSERT_EQ(retval, 0); } +/* test to verify mmap() behaviour when capability bounds are modified */ +static inline void purecap_mmap_bounds_check(void) +{
- int retval;
- ptraddr_t address;
- size_t res_length;
- void *addr, *addr_cap, *ddc_cap, *ddc_value, *new_addr, *null_cap_addr;
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- struct sigaction sa;
- /* generate a null-derived capability to be used later */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- address = cheri_address_get(addr);
- null_cap_addr = (void *)(uintptr_t)address;
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- /* bounds of capability is increased by using root capability of the DDC
* register */- ddc_value = cheri_ddc_get();
DDC is null in purecap. There is no way to increase the bounds or permissions of any root capability handed by the kernel. This happens to work right now because we do not nullify DDC yet, but we will in due course.
I suppose the main question is: what PCuABI behaviour are we trying to test here? I'm under the impression that the operation tested here is simply not allowed in PCuABI.
- ddc_cap = cheri_address_set(ddc_value, cheri_address_get(addr));
- ddc_cap = cheri_bounds_set(ddc_value, 2 * MMAP_SIZE);
- ddc_cap = cheri_perms_and(ddc_value, cheri_perms_get(addr));
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(ddc_cap, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* bounds of capability returned by mmap() is reduced, followed by a
* representable length check */- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- addr_cap = cheri_bounds_set(addr, MMAP_SIZE_REDUCED);
- res_length = cheri_representable_length((unsigned long)addr_cap);
- EXPECT_GE(res_length, MMAP_SIZE_REDUCED);
I'm not sure in what way this is testing mmap()? It looks like this is asserting that MMAP_SIZE_REDUCED is a representable length for that particular base, which is not something that is up to the kernel.
- retval = munmap(addr_cap, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* overlapping mappings produce a reservation error */
- addr = mmap(null_cap_addr, MMAP_SIZE, prot, flags | MAP_FIXED, -1, 0);
Here as well we should avoid assuming that the previous reservation initially created at this address has now been destroyed. Fortunately I don't think we need a particular address at all here, so we could just pass NULL and adjust the second mmap() call according to the returned address.
Kevin
- ASSERT_GT((unsigned long)addr, 0);
- new_addr = mmap(null_cap_addr + MMAP_SIZE_REDUCED, MMAP_SIZE_REDUCED,
prot, flags | MAP_FIXED, -1, 0);- ASSERT_EQ((unsigned long)new_addr, (unsigned long)-ERESERVATION);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
As mremap() doesn't take prot flags into consideration, the permissions
Better remove "into consideration" - mremap() simply doesn't take a prot argument (otherwise it sounds like it takes one and ignores it).
of the original address are retained. If the permissions of the new
"address" -> "pointer" (or maybe, more to the point, capability)
We need to be quite precise on the pointer/address terminology, otherwise things get confusing pretty quickly. The fact that the arguments are "officially" called old_addr / new_addr is certainly not helping us in that respect...
address do not match the set of permissions of the old address, the syscall fails with a -EINVAL error code. A test to verify this behaviour has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 68 ++++++++++++++++++++ 1 file changed, 68 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index aa552c8affba..3b8e99aafedc 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -165,6 +165,68 @@ static void purecap_map_growsdown(void) ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); } +/* test to verify mremap() behaviour when permissions are modified */ +static void purecap_mremap_perms_check(void) +{
- void *old_addr, *new_addr, *ret;
- int retval;
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- size_t old_perms, new_perms;
- /* permissions of capability returned by mremap must match the
* permissions returned by the original mapping */- old_addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)old_addr, 0);
- old_perms = cheri_perms_get(old_addr);
- new_addr = mremap(old_addr, MMAP_SIZE_REDUCED, MMAP_SIZE,
MREMAP_MAYMOVE, 0);- ASSERT_GT((unsigned long)new_addr, 0);
- new_perms = cheri_perms_get(new_addr);
- ASSERT_EQ(old_perms, new_perms);
- EXPECT_EQ(0, probe_mem_range(new_addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(new_addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* remapping to a new_addr having reduced permissions from old_addr */
- old_addr = mmap(NULL, MMAP_SIZE_REDUCED, PROT_MAX(prot | PROT_EXEC) |
prot | PROT_EXEC, flags, -1, 0);- ASSERT_GT((unsigned long)old_addr, 0);
- new_addr = mmap(NULL, MMAP_SIZE, PROT_MAX(prot) | prot, flags, -1, 0);
As a rule, PROT_MAX(prot) | prot is exactly equivalent to just prot.
- ASSERT_GT((unsigned long)new_addr, 0);
- retval = munmap(new_addr, MMAP_SIZE);
- ASSERT_EQ(0, retval);
- ret = mremap(old_addr, MMAP_SIZE_REDUCED, MMAP_SIZE,
MREMAP_MAYMOVE | MREMAP_FIXED, new_addr);
ret is unchecked here, and that's unfortunate because this mremap() will fail: either the reservation at new_addr is destroyed and new_addr becomes an invalid owning capability, or the reservation has been destroyed yet - also invalid as it is forbidden to reuse unmapped space in a reservation.
The safe thing we can do is to overwrite the mapping directly - it should be as simple as removing the munmap() above.
- EXPECT_EQ(0, probe_mem_range(ret, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(ret, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* remapping to new_addr having increased permissions from old_addr */
- old_addr = mmap(NULL, MMAP_SIZE_REDUCED, PROT_MAX(prot) | prot, flags,
-1, 0);- ASSERT_GT((unsigned long)old_addr, 0);
- new_addr = mmap(NULL, MMAP_SIZE, PROT_MAX(prot | PROT_EXEC) | prot |
PROT_EXEC, flags, -1, 0);- ASSERT_GT((unsigned long)new_addr, 0);
- retval = munmap(new_addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- ret = mremap(old_addr, MMAP_SIZE_REDUCED, MMAP_SIZE,
MREMAP_MAYMOVE | MREMAP_FIXED, new_addr);
This would fail anyway because the mapping at new_addr has already been removed (same rationale as above). We should try to avoid relying on the prioritisation of error codes, so let's move the munmap() after this mremap().
Kevin
- ASSERT_EQ((unsigned long)ret, (unsigned long)-EINVAL);
On 18/07/2023 17:43, Tudor Cretu wrote:
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
Syscalls operating on memory mappings manage their address space via owning capabilities. They must adhere to a certain set of rules[1] in order to ensure memory safety. Address space management syscalls are only allowed to manipulate mappings that are within the range of the owning capability and have the appropriate permissions. Tests to vailidate the parameters being passed to the syscall, check its bounds, range as well as permissions have been added. Additionally, a signal handler has been registered to handle invalid memory access. Finally, as certain flags and syscalls conflict with the reservation model or lack implementation, a check to verify appropriate handling of the same has also been added.
Great series! I am very impressed by the comprehensiveness of the tests and the quality of the code is generally very good! Well done!
Agreed, overall the testcases make sense and cover a good chunk of the rules. We will probably need some more for certain corner cases, but the initial goal is not to be comprehensive anyway - having tests at all is already a big improvement!
I have only a few notes/comments. If you see a comment, it might apply to multiple patches.
Likewise - quite a few of my comments apply to similar code across the series.
Kevin
On 7/18/23 21:13, Tudor Cretu wrote:
Similarly to the previous commits in Morello kselftests, the tag should be: "kselftests/arm64: morello:"
Commits specific for a specific syscall or system, could even have: "kselftests/arm64: morello: mmap:"
I'll change it accordingly.
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
Wrapper functions for system calls that are frequently invoked are added in order to improve ease of usability. Making use of the newly added support, mmap.c can be re-written in a manner that avoids directly interacting with syscalls. Required datatypes and constants have also been defined.
Additionally, dependency required to utilise signal > support has also been added.
Just a suggestion. This patch adds necessary support for mmap testcases, but doesn't specify what kind of testcases need this kind of support. The added dependency to utilise signal support looks a bit unrelated to creating the wrapper functions for syscalls. I would say leave this patch with only creating the wrapper functions, change the commit title to say specifically what the patch does (i.e. create wrapper functions for frequently invoked syscalls), optionally use the wrapper functions created in all the Morello kselftests (not only mmap.c), and move the dependency to the signal support to the patch that uses the signal support, which is "Add mmap() bounds check testcases" AFAICT.
Tudor
I see how that would be a lot clearer, I'll make the changes.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
.../testing/selftests/arm64/morello/Makefile | 1 + .../selftests/arm64/morello/freestanding.h | 62 ++++++++++++++++++- tools/testing/selftests/arm64/morello/mmap.c | 11 ++-- 3 files changed, 67 insertions(+), 7 deletions(-)
On 7/20/23 13:10, Kevin Brodsky wrote:
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
Wrapper functions for system calls that are frequently invoked are added in order to improve ease of usability. Making use of the newly added
Nit: either "ease of use", or "usability" :)
I'll change it.
support, mmap.c can be re-written in a manner that avoids directly interacting with syscalls. Required datatypes and constants have also been defined. Additionally, dependency required to utilise signal support has also been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
.../testing/selftests/arm64/morello/Makefile | 1 + .../selftests/arm64/morello/freestanding.h | 62 ++++++++++++++++++- tools/testing/selftests/arm64/morello/mmap.c | 11 ++-- 3 files changed, 67 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/Makefile b/tools/testing/selftests/arm64/morello/Makefile index ecfa6fc5e989..d2e100a5aadc 100644 --- a/tools/testing/selftests/arm64/morello/Makefile +++ b/tools/testing/selftests/arm64/morello/Makefile @@ -43,3 +43,4 @@ $(OUTPUT)/%: $(OUTPUT)/%.o $(OUTPUT)/freestanding_start.o $(OUTPUT)/freestanding $(OUTPUT)/signal: $(OUTPUT)/signal_common.o $(OUTPUT)/clone: $(OUTPUT)/signal_common.o +$(OUTPUT)/mmap: $(OUTPUT)/signal_common.o
If I understand correctly, this is required due to patch 5. I agree with Tudor, I would rather move this diff to that patch, as this new dependency is otherwise rather mysterious.
Yes, it's required due to patch 5. I'll move it to that patch.
diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 2beb52eafae1..3badf163569c 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -21,6 +21,9 @@ typedef __kernel_timer_t timer_t; typedef __kernel_clockid_t clockid_t; typedef __kernel_uid_t uid_t; typedef __kernel_mode_t mode_t; +typedef __kernel_off_t off_t; +typedef __kernel_key_t key_t;
I don't see key_t getting used in any patch.
I'll remove it.
- #ifndef __clang__ typedef __uintcap_t uintcap_t; #endif
@@ -29,6 +32,7 @@ typedef __uintcap_t uintcap_t; #define INT_MAX __INT_MAX__ #endif +#define PAGE_SIZE 4096
We should avoid hardcoding the page size to 4K. We only fully support what is in the transitional defconfig at the moment, i.e. 4K pages, but in principle other page sizes should just work too. The initial mmap tests do hardcode sizes, but this is safe as they are set to multiples of 64K (the maximum page size).
We could either set PAGE_SIZE as 64K, which is safe but may be limiting to test certain edge cases for the mmap syscalls. Or we could use the real page size - it does not seem to be necessary right now, but maybe later.
The canonical way to obtain the page size is sysconf(_SC_PAGESIZE). That said it may not be worth implementing sysconf() just for that. The way sysconf() itself obtains the page size is by reading the AT_PAGESZ auxiliary value. We could call some function in _start that finds this value and sets a global variable to it. The function could be written either in C or assembly, in that case both are about as simple.
I'll look into this and change it accordingly.
Kevin
On 7/18/23 21:13, Tudor Cretu wrote:
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
The mmap() system call is expected to fail with -EOPNOTSUPP when the MAP_GROWSDOWN flag is passed. A testcase to verify this behaviour has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 2dd4ccdb0d2a..00d4c5ea9703 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -117,6 +117,17 @@ void syscall_mmap2(void) close(fd); } +/* test to verify mmap() behaviour when MAP_GROWSDOWN flag is specified */ +static void purecap_map_growsdown(void)
These tests are under arm64/morello, so they're expected to be purecap. No need to add "purecap" in the function names.
Tudor
I'll rename it.
+{ + void *addr; + int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_GROWSDOWN;
+ addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); +}
TEST(test_syscall_mmap) { syscall_mmap(); @@ -127,9 +138,15 @@ TEST(test_syscall_mmap2) syscall_mmap2(); } +TEST(test_purecap_map_growsdown) +{ + purecap_map_growsdown(); +}
int main(void) { test_syscall_mmap(); test_syscall_mmap2(); + test_purecap_map_growsdown(); return 0; }
On 7/20/23 13:10, Kevin Brodsky wrote:
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
+TEST(test_purecap_map_growsdown) +{
- purecap_map_growsdown();
I don't think there's much point in having every testcase call its own separate helper and do nothing else. I see that the two existing testcases are written like that, I'm not sure why. I'd favour removing the indirection in the existing testcases (separate patch), and writing the new test code directly in the testcase functions.
Something else that would be nice to change in the existing code and the new tests is the use of "addr" to name pointer variables. Unfortunately it is very common in the kernel and the "official" prototypes for mmap syscalls, but it should be avoided for new code, especially when manipulating both addresses and pointers.
Kevin
I'll remove the helper functions and rename the "addr" variable to ptr instead. I could create a cleanup patch with these changes for the first two testcases.
On 7/18/23 21:13, Tudor Cretu wrote:
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
Only valid owning capabilities have the ability to modify an address space by making use of the appropriate syscalls, any deviation from this method must result in immediate failure of the syscall. When a capability is created it is assigned the maximum permissions it may ever have by passing PROT_MAX(max_prot) as one of the prot flags. Any attempt to increase the permissions beyond this would result in failure of the syscall.
A test to validate the parmeters passed to address space management syscalls has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 76 ++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 00d4c5ea9703..184dcf4ddc92 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -128,6 +128,76 @@ static void purecap_map_growsdown(void) ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); }
nit: extra newline.
I'll remove it.
+/* test to validate parameters passed to address space management syscalls */ +static inline void purecap_param_check(void) +{ + void *addr, *addr_cap, *null_cap_addr; + ptraddr_t address; + int retval; + int max_prot = PROT_READ | PROT_WRITE | PROT_EXEC; + int prot = max_prot; + int flags = MAP_PRIVATE | MAP_ANONYMOUS;
+ /* generate null-derived capability to be used later*/
Space before /*.
I'll add it.
+ addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0);
mmap returns MAP_FAILED on error, not 0. If you want to check that mmap return value is valid, use: ASSERT_FALSE(IS_ERR_VALUE(addr))
I'll change the error handling portion for all.
+ address = cheri_address_get(addr); + null_cap_addr = (void *)(uintptr_t)address;
Having both variables "addr" and "address" is a bit confusing, and you don't need "address" as an intermediary. I'd say just do: null_cap_addr = (void *)(uintptr_t)cheri_address_get(addr);
This seems better, I'll correct it.
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* passing invalid capability to sycall */
typo in syscall.
Ah my bad, I'll correct it.
Tudor
+ addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + addr_cap = cheri_tag_clear(addr); + retval = munmap(addr_cap, MMAP_SIZE); + ASSERT_EQ(retval, -EINVAL);
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* increase permission beyond the maximum prot specified for the mapping */ + addr = mmap(NULL, MMAP_SIZE, PROT_MAX(PROT_READ | PROT_WRITE) | + PROT_READ | PROT_WRITE, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mprotect(addr, MMAP_SIZE, PROT_EXEC); + ASSERT_EQ(retval, -EINVAL);
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* max_prot has fewer permissions than prot */ + max_prot = (prot & ~(PROT_READ)); + addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0); + ASSERT_EQ((unsigned long)addr, (unsigned long)-EINVAL);
+ /* max_prot has more permissions than prot */ + max_prot = prot; + prot = (prot & ~(PROT_WRITE)); + addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0);
+ retval = mprotect(addr, MMAP_SIZE, PROT_WRITE); + ASSERT_NE(retval, -EINVAL); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* repeat positive max_prot test with null-derived capability */ + addr = mmap(null_cap_addr, MMAP_SIZE, PROT_MAX(max_prot) | prot, + flags | MAP_FIXED, -1, 0); + ASSERT_GT((unsigned long)addr, 0);
+ retval = mprotect(addr, MMAP_SIZE, PROT_WRITE); + ASSERT_EQ(retval, 0);
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); +}
TEST(test_syscall_mmap) { syscall_mmap(); @@ -143,10 +213,16 @@ TEST(test_purecap_map_growsdown) purecap_map_growsdown(); } +TEST(test_purecap_param_check) +{ + purecap_param_check(); +}
int main(void) { test_syscall_mmap(); test_syscall_mmap2(); test_purecap_map_growsdown(); + test_purecap_param_check(); return 0; }
On 7/19/23 20:57, Kevin Brodsky wrote:
On 18/07/2023 17:43, Tudor Cretu wrote:
+ addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0);
mmap returns MAP_FAILED on error, not 0. If you want to check that mmap return value is valid, use: ASSERT_FALSE(IS_ERR_VALUE(addr))
To be clear: the libc mmap() wrapper returns MAP_FAILED (-1), but our wrappers just make a raw syscall, so the return value is directly the error (if one occurs).
Tudor is right that there is a problem with this approach: the return value interpreted as *unsigned* long is always going to be greater than 0. Using IS_ERR_VALUE() like in mmap_verified() is a good option.
In fact, I wonder if we shouldn't simply use mmap_verified() whenever we expect mmap() to succeed, as that function already takes care of the standard checks (return value, tag, permissions). Maybe we could add another helper (in this file) for the cases where we expect mmap() to fail, adding an argument specifying the expected error. I'm suggesting this as the casts in ASSERT_EQ() in that case are rather cumbersome.
I'm assuming you meant we *should* use mmap_verified() whenever mmap is expected to succeed?
Kevin
On 7/18/23 21:13, Tudor Cretu wrote:
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
The length of the initial mapping using an mmap() call has the ability to be reduced in a subsequent mmap() call without first being unmapped. Whereas, trying to increase the length of the initial mapping would result in failure as the capability is not permitted to access a range that it does not own.
Address space management syscalls that manipulate a given mapping are restricted to the range owned by the capability. Any attempt to modify or access memory beyond this range will result in failure of syscall.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 132 +++++++++++++++++++ 1 file changed, 132 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 184dcf4ddc92..4a2a552d47dc 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -198,6 +198,132 @@ static inline void purecap_param_check(void) ASSERT_EQ(retval, 0); } +/* test to verify address space management syscall behaviour when capability
- range is modified */
Block comments use a trailing */ on a separate line. You have a few other instances in the following patches.
I'll correct it.
+static inline void purecap_range_check(void) +{ + void *addr, *ret; + int retval; + int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS; + unsigned char vec[MMAP_SIZE / PAGE_SIZE];
+ /* mapping a smaller range at prev mmap addr in a subsequent mmap() + * call without first unmapping */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + ret = mmap(addr, MMAP_SIZE_REDUCED, prot, flags | MAP_FIXED, -1, 0); + ASSERT_GT((unsigned long)ret, 0); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + retval = munmap(addr, MMAP_SIZE_REDUCED);
You probably want to munmap the whole MMAP_SIZE here. Otherwise there's a memory leak.
I'll change that.
Tudor
+ ASSERT_EQ(retval, 0);
+ /* mapping a larger range at prev mmap addr in a subsequent mmap() + * call without first unmapping */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + ret = mmap(addr, 2 * MMAP_SIZE, prot, flags | MAP_FIXED, -1, 0); + ASSERT_EQ((unsigned long)ret, (unsigned long)-EINVAL);
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* positive madvise() range test */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = madvise(addr, MMAP_SIZE, MADV_WILLNEED); + ASSERT_NE(retval, -EINVAL); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* negative madvise() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = madvise(addr, MMAP_SIZE, MADV_NORMAL); + ASSERT_EQ(retval, -EINVAL);
+ retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0);
+ /* positive mincore() range test */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mincore(addr, MMAP_SIZE, vec); + ASSERT_EQ(retval, 0); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* negative mincore() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mincore(addr, MMAP_SIZE, vec); + ASSERT_EQ(retval, -EINVAL);
+ retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0);
+ /* positive mlock() range test */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mlock(addr, MMAP_SIZE); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + retval = munlock(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* negative mlock() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = mlock(addr, MMAP_SIZE); + ASSERT_EQ(retval, -EINVAL);
+ retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0);
+ /* negative munlock() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + EXPECT_EQ(0, mlock2(addr, MMAP_SIZE_REDUCED, MLOCK_ONFAULT)); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));
+ retval = munlock(addr, MMAP_SIZE); + ASSERT_EQ(retval, -EINVAL);
+ retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0);
+ /* positive msync() range test */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = msync(addr, MMAP_SIZE, MS_SYNC); + ASSERT_EQ(retval, 0); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* negative msync() range test */ + addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + retval = msync(addr, MMAP_SIZE, MS_SYNC); + ASSERT_EQ(retval, -EINVAL);
+ retval = munmap(addr, MMAP_SIZE_REDUCED); + ASSERT_EQ(retval, 0); +}
TEST(test_syscall_mmap) { syscall_mmap(); @@ -218,11 +344,17 @@ TEST(test_purecap_param_check) purecap_param_check(); } +TEST(test_purecap_range_check) +{ + purecap_range_check(); +}
int main(void) { test_syscall_mmap(); test_syscall_mmap2(); test_purecap_map_growsdown(); test_purecap_param_check(); + test_purecap_range_check(); return 0; }
On 7/20/23 13:15, Kevin Brodsky wrote:
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
The length of the initial mapping using an mmap() call has the ability to be reduced in a subsequent mmap() call without first being unmapped. Whereas, trying to increase the length of the initial mapping would
I have trouble making sense of this: mappings can only be enlarged or shrunk using mremap(), and these tests are not about mremap(). Rather using mmap(MAP_FIXED) in the middle of an existing mapping overwrites part of it.
result in failure as the capability is not permitted to access a range that it does not own.
Address space management syscalls that manipulate a given mapping are restricted to the range owned by the capability. Any attempt to modify or access memory beyond this range will result in failure of syscall.
It would be good to clarify that the mm syscalls do not access memory, but rather manage mappings.
I'll rephrase the commit message keeping in mind both these points.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 132 +++++++++++++++++++ 1 file changed, 132 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 184dcf4ddc92..4a2a552d47dc 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -198,6 +198,132 @@ static inline void purecap_param_check(void) ASSERT_EQ(retval, 0); } +/* test to verify address space management syscall behaviour when capability
- range is modified */
+static inline void purecap_range_check(void) +{
- void *addr, *ret;
- int retval;
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- unsigned char vec[MMAP_SIZE / PAGE_SIZE];
- /* mapping a smaller range at prev mmap addr in a subsequent mmap()
* call without first unmapping */- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- ret = mmap(addr, MMAP_SIZE_REDUCED, prot, flags | MAP_FIXED, -1, 0);
- ASSERT_GT((unsigned long)ret, 0);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* mapping a larger range at prev mmap addr in a subsequent mmap()
* call without first unmapping */- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- ret = mmap(addr, 2 * MMAP_SIZE, prot, flags | MAP_FIXED, -1, 0);
- ASSERT_EQ((unsigned long)ret, (unsigned long)-EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* positive madvise() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = madvise(addr, MMAP_SIZE, MADV_WILLNEED);
- ASSERT_NE(retval, -EINVAL);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative madvise() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = madvise(addr, MMAP_SIZE, MADV_NORMAL);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* positive mincore() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mincore(addr, MMAP_SIZE, vec);
- ASSERT_EQ(retval, 0);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative mincore() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mincore(addr, MMAP_SIZE, vec);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* positive mlock() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mlock(addr, MMAP_SIZE);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munlock(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative mlock() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mlock(addr, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* negative munlock() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- EXPECT_EQ(0, mlock2(addr, MMAP_SIZE_REDUCED, MLOCK_ONFAULT));
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE_REDUCED,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munlock(addr, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE_REDUCED);
- ASSERT_EQ(retval, 0);
- /* positive msync() range test */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = msync(addr, MMAP_SIZE, MS_SYNC);
- ASSERT_EQ(retval, 0);
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* negative msync() range test */
- addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = msync(addr, MMAP_SIZE, MS_SYNC);
- ASSERT_EQ(retval, -EINVAL);
These madvise/mincore/mlock/munlock/msync testcases will indeed fail in PCuABI (and only in PCuABI), but that is purely because the length they are passed is greater than the length of addr, not because the mapping is smaller. It would be clearer to reduce the bounds of addr instead of passing a length that is larger than the mapping. Also, given that the tests are the same regardless of the particular syscall, it would be good to refactor them to reduce duplication.
I'll change the tests accordingly. As of now I'm having one positive and one negative case for each of the above mentioned syscalls, are you referring to duplication in terms of the method of testing?
I'm only testing for this condition: CapabilityOwnsRange(addr, addr.address, len) does not hold, then the call fails with -EINVAL
As I can't increase the bounds of the capability, I would only be able to test this condition by reducing the bounds, right? Wouldn't that inevitably lead to the testcases being quite similar?
Kevin
On 7/20/23 13:13, Kevin Brodsky wrote:
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
Only valid owning capabilities have the ability to modify an address space by making use of the appropriate syscalls, any deviation from this method must result in immediate failure of the syscall. When a capability is created it is assigned the maximum permissions it may ever have by passing PROT_MAX(max_prot) as one of the prot flags. Any attempt to increase the permissions beyond this would result in failure of the syscall.
A test to validate the parmeters passed to address space management syscalls has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 76 ++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 00d4c5ea9703..184dcf4ddc92 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -128,6 +128,76 @@ static void purecap_map_growsdown(void) ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); }
+/* test to validate parameters passed to address space management syscalls */ +static inline void purecap_param_check(void) +{
- void *addr, *addr_cap, *null_cap_addr;
- ptraddr_t address;
- int retval;
- int max_prot = PROT_READ | PROT_WRITE | PROT_EXEC;
- int prot = max_prot;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- /* generate null-derived capability to be used later*/
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- address = cheri_address_get(addr);
- null_cap_addr = (void *)(uintptr_t)address;
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* passing invalid capability to sycall */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- addr_cap = cheri_tag_clear(addr);
addr_cap is a strange name, invalid_cap maybe? Alternatively you don't really need a temporary at all here, you could directly pass cheri_tag_clear(addr) to munmap().
I'll make the change.
- retval = munmap(addr_cap, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* increase permission beyond the maximum prot specified for the mapping */
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(PROT_READ | PROT_WRITE) |
PROT_READ | PROT_WRITE, flags, -1, 0);Passing R|W as prot already implies PROT_MAX(R|W), so there is no need to use PROT_MAX() for that test.
Got it, I'll change it.
- ASSERT_GT((unsigned long)addr, 0);
- retval = mprotect(addr, MMAP_SIZE, PROT_EXEC);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* max_prot has fewer permissions than prot */
- max_prot = (prot & ~(PROT_READ));
Reusing the value of prot and/or prot_max set by previous cases (or at the beginning) is not very readable. I would rather have every case set prot and prot_max explicitly. Feel free to define a macro set to R|W|X to save some typing.
Sure, I'll make that change.
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0);
- ASSERT_EQ((unsigned long)addr, (unsigned long)-EINVAL);
- /* max_prot has more permissions than prot */
- max_prot = prot;
- prot = (prot & ~(PROT_WRITE));
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mprotect(addr, MMAP_SIZE, PROT_WRITE);
- ASSERT_NE(retval, -EINVAL);
Why wouldn't we check that the call actually succeeded? I think I've noticed this pattern in other patches too. In general it is not enough to check that the return value is not a particular error, we need to check that it is valid (or a particular error).
I must have changed it during testing and missed it, I'll go though it and change it accordingly.
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* repeat positive max_prot test with null-derived capability */
- addr = mmap(null_cap_addr, MMAP_SIZE, PROT_MAX(max_prot) | prot,
flags | MAP_FIXED, -1, 0);We should avoid relying on this address that has been previously obtained from mmap() being available. That's because it is unspecified when reservations are destroyed (see the "Lifetime of reservations" note in the spec), meaning that the reservation for that initial mmap() may still exist. The MAP_FIXED + address use-case is pretty limited, but I suppose it's still worth testing. What we could do is use a hardcoded address that is not going to be ever mapped. That's not so easy to come by, but I think the very beginning of the address space is quite safe (not 0x0 though, which cannot be mapped). Maybe at address == page_size?
I see what you mean, I'll change it in this as well as patch 5.
Kevin
On 7/18/23 21:14, Tudor Cretu wrote:
On 17-07-2023 06:11, Chaitanya S Prakash wrote:
Reservations are contiguous ranges of virtual addresses that exactly match the bounds of an owning capability which is provided by the kernel. When an owning capability is passed to a syscall, its bounds are first verified against the existing reservations. If the bounds of the capability are found to not match any of the existing reservations, the syscall is expected to fail with a -ERESERVATION error code. Tests to verify this behaviour has been added.
An additional check to verify whether the representable length returned by the capability is greater than or equal to the bounds set for the capability has been added. Finally, signal handler has also been registered to handle invalid memory access.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 116 +++++++++++++++++++ 1 file changed, 116 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 4a2a552d47dc..c7f1ddb395c5 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -8,8 +8,13 @@ #include <linux/errno.h> #include <linux/fs.h> #include <linux/fcntl.h> +#include <linux/signal.h> +#include <asm/sigcontext.h> +#include <asm/siginfo.h> +#include <asm/ucontext.h> #include <cheriintrin.h> #include "freestanding.h" +#include "signal_common.h" #define MMAP_SIZE ((1ULL << 16) << 1) /* 64k x 2 */ @@ -19,6 +24,7 @@ #define PROBE_MODE_TOUCH 0x01 #define PROBE_MODE_VERIFY 0x02 +static volatile unsigned int signal_status; static inline int probe_mem_range(void *addr, size_t size, int mode) { @@ -37,6 +43,37 @@ static inline int probe_mem_range(void *addr, size_t size, int mode) return 0; } +static void purecap_sigsegv_handler(int n, siginfo_t *si, void *data) +{ + struct ucontext *uc = (struct ucontext *)data;
+ ASSERT_EQ(si->si_signo, n); + TH_LOG("Signal (%d) occurred", n); + uc->uc_mcontext.pc += 4; + signal_status = 1; +}
+static void raise_sigsegv(void) +{ + void *addr; + long value; + int retval; + unsigned int *invalid_addr;
You can leave it "void *addr", so that you don't need to do a cast below.
+ int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS;
+ addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + invalid_addr = (unsigned int *)addr;
You don't need this assignment I think.
I'll remove it, it does unnecessary.
+ EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + invalid_addr = (unsigned int *)(addr + MMAP_SIZE); + asm volatile("ldr %w0, [%1]" : "=r" (value) : "C" (invalid_addr) : "memory");
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0); +}
/* Simple test to check our ability to create a new anonymous mapping * in the virtual address space of the calling process */ @@ -324,6 +361,79 @@ static inline void purecap_range_check(void) ASSERT_EQ(retval, 0); } +/* test to verify mmap() behaviour when capability bounds are modified */ +static inline void purecap_mmap_bounds_check(void) +{ + int retval; + ptraddr_t address; + size_t res_length; + void *addr, *addr_cap, *ddc_cap, *ddc_value, *new_addr, *null_cap_addr;
addr is a valid capability throughout this test, so having both addr and addr_cap is a bit confusing.
I see what you mean, I'll rename it to something more fitting.
+ int prot = PROT_READ | PROT_WRITE; + int flags = MAP_PRIVATE | MAP_ANONYMOUS;
+ struct sigaction sa;
+ /* generate a null-derived capability to be used later */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0);
+ address = cheri_address_get(addr); + null_cap_addr = (void *)(uintptr_t)address; + retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0);
+ /* bounds of capability is increased by using root capability of the DDC + * register */ + ddc_value = cheri_ddc_get();
Don't need ddc_value and ddc_cap. Just use only one variable: ddc_cap.
+ ddc_cap = cheri_address_set(ddc_value, cheri_address_get(addr)); + ddc_cap = cheri_bounds_set(ddc_value, 2 * MMAP_SIZE);
Probably you meant cheri_bounds_set(ddc_cap,...) 🙂. Same on the line below.
+ ddc_cap = cheri_perms_and(ddc_value, cheri_perms_get(addr)); + EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE, + PROBE_MODE_TOUCH | PROBE_MODE_VERIFY)); + retval = munmap(ddc_cap, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
Is there a particular reason why you munmap the same region twice?
As per Kevin's comments this test seems to be invalid, I'll be removing it.
+ /* bounds of capability returned by mmap() is reduced, followed by a + * representable length check */ + addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + addr_cap = cheri_bounds_set(addr, MMAP_SIZE_REDUCED); + res_length = cheri_representable_length((unsigned long)addr_cap);
cheri_representable_length(size_t len) returns the length that a capability would have after using cheri_bounds_set to set the length to len (assuming appropriate alignment of the base). It's not addr_cap the argument that you want to pass to it AFAICT, maybe like this:
res_length = cheri_representable_length(cheri_length_get(addr_cap));
I'll make the change.
+ EXPECT_GE(res_length, MMAP_SIZE_REDUCED);
+ retval = munmap(addr_cap, MMAP_SIZE); + ASSERT_EQ(retval, -EINVAL);
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* overlapping mappings produce a reservation error */ + addr = mmap(null_cap_addr, MMAP_SIZE, prot, flags | MAP_FIXED, -1, 0); + ASSERT_GT((unsigned long)addr, 0); + new_addr = mmap(null_cap_addr + MMAP_SIZE_REDUCED, MMAP_SIZE_REDUCED, + prot, flags | MAP_FIXED, -1, 0); + ASSERT_EQ((unsigned long)new_addr, (unsigned long)-ERESERVATION);
+ retval = munmap(addr, MMAP_SIZE); + ASSERT_EQ(retval, 0);
+ /* register a signal handler to catch invalid memory access */
I really like your comments for each mechanism that you're testing. I would suggest adding to this comment what the code below actually tests, and then why you need to register a signal handler.
Sure, I'll add that :)
+ sigemptyset(&sa.sa_mask); + sa.sa_handler = (sighandler_t)(void *)purecap_sigsegv_handler; + sa.sa_flags = SA_SIGINFO; + sigaction(SIGSEGV, &sa, NULL); + signal_status = 0; + raise_sigsegv(); + ASSERT_TRUE(signal_status); + TH_LOG("Segmenation fault caused by invalid memory access has been handled.");
Typo in Segmentation.
I'll correct it.
Tudor
+}
TEST(test_syscall_mmap) { syscall_mmap(); @@ -349,6 +459,11 @@ TEST(test_purecap_range_check) purecap_range_check(); } +TEST(test_purecap_mmap_bounds_check) +{ + purecap_mmap_bounds_check(); +}
int main(void) { test_syscall_mmap(); @@ -356,5 +471,6 @@ int main(void) test_purecap_map_growsdown(); test_purecap_param_check(); test_purecap_range_check(); + test_purecap_mmap_bounds_check(); return 0; }
On 28/07/2023 08:55, Chaitanya S Prakash wrote:
In fact, I wonder if we shouldn't simply use mmap_verified() whenever we expect mmap() to succeed, as that function already takes care of the standard checks (return value, tag, permissions). Maybe we could add another helper (in this file) for the cases where we expect mmap() to fail, adding an argument specifying the expected error. I'm suggesting this as the casts in ASSERT_EQ() in that case are rather cumbersome.
I'm assuming you meant we *should* use mmap_verified() whenever mmap is expected to succeed?
That's an option yes, I don't have a strong opinion on this though.
Kevin
On 7/28/23 13:33, Kevin Brodsky wrote:
On 28/07/2023 08:55, Chaitanya S Prakash wrote:
In fact, I wonder if we shouldn't simply use mmap_verified() whenever we expect mmap() to succeed, as that function already takes care of the standard checks (return value, tag, permissions). Maybe we could add another helper (in this file) for the cases where we expect mmap() to fail, adding an argument specifying the expected error. I'm suggesting this as the casts in ASSERT_EQ() in that case are rather cumbersome.
I'm assuming you meant we *should* use mmap_verified() whenever mmap is expected to succeed?
That's an option yes, I don't have a strong opinion on this though.
Oh my bad I read that wrong, I'll change it accordingly.
Kevin
On 28/07/2023 09:30, Chaitanya S Prakash wrote:
These madvise/mincore/mlock/munlock/msync testcases will indeed fail in PCuABI (and only in PCuABI), but that is purely because the length they are passed is greater than the length of addr, not because the mapping is smaller. It would be clearer to reduce the bounds of addr instead of passing a length that is larger than the mapping. Also, given that the tests are the same regardless of the particular syscall, it would be good to refactor them to reduce duplication.
I'll change the tests accordingly. As of now I'm having one positive and one negative case for each of the above mentioned syscalls, are you referring to duplication in terms of the method of testing?
Yes, as far as I can tell they are all the same except for the syscall they make.
I'm only testing for this condition: CapabilityOwnsRange(addr, addr.address, len) does not hold, then the call fails with -EINVAL
As I can't increase the bounds of the capability, I would only be able to test this condition by reducing the bounds, right? Wouldn't that inevitably lead to the testcases being quite similar?
I'm not sure I understand the point, as the testcases are already the same. In terms of bounds, what that condition ensures is that the requested range falls within the capability bounds, so if we want to check the failure case it makes sense to shrink the capability bounds.
Kevin
linux-morello@op-lists.linaro.org
-
Chaitanya S Prakash -
Kevin Brodsky -
Tudor Cretu