On 10/01/2024 18:04, Akram Ahmad wrote:
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 V2 of the uaccess kselftest patch. The main changes are:
- More concise formatting changes
- Corrections to various mistakes in tests
- Addition of a strlen_user() test case
One of the most important fixes is the fixed futex test. It turns out that whilst optimisation was causing some varying results, the overall problem was to do with a mistake in the use of the syscall. I've decided to allow the syscall to timeout in the case of a valid input capability, as I feel this tests the explicit uaccess check sufficiently, but I am not sure if this is the ideal method.
Many thanks, Akram
.../testing/selftests/arm64/morello/Makefile | 2 +- .../testing/selftests/arm64/morello/uaccess.c | 294 ++++++++++++++++++ 2 files changed, 295 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..e3c935df948a --- /dev/null +++ b/tools/testing/selftests/arm64/morello/uaccess.c @@ -0,0 +1,294 @@ +// 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 1
+static struct futex_waitv waitv[NR_FUTEXES]; +uint32_t futexes[NR_FUTEXES] = {1};
+static const char file[] = "/my_file.txt"; +static int fd;
Same comment as in v1: none of the globals is justified, besides file (which is const so that's less of a concern anyway).
+static inline int fsopen(void *string) +{
- return syscall(__NR_fsopen, string);
+}
+static inline int futex_waitv(struct futex_waitv *waiters, unsigned long nr_waiters,
unsigned long flags, struct timespec *timo, clockid_t clockid)+{
- return syscall(__NR_futex_waitv, waiters, nr_waiters, flags, timo, clockid);
+}
+static int getsockname(int socket, void *sockaddr, int *socklen)
Might as well use the actual pointer type for sockaddr. Same comment for fsopen(), sigaction() and sysinfo() (that means moving the struct definitions above those prototypes, which is sensible).
+{
- 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, void *old, void *new) +{
- return syscall(__NR_rt_sigaction, sig, old, new, sizeof(sigset_t));
+}
+static inline int sysinfo(void *ptr) +{
- return syscall(__NR_sysinfo, ptr);
+}
+TEST(test_copy_to_user) +{
- struct sysinfo my_sysinfo;
- // Ensure that valid capabilities are successfully copied.
- ASSERT_EQ(sysinfo(&my_sysinfo), 0);
- // Test copying a capability which has an invalid tag.
Generally speaking, there's no need to add a comment that paraphrases the code. That's the upside of self-explanatory code! I think quite a few comments in this file could be skipped.
- ASSERT_EQ(sysinfo(cheri_tag_clear(&my_sysinfo)), -EFAULT);
- // Test copying a capability which has invalid permissions.
- ASSERT_EQ(sysinfo(cheri_perms_and(&my_sysinfo, 0)), -EFAULT);
This is hugely nicer to read compared to v1, well done! It seems that we've lost the out-of-bounds checks along the way though, I think that was useful, could we put them back?
+}
+/*
- 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;
- // Check valid capabilities are successfully copied.
- ASSERT_EQ(sigaction(SIGSEGV, NULL, &act), 0);
- ASSERT_EQ(sigaction(SIGSEGV, &act, NULL), 0);
- /*
* Capabilities with a cleared tag must not be successfully copied* from nor into user space.*/- ASSERT_EQ(sigaction(SIGSEGV, NULL, cheri_tag_clear(&act)), -EFAULT);
- ASSERT_EQ(sigaction(SIGSEGV, cheri_tag_clear(&act), NULL), -EFAULT);
- /*
* Capabilities with invalid permissions (simulated here with 0) must* not be successfully copied to or from user space.*/- ASSERT_EQ(sigaction(SIGSEGV, NULL, cheri_perms_and(&act, 0)), -EFAULT);
- ASSERT_EQ(sigaction(SIGSEGV, cheri_perms_and(&act, 0), NULL), -EFAULT);
+}
+typedef unsigned short sa_family_t;
+struct sockaddr {
- sa_family_t sa_family;
- char sa_data[14];
+};
+/*
- getsockname(2) uses both get_user and put_user to copy the addrlen argument
- from and to user space respectively. If copy_to_user works successfully,
It's not very clear which call to copy_to_user() you're referring to here (unless the reader is already pretty familiar with how getsockname() is implemented).
- getsockname(2) will return -EFAULT if either get_user or put_user fail.
- */
+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)
goto cleanup;
A failed ASSERT will cause the process to exit, so we don't really need to worry about closing file descriptors, that's done during exit() anyway.
- // Test with cleared capability tag.
- ASSERT_EQ(getsockname(my_socket, &sa, cheri_tag_clear(&sa_len)), -EFAULT)
goto cleanup;- // Test with invalid capability permissions.
- ASSERT_EQ(getsockname(my_socket, &sa, cheri_perms_and(&sa_len, 0)), -EFAULT)
goto cleanup;- /*
* If any of the getsockname syscalls failed, then the socket is* still open and must be closed before the test exits.*/+cleanup:
- ASSERT_EQ(close(my_socket), 0);
+}
+/*
- The futex_waitv syscall explicitly checks the uaddr field of the individual
- futex waiters which is provided by a capability. This capability enforcement
- is therefore tested here; in the case where the capability is valid, the
- syscall should return -ETIMEDOUT as the futex waiter requires another thread
- to signal that it should wake up using futex_wake(). futex_wait() is not
- called in this thread, thus causing a timeout to occur.
- */
+TEST(test_futex) +{
- struct timespec to = {.tv_sec = 0, .tv_nsec = 500000000};
- int res;
- // Test with valid capabilities.
- 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;- }
- res = futex_waitv(waitv, NR_FUTEXES, 0, &to, CLOCK_MONOTONIC);
- ASSERT_EQ(res, -ETIMEDOUT);
- for (int i = 0; i < NR_FUTEXES; i++)
waitv[i].uaddr = (uintptr_t)cheri_tag_clear(&futexes[i]);- res = futex_waitv(waitv, NR_FUTEXES, 0, &to, CLOCK_MONOTONIC);
- ASSERT_EQ(res, -EFAULT);
- for (int i = 0; i < NR_FUTEXES; i++)
waitv[i].uaddr = (uintptr_t)cheri_perms_and(&futexes[i], 0);- res = futex_waitv(waitv, NR_FUTEXES, 0, &to, CLOCK_MONOTONIC);
- ASSERT_EQ(res, -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";
Nit: const char * (string literals are always read-only, but C is permissive there).
- 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;
- fd = open_file();
- ASSERT_NE(fd, -1);
- // Set up buffers for reading.
- 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);
- // Make sure readv works as expected with intact buf0.
- ASSERT_GE(readv(fd, iov, iovcnt), 0) goto cleanup;
- // Clear the tag for the pointer to buf0.
- iov[0].iov_base = cheri_tag_clear(buf0);
- ASSERT_EQ(readv(fd, iov, iovcnt), -EFAULT) goto cleanup;
- // Clear permissions, but restore the tag for buf0.
- iov[0].iov_base = buf0;
That line can be removed.
- iov[0].iov_base = cheri_perms_and(buf0, 0);
- ASSERT_EQ(readv(fd, iov, iovcnt), -EFAULT) goto cleanup;
- // Set up buffers for writing.
- iov[0].iov_base = write_buf0;
- iov[0].iov_len = sizeof(write_buf0);
sizeof(ptr) gives you the size of the pointer (16 in purecap). If you want the string length, either use strlen(), or create arrays on the stack (char write_buf0[] = "literal"). The former is probably preferable as it avoids unnecessary copying to the stack.
- iov[1].iov_base = write_buf1;
- iov[1].iov_len = sizeof(write_buf1);
- iov[2].iov_base = write_buf2;
- iov[2].iov_len = sizeof(write_buf2);
- // Make sure writev works as expected for buf0.
- ASSERT_GE(writev(fd, iov, iovcnt), 0) goto cleanup;
- // Clear the tag for the pointer to buf0.
- iov[0].iov_base = cheri_tag_clear(write_buf0);
- ASSERT_EQ(writev(fd, iov, iovcnt), -EFAULT) goto cleanup;
- // Clear permissions, but restore the tag for buf0.
- iov[0].iov_base = write_buf0;
- iov[0].iov_base = cheri_perms_and(write_buf0, 0);
- ASSERT_EQ(writev(fd, iov, iovcnt), -EFAULT) goto cleanup;
+cleanup:
- close(fd);
+}
+/*
- 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) +{
- char *fsname = "my_nonexistent_filesystem";
- // strndup_user() will still be called, so fsopen() fails after this.
- ASSERT_EQ(fsopen(&fsname), -ENODEV);
&fsname is a pointer to char *, i.e. char **. I suppose it still works as expected because there'll eventually be a '\0' to terminate the string made up of arbitrary characters. But clearly not what you meant! Which is why void * in prototypes is to be avoided if possible :)
Kevin
- // Clear the tag for the input capability to strndup_user().
- ASSERT_EQ(fsopen(cheri_tag_clear(&fsname)), -EFAULT);
- // Clear the permissions for the input capability to strndup_user().
- 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;
+}