[linux-morello] Re: [PATCH] uaccess_testing: Testing of capability enforcement in uaccess

On 18/12/2023 16:11, 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 patch extends kselftests with tests for various uaccess routines. Broadly speaking, this tests the standard uaccess routines, futex operations, and iov_iter (the latter is implemented by testing readv and writev syscalls).

The tests currently examine the effects of passing a capability with a cleared tag, invalid permissions, and an invalid base address offset. These are tested separately from one another, and I would especially appreciate any comments on how this could be improved if needed (particularly on the futex test).

Thank you and happy holidays!

Akram

[1] The branch for this patch: https://git.morello-project.org/arkamnite/linux/-/commits/morello%2Fuaccess

[2] The issue ticket for this patch: https://git.morello-project.org/morello/kernel/linux/-/issues/58

tools/testing/selftests/Makefile | 1 + .../testing/selftests/arm64/morello/Makefile | 2 +- .../testing/selftests/arm64/morello/uaccess.c | 491 ++++++++++++++++++ 3 files changed, 493 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/arm64/morello/uaccess.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 90a62cf75008..82abe4ddbb29 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -86,6 +86,7 @@ TARGETS += timers endif TARGETS += tmpfs TARGETS += tpm2 +TARGETS += uaccess

That looks like a leftover from a previous approach.

TARGETS += user TARGETS += vDSO TARGETS += mm 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..66d93137b819 --- /dev/null +++ b/tools/testing/selftests/arm64/morello/uaccess.c @@ -0,0 +1,491 @@ +// 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/fs.h> +#include <linux/futex.h> +#include <linux/mman.h> +#include <linux/sysinfo.h> +#include <linux/socket.h> +#include <linux/time.h> +#include <linux/uio.h>

+#include "freestanding.h"

+#define FILE_PERM 0666

Not used.

+#define MSG_LEN 16 +#define NR_FUTEXES 2

+static struct futex_waitv waitv[NR_FUTEXES]; +uint32_t futexes[NR_FUTEXES] = {1};

+static const char file[] = "/my_file.txt";

It would be nice not to create files at the root unconditionally, but I know this is what we do already. I'll think about how to improve this.

The remaining (non-const) global variables don't seem to be justified: AFAICT there is no need for their lifetime to be longer than any individual test function, so they could be local to each function instead.

+static int fd; +static int my_socket;

+static int open_file(void) +{

• return syscall(__NR_openat, 0, file, O_RDWR | O_CREAT, 0666);

+}

+static int close_file(int __fd) +{

• return syscall(__NR_close, __fd);

+}

We already have close() in freestanding.h.

+static __always_inline +void copy_to_user1(void)

I'm not sure it helps much to split each testcase in multiple functions. In tests like get_user, it means duplicating the preparation stage (creating a socket) in each of them. Having the tests directly in the TEST(...) function is perfectly fine, that's the direction we're taking with the memory management syscalls (see Chaitanya's series [1]). Adding some comments is usually enough to separate the steps.

