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

On 06/02/2024 10:42, Akram Ahmad wrote:

[...]

+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)), -EFAULT);

This should also be + 1, same in test_get_put_user and test_futex.

• ASSERT_EQ(sigaction(SIGSEGV, &act + sizeof(struct sigaction), 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)), -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.
• • 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.
• */

+TEST(test_futex) +{

• uint32_t futex = 0;
• ASSERT_GE(futex_wake_op(&futex, &futex, 0), 0);
• ASSERT_EQ(futex_wake_op(&futex, &futex + sizeof(uint32_t), 0), -EFAULT);
• ASSERT_EQ(futex_wake_op(&futex, cheri_tag_clear(&futex), 0), -EFAULT);
• ASSERT_EQ(futex_wake_op(&futex, cheri_perms_and(&futex, 0), 0),

•   -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];
• const char *write_buf0 = "Hello I am the first char buffer!\n";
• const char *write_buf1 = "Hello, I am the second char buffer.\n";
• const char *write_buf2 = "Hello, I am the third and final char buffer.\n";
• struct iovec iov[3];
• int iovcnt;
• 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 + 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);

While testing locally I found that these calls don't fail if the file doesn't exist to start with. That's because in this case there is nothing to read, and as a result the pointer is never checked.

An easy fix would be to write first. Since we're writing more data than we're reading in the first readv() call, the subsequent ones will still have something to read and the pointer will be checked.

Kevin

• iov[0].iov_base = (void *)write_buf0;
• iov[0].iov_len = strlen(write_buf0);
• iov[1].iov_base = (void *)write_buf1;
• iov[1].iov_len = strlen(write_buf1);
• iov[2].iov_base = (void *)write_buf2;
• iov[2].iov_len = strlen(write_buf2);
• ASSERT_GE(writev(fd, iov, iovcnt), 0);
• iov[0].iov_base = (void *)write_buf0 + 100;
• ASSERT_EQ(writev(fd, iov, iovcnt), -EFAULT);
• iov[0].iov_base = (void *)cheri_tag_clear(write_buf0);
• ASSERT_EQ(writev(fd, iov, iovcnt), -EFAULT);
• iov[0].iov_base = (void *)cheri_perms_and(write_buf0, 0);
• ASSERT_EQ(writev(fd, iov, iovcnt), -EFAULT);
• close(fd);

+}

[...]