[PATCH v2] selftests: powerpc: Add test for execute-disabled pkeys
Michael Ellerman
mpe at ellerman.id.au
Fri May 29 11:48:53 AEST 2020
Hi Sandipan,
A few comments below ...
Sandipan Das <sandipan at linux.ibm.com> writes:
> Apart from read and write access, memory protection keys can
> also be used for restricting execute permission of pages on
> powerpc. This adds a test to verify if the feature works as
> expected.
>
> Signed-off-by: Sandipan Das <sandipan at linux.ibm.com>
> ---
>
> Previous versions can be found at
> v1: https://lore.kernel.org/linuxppc-dev/20200508162332.65316-1-sandipan@linux.ibm.com/
>
> Changes in v2:
> - Added .gitignore entry for test binary.
> - Fixed builds for older distros where siginfo_t might not have si_pkey as
> a formal member based on discussion with Michael.
>
> ---
> tools/testing/selftests/powerpc/mm/.gitignore | 1 +
> tools/testing/selftests/powerpc/mm/Makefile | 3 +-
> .../selftests/powerpc/mm/pkey_exec_prot.c | 336 ++++++++++++++++++
> 3 files changed, 339 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
>
> diff --git a/tools/testing/selftests/powerpc/mm/.gitignore b/tools/testing/selftests/powerpc/mm/.gitignore
> index 2ca523255b1b..8f841f925baa 100644
> --- a/tools/testing/selftests/powerpc/mm/.gitignore
> +++ b/tools/testing/selftests/powerpc/mm/.gitignore
> @@ -8,3 +8,4 @@ wild_bctr
> large_vm_fork_separation
> bad_accesses
> tlbie_test
> +pkey_exec_prot
> diff --git a/tools/testing/selftests/powerpc/mm/Makefile b/tools/testing/selftests/powerpc/mm/Makefile
> index b9103c4bb414..2816229f648b 100644
> --- a/tools/testing/selftests/powerpc/mm/Makefile
> +++ b/tools/testing/selftests/powerpc/mm/Makefile
> @@ -3,7 +3,7 @@ noarg:
> $(MAKE) -C ../
>
> TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot prot_sao segv_errors wild_bctr \
> - large_vm_fork_separation bad_accesses
> + large_vm_fork_separation bad_accesses pkey_exec_prot
> TEST_GEN_PROGS_EXTENDED := tlbie_test
> TEST_GEN_FILES := tempfile
>
> @@ -17,6 +17,7 @@ $(OUTPUT)/prot_sao: ../utils.c
> $(OUTPUT)/wild_bctr: CFLAGS += -m64
> $(OUTPUT)/large_vm_fork_separation: CFLAGS += -m64
> $(OUTPUT)/bad_accesses: CFLAGS += -m64
> +$(OUTPUT)/pkey_exec_prot: CFLAGS += -m64
>
> $(OUTPUT)/tempfile:
> dd if=/dev/zero of=$@ bs=64k count=1
> diff --git a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
> new file mode 100644
> index 000000000000..147fb9ed47d5
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * Copyright 2020, Sandipan Das, IBM Corp.
> + *
> + * Test if applying execute protection on pages using memory
> + * protection keys works as expected.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <signal.h>
> +
> +#include <time.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +
> +#include "utils.h"
> +
> +/* Override definitions as they might be inconsistent */
Can you please expand the comment to say why/where you've seen problems,
so one day we can drop these once those old libcs are no longer around.
> +#undef PKEY_DISABLE_ACCESS
> +#define PKEY_DISABLE_ACCESS 0x3
> +
> +#undef PKEY_DISABLE_WRITE
> +#define PKEY_DISABLE_WRITE 0x2
> +
> +#undef PKEY_DISABLE_EXECUTE
> +#define PKEY_DISABLE_EXECUTE 0x4
> +
> +/* Older distros might not define this */
> +#ifndef SEGV_PKUERR
> +#define SEGV_PKUERR 4
> +#endif
> +
> +#define SI_PKEY_OFFSET 0x20
> +
> +#define SYS_pkey_mprotect 386
> +#define SYS_pkey_alloc 384
> +#define SYS_pkey_free 385
> +
> +#define PKEY_BITS_PER_PKEY 2
> +#define NR_PKEYS 32
> +
> +#define PKEY_BITS_MASK ((1UL << PKEY_BITS_PER_PKEY) - 1)
If you include "reg.h" then there's a mfspr()/mtspr() macro you can use.
> +static unsigned long pkeyreg_get(void)
> +{
> + unsigned long uamr;
The SPR is AMR not uamr?
> + asm volatile("mfspr %0, 0xd" : "=r"(uamr));
> + return uamr;
> +}
> +
> +static void pkeyreg_set(unsigned long uamr)
> +{
> + asm volatile("isync; mtspr 0xd, %0; isync;" : : "r"(uamr));
> +}
You can use mtspr() there, but you'll need to add the isync's yourself.
> +static void pkey_set_rights(int pkey, unsigned long rights)
> +{
> + unsigned long uamr, shift;
> +
> + shift = (NR_PKEYS - pkey - 1) * PKEY_BITS_PER_PKEY;
> + uamr = pkeyreg_get();
> + uamr &= ~(PKEY_BITS_MASK << shift);
> + uamr |= (rights & PKEY_BITS_MASK) << shift;
> + pkeyreg_set(uamr);
> +}
> +
> +static int sys_pkey_mprotect(void *addr, size_t len, int prot, int pkey)
> +{
> + return syscall(SYS_pkey_mprotect, addr, len, prot, pkey);
> +}
> +
> +static int sys_pkey_alloc(unsigned long flags, unsigned long rights)
> +{
> + return syscall(SYS_pkey_alloc, flags, rights);
> +}
> +
> +static int sys_pkey_free(int pkey)
> +{
> + return syscall(SYS_pkey_free, pkey);
> +}
> +
> +static volatile int fpkey, fcode, ftype, faults;
The "proper" type to use for things accessed in signal handlers is
volatile sig_atomic_t, which should work here AFACIS.
> +static unsigned long pgsize, numinsns;
> +static volatile unsigned int *faddr;
> +static unsigned int *insns;
> +
> +static void segv_handler(int signum, siginfo_t *sinfo, void *ctx)
> +{
> + int pkey;
> +
> +#ifdef si_pkey
> + pkey = sinfo->si_pkey;
> +#else
> + pkey = *((int *)(((char *) sinfo) + SI_PKEY_OFFSET));
> +#endif
> +
> + /* Check if this fault originated because of the expected reasons */
> + if (sinfo->si_code != SEGV_ACCERR && sinfo->si_code != SEGV_PKUERR) {
> + printf("got an unexpected fault, code = %d\n",
> + sinfo->si_code);
printf() isn't signal safe, so this is a bit dicey. You can call
write(2) if you really want to.
If this is an unexpected condition you might better to just call
_exit(1) to bail out.
> + goto fail;
> + }
> +
> + /* Check if this fault originated from the expected address */
> + if (sinfo->si_addr != (void *) faddr) {
> + printf("got an unexpected fault, addr = %p\n",
> + sinfo->si_addr);
> + goto fail;
> + }
> +
> + /* Check if the expected number of faults has been exceeded */
> + if (faults == 0)
> + goto fail;
> +
> + fcode = sinfo->si_code;
> +
> + /* Restore permissions in order to continue */
> + switch (fcode) {
> + case SEGV_ACCERR:
> + if (mprotect(insns, pgsize, PROT_READ | PROT_WRITE)) {
mprotect() also isn't listed as being signal safe, though I don't see
why not. So that's probably fine for test code. We could always call the
syscall directly if necessary.
> + perror("mprotect");
> + goto fail;
> + }
> + break;
> + case SEGV_PKUERR:
> + if (pkey != fpkey)
> + goto fail;
> +
> + if (ftype == PKEY_DISABLE_ACCESS) {
> + pkey_set_rights(fpkey, 0);
> + } else if (ftype == PKEY_DISABLE_EXECUTE) {
> + /*
> + * Reassociate the exec-only pkey with the region
> + * to be able to continue. Unlike AMR, we cannot
> + * set IAMR directly from userspace to restore the
> + * permissions.
> + */
> + if (mprotect(insns, pgsize, PROT_EXEC)) {
> + perror("mprotect");
> + goto fail;
> + }
> + } else {
> + goto fail;
> + }
> + break;
> + }
> +
> + faults--;
> + return;
> +
> +fail:
> + /* Restore all page permissions to avoid repetitive faults */
> + if (mprotect(insns, pgsize, PROT_READ | PROT_WRITE | PROT_EXEC))
> + perror("mprotect");
> + if (sinfo->si_code == SEGV_PKUERR)
> + pkey_set_rights(pkey, 0);
> + faults = -1; /* Something unexpected happened */
> +}
> +
> +static int pkeys_unsupported(void)
> +{
> + bool using_hash = false;
> + char line[128];
> + int pkey;
> + FILE *f;
> +
> + f = fopen("/proc/cpuinfo", "r");
> + FAIL_IF(!f);
> +
> + /* Protection keys are currently supported on Hash MMU only */
> + while (fgets(line, sizeof(line), f)) {
> + if (strcmp(line, "MMU : Hash\n") == 0) {
> + using_hash = true;
> + break;
> + }
> + }
We already have using_hash_mmu() in the bad_accesses.c test.
Can you move using_hash_mmu() into
tools/testing/selftests/powerpc/utils.c, and declare it in
tools/testing/selftests/powerpc/include/utils.h and then use it in your
test.
> + fclose(f);
> + SKIP_IF(!using_hash);
> +
> + /* Check if the system call is supported */
> + pkey = sys_pkey_alloc(0, 0);
> + SKIP_IF(pkey < 0);
> + sys_pkey_free(pkey);
> +
> + return 0;
> +}
> +
> +static int test(void)
> +{
> + struct sigaction act;
> + int pkey, ret, i;
> +
> + ret = pkeys_unsupported();
> + if (ret)
> + return ret;
> +
> + /* Setup signal handler */
> + act.sa_handler = 0;
> + act.sa_sigaction = segv_handler;
> + FAIL_IF(sigprocmask(SIG_SETMASK, 0, &act.sa_mask) != 0);
> + act.sa_flags = SA_SIGINFO;
> + act.sa_restorer = 0;
> + FAIL_IF(sigaction(SIGSEGV, &act, NULL) != 0);
> +
> + /* Setup executable region */
> + pgsize = sysconf(_SC_PAGESIZE);
getpagesize() is cleaner.
> + numinsns = pgsize / sizeof(unsigned int);
> + insns = (unsigned int *) mmap(NULL, pgsize, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> + FAIL_IF(insns == MAP_FAILED);
> +
> + /* Write the instruction words */
> + for (i = 0; i < numinsns - 1; i++)
> + insns[i] = 0x60000000; /* nop */
> +
> + /*
> + * Later, to jump to the executable region, we use a linked
> + * branch which sets the return address automatically in LR.
"linked branch" is usually called "branch and link".
> + * Use that to return back.
> + */
> + insns[numinsns - 1] = 0x4e800020; /* blr */
> +
> + /* Allocate a pkey that restricts execution */
> + pkey = sys_pkey_alloc(0, PKEY_DISABLE_EXECUTE);
> + FAIL_IF(pkey < 0);
> +
> + /*
> + * Pick a random instruction address from the executable
> + * region.
> + */
> + srand(time(NULL));
> + faddr = &insns[rand() % (numinsns - 1)];
I'm not really sure the randomisation adds much, given it's only
randomised within the page and the protections only operate at page
granularity.
> +
> + /* The following two cases will avoid SEGV_PKUERR */
> + ftype = -1;
> + fpkey = -1;
> +
> + /*
> + * Read an instruction word from the address when AMR bits
> + * are not set.
You should explain for people who aren't familiar with the ISA that "AMR
bits not set" means "read/write access allowed".
> + *
> + * This should not generate a fault as having PROT_EXEC
> + * implicitly allows reads. The pkey currently restricts
Whether PROT_EXEC implies read is not well defined (see the man page).
If you want to test this case I think you'd be better off specifying
PROT_EXEC | PROT_READ explicitly.
> + * execution only based on the IAMR bits. The AMR bits are
> + * cleared.
> + */
> + faults = 0;
> + FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
> + printf("read from %p, pkey is execute-disabled\n", (void *) faddr);
> + i = *faddr;
> + FAIL_IF(faults != 0);
> +
> + /*
> + * Write an instruction word to the address when AMR bits
> + * are not set.
> + *
> + * This should generate an access fault as having just
> + * PROT_EXEC also restricts writes. The pkey currently
OK that one is correct, PROT_EXEC without PROT_WRITE must prevent writes.
> + * restricts execution only based on the IAMR bits. The
> + * AMR bits are cleared.
> + */
> + faults = 1;
> + FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
> + printf("write to %p, pkey is execute-disabled\n", (void *) faddr);
> + *faddr = 0x60000000; /* nop */
faddr is already == nop because you set the entire page to nops previously.
It would be a more convincing test if you set faddr to a trap at the
beginning, that way later when you execute it you can test that the
write of the nop succeeded.
> + FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
> +
> + /* The following three cases will generate SEGV_PKUERR */
> + ftype = PKEY_DISABLE_ACCESS;
> + fpkey = pkey;
> +
> + /*
> + * Read an instruction word from the address when AMR bits
> + * are set.
> + *
> + * This should generate a pkey fault based on AMR bits only
> + * as having PROT_EXEC implicitly allows reads.
Again would be better to specify PROT_READ IMHO.
> + */
> + faults = 1;
> + FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
> + printf("read from %p, pkey is execute-disabled, access-disabled\n",
> + (void *) faddr);
> + pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
> + i = *faddr;
> + FAIL_IF(faults != 0 || fcode != SEGV_PKUERR);
> +
> + /*
> + * Write an instruction word to the address when AMR bits
> + * are set.
> + *
> + * This should generate two faults. First, a pkey fault based
> + * on AMR bits and then an access fault based on PROT_EXEC.
> + */
> + faults = 2;
Setting faults to the expected value and decrementing it in the fault
handler is kind of weird.
I think it would be clearer if faults was just a zero-based counter of
how many faults we've taken, and then you test that it's == 2 below.
> + FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
> + printf("write to %p, pkey is execute-disabled, access-disabled\n",
> + (void *) faddr);
> + pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
> + *faddr = 0x60000000; /* nop */
> + FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
ie. FAIL_IF(faults != 2 || ... )
> + /*
> + * Jump to the executable region. This should generate a pkey
> + * fault based on IAMR bits. AMR bits will not affect execution.
> + */
> + faddr = insns;
> + ftype = PKEY_DISABLE_EXECUTE;
> + fpkey = pkey;
> + faults = 1;
> + FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
> + pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
> + printf("execute at %p, ", (void *) faddr);
> + printf("pkey is execute-disabled, access-disabled\n");
> +
> + /* Branch into the executable region */
> + asm volatile("mtctr %0" : : "r"((unsigned long) insns));
> + asm volatile("bctrl");
I'm not sure that's safe, they should be part of a single asm block.
> + FAIL_IF(faults != 0 || fcode != SEGV_PKUERR);
I think as a final test you should remove the protections and confirm
you can successfully execute from the insns page.
> + /* Cleanup */
> + munmap((void *) insns, pgsize);
> + sys_pkey_free(pkey);
> +
> + return 0;
> +}
cheers
More information about the Linuxppc-dev
mailing list