[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...

+{

• int result;
• struct sysinfo my_sysinfo;

I think just this struct is enough, you can reuse it. Having temporary variables to hold the pointers to the struct is not particularly useful either (that applies to most functions).

• struct sysinfo *p_si = &my_sysinfo;
• struct sysinfo my_sysinfo2;
• struct sysinfo *p_si2 = &my_sysinfo2 + 10 * sizeof(struct sysinfo);
• result = syscall(__NR_sysinfo, p_si);
• TH_LOG("%s: sysinfo with valid capability", __func__);
• ASSERT_EQ(result, 0);
• TH_LOG("%s: sysinfo with invalid capability offset", __func__);
• result = syscall(__NR_sysinfo, p_si2);
• ASSERT_EQ(result, -EFAULT);

+}

+static __always_inline +void copy_to_user2(void) +{

• int result;
• struct sysinfo my_sysinfo;
• struct sysinfo *cap = &my_sysinfo;
• TH_LOG("%s: sysinfo with cleared capability tag", __func__);
• cap = (struct sysinfo *)cheri_tag_clear(cap);

The cast shouldn't be necessary (if you keep the variable at all), as void * is implicitly convertible to and from any pointer type in C (unlike C++).

• result = syscall(__NR_sysinfo, cap);
• ASSERT_EQ(result, -EFAULT);

+}

+static __always_inline +void copy_to_user3(void) +{

• int result;
• struct sysinfo my_sysinfo;
• struct sysinfo *cap = &my_sysinfo;
• TH_LOG("%s: sysinfo with no capability permissions", __func__);
• cap = (struct sysinfo *)cheri_perms_and(cap, 0);
• result = syscall(__NR_sysinfo, cap);
• ASSERT_EQ(result, -EFAULT);

+}

+TEST(test_copy_to_user) +{

• copy_to_user1();
• copy_to_user2();
• copy_to_user3();

+}

+static __always_inline +void copy_from_user1(void) +{

• int result;
• size_t my_offset = sizeof(int);
• size_t *good_p = &my_offset;
• size_t *bad_p = &my_offset + sizeof(int);
• /**

• * Setup the parameters for our syscall:
  

• * fd_in, fd_out: these can be the same fd.
  

• * off_in: the capability pointing to a buffer for the offset
  

• * off_out: NULL, as we do not need this.
  

• * len: MSG_LEN
  

• */
  

• fd = open_file();
• result = syscall(__NR_copy_file_range, fd, good_p, fd, NULL, MSG_LEN, 0);

This is quite a complicated syscall to test copy_from_user(), especially since it's only used to copy in an int (that could easily be replaced with get_user() in the future). For this specific case, the man page says:

fd_in and fd_out can refer to the same file.  If they refer to the

same file, then the source and target ranges are not allowed to overlap.

So I don't think this is a valid call in the first place, even though it might not error out. I've considered various sycalls as alternative, in the end I think that sigaction() or sigaltstack() are the most straightforward: first obtain the old action with something like sigaction(SIGTERM, NULL, &act) and then install it with sigaction(SIGTERM, &act, NULL). The overall operation is a no-op, but the first part exercises copy_to_user() and the latter copy_from_user().

• TH_LOG("%s: copy_file_range with valid capability", __func__);
• ASSERT_GE(0, result);
• result = syscall(__NR_copy_file_range, fd, bad_p, fd, NULL, MSG_LEN, 0);
• TH_LOG("%s: copy_file_range with invalid capability offset", __func__);
• ASSERT_EQ(result, -EFAULT);
• close_file(fd);

+}

+static __always_inline +void copy_from_user2(void) +{

• int result;
• size_t my_offset = sizeof(int);
• size_t *good_p = &my_offset;
• size_t *bad_p = (size_t *)cheri_tag_clear(good_p);
• fd = open_file();
• result = syscall(__NR_copy_file_range, fd, bad_p, fd, NULL, MSG_LEN, 0);
• TH_LOG("%s: copy_file_range with cleared capability tag", __func__);
• ASSERT_EQ(result, -EFAULT);
• close_file(fd);

+}

+static __always_inline +void copy_from_user3(void) +{

• int result;
• size_t my_offset = sizeof(int);
• size_t *bad_p = (size_t *)cheri_perms_and(&my_offset, 0);
• fd = open_file();
• result = syscall(__NR_copy_file_range, fd, bad_p, fd, NULL, MSG_LEN, 0);
• TH_LOG("%s: copy_file_range with capability with no permissions", __func__);
• ASSERT_EQ(result, -EFAULT);
• close_file(fd);

+}

+TEST(test_copy_from_user) +{

• copy_from_user1();
• copy_from_user2();
• copy_from_user3();

+}

+typedef unsigned short sa_family_t;

+struct sockaddr {

• sa_family_t sa_family;
• char sa_data[14];

+};

+static __always_inline +void get_user1(void) +{

• int result;
• struct sockaddr sa;
• struct sockaddr sa2;
• struct sockaddr *my_sockaddr_ptr = &sa2 + sizeof(int) * 1000;
• // socket(AF_INET, SOCK_STREAM, 0)
• my_socket = syscall(__NR_socket, 2, 1, 0);
• TH_LOG("%s: socket syscall complete", __func__);

Nit: I think we can do without such logs, one per tested syscall is already quite a bit.

• ASSERT_GE(my_socket, 0);
• int sa_len = sizeof(sa);
• int *my_len_pointer = &sa_len;
• TH_LOG("%s: getsockname with valid capability", __func__);
• result = syscall(__NR_getsockname, my_socket, &sa, &sa_len);

Interesting syscall to test uaccess, since it uses all of get_user(), put_user() and copy_to_user(), nice find! Just one thing to keep in mind: get_user / put_user are used for the length pointer, not the sockaddr pointer, but the tests below manipulate the sockaddr pointer.

• ASSERT_EQ(result, 0);
• sa_len = sizeof(sa2);
• TH_LOG("%s: getsockname with invalid capability offset", __func__);
• result = syscall(__NR_getsockname, my_socket, my_sockaddr_ptr, my_len_pointer);
• ASSERT_EQ(result, -EFAULT);

The socket should be closed here.

+}

+static __always_inline +void get_user2(void) +{

• int result;
• struct sockaddr sa;
• int sa_len = sizeof(sa);
• struct sockaddr *p_sockaddr = cheri_tag_clear(&sa);
• // socket(AF_INET, SOCK_STREAM, 0)
• my_socket = syscall(__NR_socket, 2, 1, 0);
• ASSERT_GE(my_socket, 0);
• result = syscall(__NR_getsockname, my_socket, p_sockaddr, &sa_len);
• TH_LOG("%s: getsockname with cleared capability tag", __func__);
• ASSERT_EQ(result, -EFAULT);

+}

+static __always_inline +void get_user3(void) +{

• int result;
• struct sockaddr sa;
• int sa_len = sizeof(sa);
• struct sockaddr *p_sockaddr = cheri_perms_and(&sa, 0);
• // socket(AF_INET, SOCK_STREAM, 0)
• my_socket = syscall(__NR_socket, 2, 1, 0);
• ASSERT_GE(my_socket, 0);
• result = syscall(__NR_getsockname, my_socket, p_sockaddr, &sa_len);
• TH_LOG("%s: getsockname with no capability permissions", __func__);
• ASSERT_EQ(result, -EFAULT);

+}

+TEST(test_get_user) +{

• get_user1();
• get_user2();
• get_user3();

+}

+static __always_inline +void put_user1(void) +{

• int result;
• struct timeval tv;
• struct timeval *p_tv = &tv;
• result = syscall(__NR_gettimeofday, p_tv, NULL);
• TH_LOG("%s: gettimeofday with valid capability", __func__);
• ASSERT_EQ(result, 0);
• p_tv = &tv + sizeof(struct timeval) * 1000;
• result = syscall(__NR_gettimeofday, p_tv, NULL);
• TH_LOG("%s: gettimeofday with invalid capability offset", __func__);
• ASSERT_EQ(result, -EFAULT);

+}

+static __always_inline +void put_user2(void) +{

• int result;
• struct timeval tv;
• struct timeval *p_tv = &tv;
• p_tv = cheri_tag_clear(p_tv);
• result = syscall(__NR_gettimeofday, p_tv, NULL);
• TH_LOG("%s: gettimeofday with cleared capability tag", __func__);
• ASSERT_EQ(result, -EFAULT);

+}

+static __always_inline +void put_user3(void) +{

• int result;
• struct timeval tv;
• struct timeval *p_tv = &tv;
• p_tv = cheri_perms_and(p_tv, 0);
• result = syscall(__NR_gettimeofday, p_tv, NULL);
• TH_LOG("%s: gettimeofday with capability with no permissions", __func__);
• ASSERT_EQ(result, -EFAULT);

+}

+TEST(test_put_user) +{

• put_user1();
• put_user2();
• put_user3();

+}

+static inline int futex_waitv(volatile 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 __always_inline +void futex1(void) +{

• struct timespec to;
• for (int i = 0; i < NR_FUTEXES; i++) {

• waitv[i].uaddr = (uintptr_t)cheri_tag_clear(&futexes[i]);
  

• waitv[i].flags = FUTEX_32 | FUTEX_PRIVATE_FLAG;
  

• waitv[i].val = i;
  

• waitv[i].__reserved = 0;
  

• // This is necessary otherwise the loop is optimised away
  

That is definitely not expected and neither is the need for volatile in the declaration of futex_waitv(). This should be investigated further.

• TH_LOG("%s: waitv[%d] initialised", __func__, i);
  

• }
• int res = futex_waitv(waitv, NR_FUTEXES, 0, &to, CLOCK_MONOTONIC);
• ASSERT_EQ(res, -EFAULT);

+}

+TEST(test_futex) +{

• futex1();

+}

+static __always_inline +void explicit_readv1(void) +{

• ssize_t bytes_read;
• char buf0[2];
• char buf1[4];
• char buf2[6];
• struct iovec iov[3];
• int iovcnt;
• 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);
• fd = open_file();
• TH_LOG("%s: readv with valid capability", __func__);
• bytes_read = syscall(__NR_readv, fd, iov, iovcnt);
• close_file(fd);
• ASSERT_GE(bytes_read, 0);

+}

+static __always_inline +void explicit_readv2(void) +{

• ssize_t bytes_read;
• char buf0[2];
• char buf1[4];
• char buf2[6];
• struct iovec iov[3];
• int iovcnt;
• 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);
• fd = open_file();
• TH_LOG("%s: readv with invalid capability tag", __func__);
• bytes_read = syscall(__NR_readv, fd, cheri_tag_clear(iov), iovcnt);

We want to test explicit checking here, not standard uaccess. The struct iovec themselves are read using get_user(), so invalidating this capability causes get_user() to fail. We rather want to invalidate iov_base, as it is explicitly checked.

• close_file(fd);
• ASSERT_EQ(bytes_read, -EFAULT);

+}

