| Message ID | 20240808065732.64328-1-liwang@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pkey01: Adding test for PKEY_DISABLE_EXECUTE | expand |
Hi Li, > The pkey_test function now includes a code snippet to test execute > permissions, ensuring proper handling of execution rights in memory > protection keys. Reviewed-by: Petr Vorel <pvorel@suse.cz> LGTM, thanks! Few notes + tiny fixes below (which can be done before merge). ... > + pkey = pkey_alloc(0, PKEY_DISABLE_EXECUTE); > + if (pkey == -1) { > + if (errno == EINVAL) { > + tst_res(TINFO, "PKEY_DISABLE_EXECUTE hasn't implement."); nit: maybe "PKEY_DISABLE_EXECUTE not implemented" > + execute_supported = 0; > + } else { > + tst_brk(TBROK | TERRNO, "pkey_alloc failed"); > + } > + } > + pkey_free(pkey); ... > static void pkey_test(struct tcase *tc, struct mmap_param *mpa) > { > pid_t pid; > char *buffer; > int pkey, status; > int fd = mpa->fd; > + size_t (*func)(); > + size_t func_size = -1; > + > + if (!execute_supported && (tc->access_rights == PKEY_DISABLE_EXECUTE)) > + tst_brk(TCONF, "skip PKEY_DISABLE_EXECUTE test"); NOTE due to possible tst_brk() it'd be good to keep PKEY_DISABLE_EXECUTE as a last test. Not sure if it's worth to add a comment (or replace with tst_res() and return). ... > + if (mpa->prot == (PROT_READ | PROT_WRITE | PROT_EXEC)) { > + func_size = function_size((void (*)(void))function_size); > + memcpy(buffer, (void *)function_size, func_size); > + } > + > pkey = pkey_alloc(tc->flags, tc->access_rights); > if (pkey == -1) > tst_brk(TBROK | TERRNO, "pkey_alloc failed"); > @@ -169,6 +203,10 @@ static void pkey_test(struct tcase *tc, struct mmap_param *mpa) > tst_res(TFAIL | TERRNO, > "Write buffer success, buffer[0] = %d", *buffer); > break; > + case PKEY_DISABLE_EXECUTE: > + func = (size_t (*)())buffer;; nit: double ; (please remove the last one). > + tst_res(TFAIL | TERRNO, "Excute buffer result = %lu", func(func)); s/%lu/%zi/ (otherwise it introduces warnings on 32bit) nit: s/Excute/Execute/ > + break; > } > exit(0); > } > @@ -193,10 +231,16 @@ static void pkey_test(struct tcase *tc, struct mmap_param *mpa) > tst_res(TPASS, "Write buffer success, buffer[0] = %d", *buffer); > break; > case PROT_READ | PROT_WRITE: > - case PROT_READ | PROT_WRITE | PROT_EXEC: > *buffer = 'a'; > tst_res(TPASS, "Read & Write buffer success, buffer[0] = %d", *buffer); > break; > + case PROT_READ | PROT_WRITE | PROT_EXEC: > + func = (size_t (*)())buffer;; > + if (func_size == func(func)) > + tst_res(TPASS, "Execute buffer success, result = %lu", func_size); s/%lu/%zi/ > + else > + tst_res(TFAIL, "Execute buffer with unexpected result: %lu", func(func)); s/%lu/%zi/ Kind regards, Petr
Hi Petr, Petr Vorel <pvorel@suse.cz> wrote: > > + if (!execute_supported && (tc->access_rights == > PKEY_DISABLE_EXECUTE)) > > + tst_brk(TCONF, "skip PKEY_DISABLE_EXECUTE test"); > NOTE due to possible tst_brk() it'd be good to keep PKEY_DISABLE_EXECUTE > as a > last test. Not sure if it's worth to add a comment (or replace with > tst_res() > and return). > Or, we can use 'tst_res + return' which is similar to tst_hugepages missing way. The rest comments all look good.
Pushed with minor changes as you suggested. Thanks! P.s. The new test passed on my POWER9+kernel-6.10 and Skylake-SP+ kernel-5.14.
diff --git a/include/lapi/pkey.h b/include/lapi/pkey.h index 398e3be5f..eb9bf7fb4 100644 --- a/include/lapi/pkey.h +++ b/include/lapi/pkey.h @@ -17,6 +17,10 @@ # define PKEY_DISABLE_WRITE 0x2 #endif +#ifndef PKEY_DISABLE_EXECUTE +# define PKEY_DISABLE_EXECUTE 0x4 +#endif + #ifndef HAVE_PKEY_MPROTECT inline int pkey_mprotect(void *addr, size_t len, int prot, int pkey) { diff --git a/testcases/kernel/syscalls/pkeys/pkey01.c b/testcases/kernel/syscalls/pkeys/pkey01.c index f1ecfec0b..70fdffe48 100644 --- a/testcases/kernel/syscalls/pkeys/pkey01.c +++ b/testcases/kernel/syscalls/pkeys/pkey01.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later /* - * Copyright (c) 2019 Red Hat, Inc. + * Copyright (c) 2019-2024 Red Hat, Inc. */ /*\ @@ -41,6 +41,7 @@ #define PATH_VM_NRHPS "/proc/sys/vm/nr_hugepages" static int size; +static int execute_supported = 1; static struct tcase { unsigned long flags; @@ -49,14 +50,26 @@ static struct tcase { } tcases[] = { {0, PKEY_DISABLE_ACCESS, "PKEY_DISABLE_ACCESS"}, {0, PKEY_DISABLE_WRITE, "PKEY_DISABLE_WRITE"}, + {0, PKEY_DISABLE_EXECUTE, "PKEY_DISABLE_EXECUTE"}, }; static void setup(void) { - int i, fd; + int i, fd, pkey; check_pkey_support(); + pkey = pkey_alloc(0, PKEY_DISABLE_EXECUTE); + if (pkey == -1) { + if (errno == EINVAL) { + tst_res(TINFO, "PKEY_DISABLE_EXECUTE hasn't implement."); + execute_supported = 0; + } else { + tst_brk(TBROK | TERRNO, "pkey_alloc failed"); + } + } + pkey_free(pkey); + if (tst_hugepages == test.hugepages.number) size = SAFE_READ_MEMINFO("Hugepagesize:") * 1024; else @@ -130,12 +143,28 @@ static char *flag_to_str(int flags) } } +static size_t function_size(void (*func)(void)) +{ + unsigned char *start = (unsigned char *)func; + unsigned char *end = start; + + while (*end != 0xC3 && *end != 0xC2) + end++; + + return (size_t)(end - start + 1); +} + static void pkey_test(struct tcase *tc, struct mmap_param *mpa) { pid_t pid; char *buffer; int pkey, status; int fd = mpa->fd; + size_t (*func)(); + size_t func_size = -1; + + if (!execute_supported && (tc->access_rights == PKEY_DISABLE_EXECUTE)) + tst_brk(TCONF, "skip PKEY_DISABLE_EXECUTE test"); if (!tst_hugepages && (mpa->flags & MAP_HUGETLB)) { tst_res(TINFO, "Skip test on (%s) buffer", flag_to_str(mpa->flags)); @@ -147,6 +176,11 @@ static void pkey_test(struct tcase *tc, struct mmap_param *mpa) buffer = SAFE_MMAP(NULL, size, mpa->prot, mpa->flags, fd, 0); + if (mpa->prot == (PROT_READ | PROT_WRITE | PROT_EXEC)) { + func_size = function_size((void (*)(void))function_size); + memcpy(buffer, (void *)function_size, func_size); + } + pkey = pkey_alloc(tc->flags, tc->access_rights); if (pkey == -1) tst_brk(TBROK | TERRNO, "pkey_alloc failed"); @@ -169,6 +203,10 @@ static void pkey_test(struct tcase *tc, struct mmap_param *mpa) tst_res(TFAIL | TERRNO, "Write buffer success, buffer[0] = %d", *buffer); break; + case PKEY_DISABLE_EXECUTE: + func = (size_t (*)())buffer;; + tst_res(TFAIL | TERRNO, "Excute buffer result = %lu", func(func)); + break; } exit(0); } @@ -193,10 +231,16 @@ static void pkey_test(struct tcase *tc, struct mmap_param *mpa) tst_res(TPASS, "Write buffer success, buffer[0] = %d", *buffer); break; case PROT_READ | PROT_WRITE: - case PROT_READ | PROT_WRITE | PROT_EXEC: *buffer = 'a'; tst_res(TPASS, "Read & Write buffer success, buffer[0] = %d", *buffer); break; + case PROT_READ | PROT_WRITE | PROT_EXEC: + func = (size_t (*)())buffer;; + if (func_size == func(func)) + tst_res(TPASS, "Execute buffer success, result = %lu", func_size); + else + tst_res(TFAIL, "Execute buffer with unexpected result: %lu", func(func)); + break; } if (fd >= 0)
The pkey_test function now includes a code snippet to test execute permissions, ensuring proper handling of execution rights in memory protection keys. Signed-off-by: Li Wang <liwang@redhat.com> --- include/lapi/pkey.h | 4 ++ testcases/kernel/syscalls/pkeys/pkey01.c | 50 ++++++++++++++++++++++-- 2 files changed, 51 insertions(+), 3 deletions(-)