Hi,
This series fixes a number of issues that the switch to checked (capability-based) uaccess [1] has revealed. These patches complete those that I have posted upstream [2] (and are already merged); they address issues that are likely to be too purecap-specific for upstream.
Review branch:
https://git.morello-project.org/kbrodsky-arm/morello-linux-ltp/-/commits/rev...
Cheers, Kevin
[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/... [2] https://lore.kernel.org/ltp/20231023135647.2157030-1-kevin.brodsky@arm.com/
Kevin Brodsky (3): syscalls/clone3: Avoid truncating pointers in purecap syscalls/prctl: Fix invalid pointer cast in purecap syscalls/sockioctl: Avoid out-of-bound uaccess in purecap
testcases/kernel/syscalls/clone3/clone301.c | 6 +++--- testcases/kernel/syscalls/prctl/prctl02.c | 15 +++++++++------ testcases/kernel/syscalls/sockioctl/sockioctl01.c | 2 +- 3 files changed, 13 insertions(+), 10 deletions(-)
Members of struct clone_args that represent pointers have already been enlarged to hold capabilities in PCuABI. However, for that to be useful, we also need to make sure that pointers are stored there as uintptr_t, instead of truncating them to 64-bit integers.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- testcases/kernel/syscalls/clone3/clone301.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/testcases/kernel/syscalls/clone3/clone301.c b/testcases/kernel/syscalls/clone3/clone301.c index d0fadbc5d033..75a67c0bde47 100644 --- a/testcases/kernel/syscalls/clone3/clone301.c +++ b/testcases/kernel/syscalls/clone3/clone301.c @@ -114,9 +114,9 @@ static void run(unsigned int n) pid_t pid;
args->flags = tc->flags; - args->pidfd = (uint64_t)(&pidfd); - args->child_tid = (uint64_t)(&child_tid); - args->parent_tid = (uint64_t)(&parent_tid); + args->pidfd = (uintptr_t)(&pidfd); + args->child_tid = (uintptr_t)(&child_tid); + args->parent_tid = (uintptr_t)(&parent_tid); args->exit_signal = tc->exit_signal; args->stack = 0; args->stack_size = 0;
The prctl02 testcases represent the third argument as unsigned long. In purecap, this is not appropriate for operations that take a pointer, such as PR_SET_SECCOMP.
Changing the struct to represent the third argument as uintptr_t would be invasive, due to the pointer indirection. Instead, special-case PR_SET_SECCOMP and use uintptr_t to represent its argument.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- testcases/kernel/syscalls/prctl/prctl02.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/testcases/kernel/syscalls/prctl/prctl02.c b/testcases/kernel/syscalls/prctl/prctl02.c index 1cd33f88ca55..d8b96a91fec7 100644 --- a/testcases/kernel/syscalls/prctl/prctl02.c +++ b/testcases/kernel/syscalls/prctl/prctl02.c @@ -67,9 +67,9 @@ static const struct sock_fprog strict = { .filter = (struct sock_filter *)strict_filter };
-static unsigned long strict_addr = (unsigned long)&strict; +static uintptr_t strict_addr = (uintptr_t)&strict; +static uintptr_t bad_addr;
-static unsigned long bad_addr; static unsigned long num_0; static unsigned long num_1 = 1; static unsigned long num_2 = 2; @@ -93,8 +93,8 @@ static struct tcase { {PR_SET_PDEATHSIG, &num_invalid, &num_0, EINVAL, "PR_SET_PDEATHSIG"}, {PR_SET_DUMPABLE, &num_2, &num_0, EINVAL, "PR_SET_DUMPABLE"}, {PR_SET_NAME, &bad_addr, &num_0, EFAULT, "PR_SET_NAME"}, - {PR_SET_SECCOMP, &num_2, &bad_addr, EFAULT, "PR_SET_SECCOMP"}, - {PR_SET_SECCOMP, &num_2, &strict_addr, EACCES, "PR_SET_SECCOMP"}, + {PR_SET_SECCOMP, &num_2, (unsigned long *)&bad_addr, EFAULT, "PR_SET_SECCOMP"}, + {PR_SET_SECCOMP, &num_2, (unsigned long *)&strict_addr, EACCES, "PR_SET_SECCOMP"}, {PR_SET_TIMING, &num_1, &num_0, EINVAL, "PR_SET_TIMING"}, {PR_SET_NO_NEW_PRIVS, &num_0, &num_0, EINVAL, "PR_SET_NO_NEW_PRIVS"}, {PR_SET_NO_NEW_PRIVS, &num_1, &num_1, EINVAL, "PR_SET_NO_NEW_PRIVS"}, @@ -152,7 +152,10 @@ static void verify_prctl(unsigned int n) break; }
- TEST(prctl(tc->option, *tc->arg2, *tc->arg3, 0, 0)); + if (tc->option == PR_SET_SECCOMP) + TEST(prctl(tc->option, *tc->arg2, *(uintptr_t *)tc->arg3, 0, 0)); + else + TEST(prctl(tc->option, *tc->arg2, *tc->arg3, 0, 0)); if (TST_RET == 0) { tst_res(TFAIL, "prctl() succeeded unexpectedly"); return; @@ -171,7 +174,7 @@ static void verify_prctl(unsigned int n)
static void setup(void) { - bad_addr = (unsigned long)tst_get_bad_addr(NULL); + bad_addr = (uintptr_t)tst_get_bad_addr(NULL);
TEST(prctl(PR_GET_SECCOMP)); if (TST_ERR == EINVAL)
ioctl(sock, SIOCATMARK, arg) is invalid if sock is a UDP socket, as SIOCATMARK is only supported for TCP sockets. In the case where the protocol doesn't recognise the command, sock_do_ioctl() (net/socket.c) attempts to hand it down to the NIC driver, and for that purpose interprets arg as a pointer to struct ifreq. However, since we are passing a pointer to int, which is smaller than struct ifreq, the kernel will attempt to read beyond the bounds of the specified capability pointer in purecap, resulting in -EFAULT instead of the expected -EINVAL.
Given that what arg actually points to is irrelevant to that testcase, let's just use a capability with large enough bounds, namely &if, to avoid getting -EFAULT in purecap.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- testcases/kernel/syscalls/sockioctl/sockioctl01.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/testcases/kernel/syscalls/sockioctl/sockioctl01.c b/testcases/kernel/syscalls/sockioctl/sockioctl01.c index 6ba39faae0e2..96ff9d14446e 100644 --- a/testcases/kernel/syscalls/sockioctl/sockioctl01.c +++ b/testcases/kernel/syscalls/sockioctl/sockioctl01.c @@ -101,7 +101,7 @@ struct test_case_t { , #endif { - PF_INET, SOCK_DGRAM, 0, SIOCATMARK, &optval, + PF_INET, SOCK_DGRAM, 0, SIOCATMARK, &ifr, (struct sockaddr *)&fsin1, sizeof(fsin1), -1, EINVAL, setup1, cleanup1, "ATMARK on UDP"} , {
Had a cursory look and the series looks good to me, it make sense.
Small nit : patch 3/3, "&if" in the commit message instead of "&ifr"
On 26/10/2023 13:23, Kevin Brodsky wrote:
Hi,
This series fixes a number of issues that the switch to checked (capability-based) uaccess [1] has revealed. These patches complete those that I have posted upstream [2] (and are already merged); they address issues that are likely to be too purecap-specific for upstream.
Review branch:
https://git.morello-project.org/kbrodsky-arm/morello-linux-ltp/-/commits/rev...
Cheers, Kevin
[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/... [2] https://lore.kernel.org/ltp/20231023135647.2157030-1-kevin.brodsky@arm.com/
Kevin Brodsky (3): syscalls/clone3: Avoid truncating pointers in purecap syscalls/prctl: Fix invalid pointer cast in purecap syscalls/sockioctl: Avoid out-of-bound uaccess in purecap
testcases/kernel/syscalls/clone3/clone301.c | 6 +++--- testcases/kernel/syscalls/prctl/prctl02.c | 15 +++++++++------ testcases/kernel/syscalls/sockioctl/sockioctl01.c | 2 +- 3 files changed, 13 insertions(+), 10 deletions(-)
linux-morello-ltp@op-lists.linaro.org
-
Kevin Brodsky -
Teo Couprie Diaz