+static __always_inline +void explicit_readv3(void) +{

• ssize_t bytes_read;
• char buf0[2];
• char buf1[4];
• char buf2[6];
• struct iovec iov[3];
• int iovcnt;
• iov[0].iov_base = cheri_perms_and(buf0, 0);
• 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);
• fd = open_file();
• TH_LOG("%s: readv with invalid capability permissions", __func__);
• bytes_read = syscall(__NR_readv, fd, cheri_perms_and(iov, 0), iovcnt);
• close_file(fd);
• ASSERT_EQ(bytes_read, -EFAULT);

+}

+static __always_inline +void explicit_writev1(void) +{

• ssize_t bytes_written;
• char *buf0 = "Hello I am the first char buffer!\n";
• char *buf1 = "Hello, I am the second char buffer.\n";
• char *buf2 = "Hello, I am the third and final char buffer.\n";
• struct iovec iov[3];
• int iovcnt;
• 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);
• fd = open_file();
• TH_LOG("%s: writev with valid capability", __func__);
• bytes_written = syscall(__NR_writev, fd, iov, iovcnt);
• close_file(fd);
• ASSERT_GE(bytes_written, 0);

+}

+static __always_inline +void explicit_writev2(void) +{

• ssize_t bytes_written;
• char *buf0 = "Hello I am the first char buffer!\n";
• char *buf1 = "Hello, I am the second char buffer.\n";
• char *buf2 = "Hello, I am the third and final char buffer.\n";
• struct iovec iov[3];
• int iovcnt;
• 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);
• fd = open_file();
• TH_LOG("%s: writev with invalid capability tag", __func__);
• bytes_written = syscall(__NR_writev, fd, cheri_tag_clear(iov), iovcnt);
• close_file(fd);
• ASSERT_EQ(bytes_written, -EFAULT);

