Add tests for capability enforcement via the standard copy routines (copy_{to, from}_user, {get, put}_user), futex operations and the explicit uaccess checks used in iov_iter via readv and writev.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com ---
Hi everyone,
This is v3 of the uaccess kselftest patch. The main changes from V2 to V3 are: - Corrections to function prototypes to remove void * in favour of the actual struct pointer types and other minor corrections. - Correction to the futex test, which now tests for a correctly enforced uaccess routine when used in an atomic operation. - Reintroduction of the out-of-bounds checking in tests.
V2 introduced the following changes: - More concise formatting changes - Corrections to various mistakes in tests - Addition of a strlen_user() test - Fixed futex test
The review branch can be found at: https://git.morello-project.org/arkamnite/linux/-/commits/morello%2Fuaccess_...
The related issue can also be found at: https://git.morello-project.org/morello/kernel/linux/-/issues/58
Many thanks,
Akram
--- .../testing/selftests/arm64/morello/Makefile | 2 +- .../testing/selftests/arm64/morello/uaccess.c | 271 ++++++++++++++++++ 2 files changed, 272 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/arm64/morello/uaccess.c
diff --git a/tools/testing/selftests/arm64/morello/Makefile b/tools/testing/selftests/arm64/morello/Makefile index ecfa6fc5e989..7b794d3e436c 100644 --- a/tools/testing/selftests/arm64/morello/Makefile +++ b/tools/testing/selftests/arm64/morello/Makefile @@ -17,7 +17,7 @@ CFLAGS += $(CLANG_FLAGS) $(CFLAGS_PURECAP) $(CFLAGS_COMMON) LDFLAGS += $(CLANG_LDFLAGS) $(CLANG_FLAGS) -nostdlib -static
SRCS := $(wildcard *.c) $(wildcard *.S) -PROGS := bootstrap clone exit mmap read_write sched signal +PROGS := bootstrap clone exit mmap read_write sched signal uaccess DEPS := $(wildcard *.h)
# these are the final executables diff --git a/tools/testing/selftests/arm64/morello/uaccess.c b/tools/testing/selftests/arm64/morello/uaccess.c new file mode 100644 index 000000000000..d1624f61adcd --- /dev/null +++ b/tools/testing/selftests/arm64/morello/uaccess.c @@ -0,0 +1,271 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2023 Arm Limited + +#include <stdint.h> +#include <stddef.h> +#include <stdbool.h> + +#include <asm/fcntl.h> +#include <asm-generic/errno-base.h> +#include <cheriintrin.h> +#include <linux/errno.h> +#include <linux/fs.h> +#include <linux/futex.h> +#include <linux/mman.h> +#include <linux/signal.h> +#include <linux/sysinfo.h> +#include <linux/socket.h> +#include <linux/time.h> +#include <linux/uio.h> + +#include "freestanding.h" + +#define MSG_LEN 16 +#define NR_FUTEXES 5 + +static const char file[] = "/my_file.txt"; + +static inline int fsopen(const char *string) +{ + return syscall(__NR_fsopen, string); +} + +/* + * FUTEX_WAKE_OP is an operation used to handle more than one futex at the same + * time. When using FUTEX_WAKE_OP, val3 is an encoding of both the operation as + * well as the comparison to be performed on the futex word at uaddr2. If val3 + * is set to 0, this represents an operation of FUTEX_OP_SET and a comparison + * FUTEX_OP_CMP_EQ, where both oparg and comparg are 0. + */ +static inline int futex_wake(uint32_t *uaddr, uint32_t *uaddr2, uint32_t val3, + const struct timespec *timeout) +{ + return syscall(__NR_futex, uaddr, FUTEX_WAKE_OP, NR_FUTEXES, timeout, + uaddr2, val3); +} + +typedef unsigned short sa_family_t; + +struct sockaddr { + sa_family_t sa_family; + char sa_data[14]; +}; + +static int getsockname(int socket, struct sockaddr *sockaddr, int *socklen) +{ + return syscall(__NR_getsockname, socket, sockaddr, socklen); +} + +static int open_file(void) +{ + return syscall(__NR_openat, 0, file, O_RDWR | O_CREAT, 0666); +} + +static inline int readv(int fd, struct iovec *iov, int iovcnt) +{ + return syscall(__NR_readv, fd, iov, iovcnt); +} + +static inline int writev(int fd, struct iovec *iov, int iovcnt) +{ + return syscall(__NR_writev, fd, iov, iovcnt); +} + +static inline int sigaction(int sig, struct sigaction *old, struct sigaction *new) +{ + return syscall(__NR_rt_sigaction, sig, old, new, sizeof(sigset_t)); +} + +static inline int sysinfo(struct sysinfo *ptr) +{ + return syscall(__NR_sysinfo, ptr); +} + +TEST(test_copy_to_user) +{ + struct sysinfo my_sysinfo; + + ASSERT_EQ(sysinfo(&my_sysinfo), 0); + + ASSERT_EQ(sysinfo(&my_sysinfo + sizeof(struct sysinfo) * 100), -EFAULT); + + ASSERT_EQ(sysinfo(cheri_tag_clear(&my_sysinfo)), -EFAULT); + + ASSERT_EQ(sysinfo(cheri_perms_and(&my_sysinfo, 0)), -EFAULT); +} + +/* + * sigaction() tests both copy_to_user and copy_from_user by copying + * into and from a pointer to struct sigaction. The resulting action + * in this case is a no-op, but this does not affect the validity of + * the test. sigaction attempts to copy the new action from user space + * and the old action to user space, if the arguments is valid in either + * case. + */ +TEST(test_copy_user) +{ + struct sigaction act; + + ASSERT_EQ(sigaction(SIGSEGV, NULL, &act), 0); + ASSERT_EQ(sigaction(SIGSEGV, &act, NULL), 0); + + ASSERT_EQ(sigaction(SIGSEGV, NULL, &act + sizeof(struct sigaction) * 100), -EFAULT); + ASSERT_EQ(sigaction(SIGSEGV, &act + sizeof(struct sigaction) * 100, NULL), -EFAULT); + + ASSERT_EQ(sigaction(SIGSEGV, NULL, cheri_tag_clear(&act)), -EFAULT); + ASSERT_EQ(sigaction(SIGSEGV, cheri_tag_clear(&act), NULL), -EFAULT); + + ASSERT_EQ(sigaction(SIGSEGV, NULL, cheri_perms_and(&act, 0)), -EFAULT); + ASSERT_EQ(sigaction(SIGSEGV, cheri_perms_and(&act, 0), NULL), -EFAULT); +} + +/* + * getsockname(2) uses both get_user and put_user to copy the addrlen argument + * from and to user space respectively. getsockname(2) calls move_addr_to_user + * to copy an address to user space. move_addr_to_user also uses copy_to_user; + * if copy_to_user is successful, but either of get_user or put_user fail then + * getsockname(2) will return -EFAULT appropriately. + */ +TEST(test_get_put_user) +{ + struct sockaddr sa; + int sa_len = sizeof(sa); + int my_socket; + + // socket(AF_INET, SOCK_STREAM, 0) + my_socket = syscall(__NR_socket, 2, 1, 0); + + // The socket must be successfully opened before proceeding. + ASSERT_GE(my_socket, 0); + + ASSERT_EQ(getsockname(my_socket, &sa, &sa_len), 0) + ASSERT_EQ(getsockname(my_socket, &sa, &sa_len + sizeof(struct sockaddr) * 100), -EFAULT) + ASSERT_EQ(getsockname(my_socket, &sa, cheri_tag_clear(&sa_len)), -EFAULT) + ASSERT_EQ(getsockname(my_socket, &sa, cheri_perms_and(&sa_len, 0)), -EFAULT) + close(my_socket); +} + +/* + * The FUTEX_WAKE_OP operation in the futex(2) syscall is used to handle more + * than one futex at the same time. This is an atomic operation that attempts + * to read, modify and write a value at a second uaddr (uaddr2) and also wake + * up waiters on the futex words at uaddr1 and uaddr2. uaddr1 and uaddr2 are + * provided via capabilities, and so this futex tests the case of an atomic + * uaccess operation. + */ +TEST(test_futex) +{ + static struct futex_waitv waitv[NR_FUTEXES]; + uint32_t futexes[NR_FUTEXES] = {0}; + struct timespec to = {.tv_sec = 0, .tv_nsec = 500000000}; + + for (int i = 0; i < NR_FUTEXES; i++) { + waitv[i].uaddr = (uintptr_t)&futexes[i]; + waitv[i].flags = FUTEX_32 | FUTEX_PRIVATE_FLAG; + waitv[i].val = futexes[i]; + waitv[i].__reserved = 0; + } + + ASSERT_GE(futex_wake(&futexes[0], &futexes[0], 0, &to), 0); + ASSERT_EQ(futex_wake(&futexes[0], + &futexes[0] + sizeof(uint32_t) * NR_FUTEXES, 0, + &to), + -EFAULT); + ASSERT_EQ(futex_wake(&futexes[0], cheri_tag_clear(&futexes[0]), 0, &to), + -EFAULT); + ASSERT_EQ(futex_wake(&futexes[0], cheri_perms_and(&futexes[0], 0), 0, + &to), + -EFAULT); +} + +/* + * Test explicit accesses used in iov_iter via readv and writev. Both + * syscalls use explicit checking on the iov_base field of struct iovec, + * so the metadata of the capability provided for iov_base is modified as + * per the needs of each individual test. + */ +TEST(test_explicit_iov_iter) +{ + char buf0[2]; + char buf1[4]; + char buf2[6]; + char *write_buf0 = "Hello I am the first char buffer!\n"; + char *write_buf1 = "Hello, I am the second char buffer.\n"; + char *write_buf2 = "Hello, I am the third and final char buffer.\n"; + struct iovec iov[3]; + int iovcnt; + static int fd; + + fd = open_file(); + ASSERT_NE(fd, -1); + + iov[0].iov_base = buf0; + iov[0].iov_len = sizeof(buf0); + iov[1].iov_base = buf1; + iov[1].iov_len = sizeof(buf1); + iov[2].iov_base = buf2; + iov[2].iov_len = sizeof(buf2); + + iovcnt = sizeof(iov) / sizeof(struct iovec); + + ASSERT_GE(readv(fd, iov, iovcnt), 0); + + iov[0].iov_base = buf0 + sizeof(iov) * 100; + ASSERT_EQ(readv(fd, iov, iovcnt), -EFAULT); + + iov[0].iov_base = cheri_tag_clear(buf0); + ASSERT_EQ(readv(fd, iov, iovcnt), -EFAULT); + + iov[0].iov_base = cheri_perms_and(buf0, 0); + ASSERT_EQ(readv(fd, iov, iovcnt), -EFAULT); + + iov[0].iov_base = write_buf0; + iov[0].iov_len = strlen(write_buf0); + iov[1].iov_base = write_buf1; + iov[1].iov_len = strlen(write_buf1); + iov[2].iov_base = write_buf2; + iov[2].iov_len = strlen(write_buf2); + + ASSERT_GE(writev(fd, iov, iovcnt), 0); + + iov[0].iov_base = write_buf0 + sizeof(iov) * 100; + ASSERT_EQ(writev(fd, iov, iovcnt), -EFAULT); + + iov[0].iov_base = cheri_tag_clear(write_buf0); + ASSERT_EQ(writev(fd, iov, iovcnt), -EFAULT); + + iov[0].iov_base = cheri_perms_and(write_buf0, 0); + ASSERT_EQ(writev(fd, iov, iovcnt), -EFAULT); +} + +/* + * strlen_user() explicitly inspects an input capability, the behaviour + * of which must also be verified within these tests. The fsopen() syscall + * makes use of strlen_user() by duplicating a string (representing the name + * of a filesystem) with the strndup_user() function. strlen_user() can + * therefore be tested with a call to fsopen(). + */ +TEST(test_strlen_user) +{ + const char *fsname = "my_nonexistent_filesystem"; + + // strndup_user() will still be called, so fsopen() fails after this. + ASSERT_EQ(fsopen(fsname), -ENODEV); + + ASSERT_EQ(fsopen(fsname + strlen(fsname) * 10), -EFAULT); + + ASSERT_EQ(fsopen(cheri_tag_clear(fsname)), -EFAULT); + + ASSERT_EQ(fsopen(cheri_perms_and(fsname, 0)), -EFAULT); +} + +int main(void) +{ + test_copy_to_user(); + test_copy_user(); + test_get_put_user(); + test_futex(); + test_explicit_iov_iter(); + test_strlen_user(); + return 0; +}
On 15/01/2024 17:53, Akram Ahmad wrote:
[...]
+/*
- FUTEX_WAKE_OP is an operation used to handle more than one futex at the same
- time. When using FUTEX_WAKE_OP, val3 is an encoding of both the operation as
- well as the comparison to be performed on the futex word at uaddr2. If val3
- is set to 0, this represents an operation of FUTEX_OP_SET and a comparison
- FUTEX_OP_CMP_EQ, where both oparg and comparg are 0.
I think it would be more useful to explain this where the function is called, as this is where the mysterious literal 0 appears. The wrapper itself is pretty self-explanatory, the reader can easily find out about FUTEX_WAKE_OP themselves.
- */
+static inline int futex_wake(uint32_t *uaddr, uint32_t *uaddr2, uint32_t val3,
Let's call it futex_wake_op(), because otherwise it suggests we're using FUTEX_WAKE.
const struct timespec *timeout)+{
- return syscall(__NR_futex, uaddr, FUTEX_WAKE_OP, NR_FUTEXES, timeout,
val is the number of waiters to wake, not a number of futexes like in futex_waitv. We're not actually going to wake anything so it doesn't matter, we could just pass 1. In fact the same applies to val2, it is the number of waiters to wake for the second futex, not a timeout.
On that note we don't need an array of futexes any more, just one is enough.
uaddr2, val3);+}
+typedef unsigned short sa_family_t;
+struct sockaddr {
- sa_family_t sa_family;
- char sa_data[14];
+};
+static int getsockname(int socket, struct sockaddr *sockaddr, int *socklen) +{
- return syscall(__NR_getsockname, socket, sockaddr, socklen);
+}
+static int open_file(void) +{
- return syscall(__NR_openat, 0, file, O_RDWR | O_CREAT, 0666);
+}
+static inline int readv(int fd, struct iovec *iov, int iovcnt) +{
- return syscall(__NR_readv, fd, iov, iovcnt);
+}
+static inline int writev(int fd, struct iovec *iov, int iovcnt) +{
- return syscall(__NR_writev, fd, iov, iovcnt);
+}
+static inline int sigaction(int sig, struct sigaction *old, struct sigaction *new) +{
- return syscall(__NR_rt_sigaction, sig, old, new, sizeof(sigset_t));
+}
+static inline int sysinfo(struct sysinfo *ptr) +{
- return syscall(__NR_sysinfo, ptr);
+}
+TEST(test_copy_to_user) +{
- struct sysinfo my_sysinfo;
- ASSERT_EQ(sysinfo(&my_sysinfo), 0);
- ASSERT_EQ(sysinfo(&my_sysinfo + sizeof(struct sysinfo) * 100), -EFAULT);
This actually results in a pointer at an offset (in bytes) of sizeof(struct sysinfo) * sizeof(struct sysinfo) * 100, considering the type of my_sysinfo.
Generally speaking all these structs are small enough that their bounds are exactly representable, so we don't need a big offset, incrementing the pointer by 1 (i.e. the size of the struct) is enough. This way we know for sure that the bounds remain representable (the tag won't be cleared), and we also check that the kernel's bounds check is precise.
- ASSERT_EQ(sysinfo(cheri_tag_clear(&my_sysinfo)), -EFAULT);
- ASSERT_EQ(sysinfo(cheri_perms_and(&my_sysinfo, 0)), -EFAULT);
+}
[...]
+TEST(test_explicit_iov_iter) +{
- char buf0[2];
- char buf1[4];
- char buf2[6];
- char *write_buf0 = "Hello I am the first char buffer!\n";
- char *write_buf1 = "Hello, I am the second char buffer.\n";
- char *write_buf2 = "Hello, I am the third and final char buffer.\n";
Should be const.
- struct iovec iov[3];
- int iovcnt;
- static int fd;
fd shouldn't need to be static, also we should close it at the end.
Kevin
[...]
linux-morello@op-lists.linaro.org
-
Akram Ahmad -
Kevin Brodsky