+}

+static __always_inline +void explicit_writev3(void) +{

• ssize_t bytes_written;
• char *buf0 = "Hello I am the first char buffer!\n";
• char *buf1 = "Hello, I am the second char buffer.\n";
• char *buf2 = "Hello, I am the third and final char buffer.\n";
• struct iovec iov[3];
• int iovcnt;
• iov[0].iov_base = cheri_perms_and(buf0, 0);
• 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);
• fd = open_file();
• TH_LOG("%s: writev with invalid capability permissions", __func__);
• bytes_written = syscall(__NR_writev, fd, cheri_perms_and(iov, 0), iovcnt);
• close_file(fd);
• ASSERT_EQ(bytes_written, -EFAULT);

+}

+/*

• • Test readv and writev where the array of iovecs is provided
• • by a capability.
• */

+TEST(test_explicit_iov_iter) +{

• explicit_readv1();
• explicit_readv2();
• explicit_readv3();
• explicit_writev1();
• explicit_writev2();
• explicit_writev3();

+}

+int main(void) +{

• test_copy_to_user();
• test_copy_from_user();
• test_get_user();
• test_put_user();
• test_futex();
• test_explicit_iov_iter();
• return 0;

+}

I think this covers the standard uaccess routines and check_user_ptr_*(), well done! One more thing that would be nice to test is strnlen_user(), because it will inspect the input capability explicitly as a result of [2]. It looks like the easiest way to test it would be by calling the relatively new syscall fsopen(), as it starts by duplicating the input string (which uses strnlen_user()).

Kevin

[2] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...