| Message ID | 20241115154434.39461-1-mdoucha@suse.cz |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE" | expand |
Sorry, I forgot to CC Li in the patch. On 15. 11. 24 16:44, Martin Doucha wrote: > This reverts commit d2b8a476c29d52c484b387454082bbc906b0b4f8. > > Remove the PKEY_DISABLE_EXECUTE subtest. The function_size() code > is broken in a way that I cannot easily fix. The function tries > to calculate the size of a function by finding the first RET > instruction. However, in 32bit LTP builds, the code gets compiled > to this: > > 0804b690 <function_size>: > 804b690: 8b 4c 24 04 mov 0x4(%esp),%ecx > 804b694: 0f b6 01 movzbl (%ecx),%eax > 804b697: 83 c0 3e add $0x3e,%eax > 804b69a: 3c 01 cmp $0x1,%al > 804b69c: 76 1a jbe 804b6b8 <function_size+0x28> > 804b69e: 89 c8 mov %ecx,%eax > 804b6a0: 83 c0 01 add $0x1,%eax > 804b6a3: 0f b6 10 movzbl (%eax),%edx > 804b6a6: 83 c2 3e add $0x3e,%edx > 804b6a9: 80 fa 01 cmp $0x1,%dl > 804b6ac: 77 f2 ja 804b6a0 <function_size+0x10> > 804b6ae: 29 c8 sub %ecx,%eax > 804b6b0: 83 c0 10 add $0x10,%eax > 804b6b3: c3 ret > 804b6b4: 8d 74 26 00 lea 0x0(%esi,%eiz,1),%esi > 804b6b8: b8 10 00 00 00 mov $0x10,%eax > 804b6bd: c3 ret > 804b6be: 66 90 xchg %ax,%ax > > If you look closely enough, you'll notice a C2 byte in add $0x3e,%edx > instruction on address 804b6a6. The function will assume this byte is > a RET instruction, return a size that's 22 bytes too short and then > the code execution inside the executable buffer will run past the end > of buffer, resulting in a segfault. > > Signed-off-by: Martin Doucha <mdoucha@suse.cz> > --- > testcases/kernel/syscalls/pkeys/pkey01.c | 52 ++---------------------- > 1 file changed, 3 insertions(+), 49 deletions(-) > > diff --git a/testcases/kernel/syscalls/pkeys/pkey01.c b/testcases/kernel/syscalls/pkeys/pkey01.c > index c041cbcfd..e49e48846 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-2024 Red Hat, Inc. > + * Copyright (c) 2019 Red Hat, Inc. > */ > > /*\ > @@ -41,7 +41,6 @@ > #define PATH_VM_NRHPS "/proc/sys/vm/nr_hugepages" > > static int size; > -static int execute_supported = 1; > > #define PERM_NAME(x) .access_rights = x, .name = #x > static struct tcase { > @@ -51,26 +50,14 @@ static struct tcase { > } tcases[] = { > {PERM_NAME(PKEY_DISABLE_ACCESS)}, > {PERM_NAME(PKEY_DISABLE_WRITE)}, > - {PERM_NAME(PKEY_DISABLE_EXECUTE)} /* keep it the last */ > }; > > static void setup(void) > { > - int i, fd, pkey; > + int i, fd; > > check_pkey_support(); > > - pkey = pkey_alloc(0, PKEY_DISABLE_EXECUTE); > - if (pkey == -1) { > - if (errno == EINVAL) { > - tst_res(TINFO, "PKEY_DISABLE_EXECUTE not implemented"); > - 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 > @@ -144,17 +131,6 @@ 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); > -} > - > /* > * return: 1 if it's safe to quit testing on failure (all following would be > * TCONF, O otherwise. > @@ -165,13 +141,6 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa) > char *buffer; > int pkey, status; > int fd = mpa->fd; > - size_t (*func)(); > - size_t func_size = 0; > - > - if (!execute_supported && (tc->access_rights == PKEY_DISABLE_EXECUTE)) { > - tst_res(TCONF, "skip PKEY_DISABLE_EXECUTE test"); > - return 1; > - } > > if (!tst_hugepages && (mpa->flags & MAP_HUGETLB)) { > tst_res(TCONF, "Skip test on (%s) buffer", flag_to_str(mpa->flags)); > @@ -183,11 +152,6 @@ static int 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"); > @@ -210,10 +174,6 @@ static int 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, "Execute buffer result = %zi", func(func)); > - break; > } > exit(0); > } > @@ -238,16 +198,10 @@ static int 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 = %zi", func_size); > - else > - tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func(func)); > - break; > } > > if (fd >= 0)
Hi Li, Martin, > Sorry, I forgot to CC Li in the patch. > On 15. 11. 24 16:44, Martin Doucha wrote: > > This reverts commit d2b8a476c29d52c484b387454082bbc906b0b4f8. > > Remove the PKEY_DISABLE_EXECUTE subtest. The function_size() code > > is broken in a way that I cannot easily fix. The function tries > > to calculate the size of a function by finding the first RET > > instruction. However, in 32bit LTP builds, the code gets compiled > > to this: > > 0804b690 <function_size>: > > 804b690: 8b 4c 24 04 mov 0x4(%esp),%ecx > > 804b694: 0f b6 01 movzbl (%ecx),%eax > > 804b697: 83 c0 3e add $0x3e,%eax > > 804b69a: 3c 01 cmp $0x1,%al > > 804b69c: 76 1a jbe 804b6b8 <function_size+0x28> > > 804b69e: 89 c8 mov %ecx,%eax > > 804b6a0: 83 c0 01 add $0x1,%eax > > 804b6a3: 0f b6 10 movzbl (%eax),%edx > > 804b6a6: 83 c2 3e add $0x3e,%edx > > 804b6a9: 80 fa 01 cmp $0x1,%dl > > 804b6ac: 77 f2 ja 804b6a0 <function_size+0x10> > > 804b6ae: 29 c8 sub %ecx,%eax > > 804b6b0: 83 c0 10 add $0x10,%eax > > 804b6b3: c3 ret > > 804b6b4: 8d 74 26 00 lea 0x0(%esi,%eiz,1),%esi > > 804b6b8: b8 10 00 00 00 mov $0x10,%eax > > 804b6bd: c3 ret > > 804b6be: 66 90 xchg %ax,%ax > > If you look closely enough, you'll notice a C2 byte in add $0x3e,%edx > > instruction on address 804b6a6. The function will assume this byte is > > a RET instruction, return a size that's 22 bytes too short and then > > the code execution inside the executable buffer will run past the end > > of buffer, resulting in a segfault. Martin, thanks a lot for debugging! Acked-by: Petr Vorel <pvorel@suse.cz> Other option would be to disable this test only for 32bit (keep it for 64bit). Li, any change you could have look into it? Kind regards, Petr
On Fri, Nov 15, 2024 at 7:58 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, Martin, > > > Sorry, I forgot to CC Li in the patch. > > > On 15. 11. 24 16:44, Martin Doucha wrote: > > > This reverts commit d2b8a476c29d52c484b387454082bbc906b0b4f8. > > > > Remove the PKEY_DISABLE_EXECUTE subtest. The function_size() code > > > is broken in a way that I cannot easily fix. The function tries > > > to calculate the size of a function by finding the first RET > > > instruction. However, in 32bit LTP builds, the code gets compiled > > > to this: > > > > 0804b690 <function_size>: > > > 804b690: 8b 4c 24 04 mov 0x4(%esp),%ecx > > > 804b694: 0f b6 01 movzbl (%ecx),%eax > > > 804b697: 83 c0 3e add $0x3e,%eax > > > 804b69a: 3c 01 cmp $0x1,%al > > > 804b69c: 76 1a jbe 804b6b8 <function_size+0x28> > > > 804b69e: 89 c8 mov %ecx,%eax > > > 804b6a0: 83 c0 01 add $0x1,%eax > > > 804b6a3: 0f b6 10 movzbl (%eax),%edx > > > 804b6a6: 83 c2 3e add $0x3e,%edx > > > 804b6a9: 80 fa 01 cmp $0x1,%dl > > > 804b6ac: 77 f2 ja 804b6a0 <function_size+0x10> > > > 804b6ae: 29 c8 sub %ecx,%eax > > > 804b6b0: 83 c0 10 add $0x10,%eax > > > 804b6b3: c3 ret > > > 804b6b4: 8d 74 26 00 lea 0x0(%esi,%eiz,1),%esi > > > 804b6b8: b8 10 00 00 00 mov $0x10,%eax > > > 804b6bd: c3 ret > > > 804b6be: 66 90 xchg %ax,%ax > > > > If you look closely enough, you'll notice a C2 byte in add $0x3e,%edx > > > instruction on address 804b6a6. The function will assume this byte is > > > a RET instruction, return a size that's 22 bytes too short and then > > > the code execution inside the executable buffer will run past the end > > > of buffer, resulting in a segfault. > > Martin, thanks a lot for debugging! > > Acked-by: Petr Vorel <pvorel@suse.cz> > > Other option would be to disable this test only for 32bit (keep it for 64bit). > Li, any change you could have look into it? I vaguely recall we already dealt with something similar for a different test, which copied function, then changed protection to EXEC and finally ran it. I'll see if I can find it. > > Kind regards, > Petr > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp >
On Tue, Nov 19, 2024 at 2:11 PM Jan Stancek <jstancek@redhat.com> wrote: > > On Fri, Nov 15, 2024 at 7:58 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > Hi Li, Martin, > > > > > Sorry, I forgot to CC Li in the patch. > > > > > On 15. 11. 24 16:44, Martin Doucha wrote: > > > > This reverts commit d2b8a476c29d52c484b387454082bbc906b0b4f8. > > > > > > Remove the PKEY_DISABLE_EXECUTE subtest. The function_size() code > > > > is broken in a way that I cannot easily fix. The function tries > > > > to calculate the size of a function by finding the first RET > > > > instruction. However, in 32bit LTP builds, the code gets compiled > > > > to this: > > > > > > 0804b690 <function_size>: > > > > 804b690: 8b 4c 24 04 mov 0x4(%esp),%ecx > > > > 804b694: 0f b6 01 movzbl (%ecx),%eax > > > > 804b697: 83 c0 3e add $0x3e,%eax > > > > 804b69a: 3c 01 cmp $0x1,%al > > > > 804b69c: 76 1a jbe 804b6b8 <function_size+0x28> > > > > 804b69e: 89 c8 mov %ecx,%eax > > > > 804b6a0: 83 c0 01 add $0x1,%eax > > > > 804b6a3: 0f b6 10 movzbl (%eax),%edx > > > > 804b6a6: 83 c2 3e add $0x3e,%edx > > > > 804b6a9: 80 fa 01 cmp $0x1,%dl > > > > 804b6ac: 77 f2 ja 804b6a0 <function_size+0x10> > > > > 804b6ae: 29 c8 sub %ecx,%eax > > > > 804b6b0: 83 c0 10 add $0x10,%eax > > > > 804b6b3: c3 ret > > > > 804b6b4: 8d 74 26 00 lea 0x0(%esi,%eiz,1),%esi > > > > 804b6b8: b8 10 00 00 00 mov $0x10,%eax > > > > 804b6bd: c3 ret > > > > 804b6be: 66 90 xchg %ax,%ax > > > > > > If you look closely enough, you'll notice a C2 byte in add $0x3e,%edx > > > > instruction on address 804b6a6. The function will assume this byte is > > > > a RET instruction, return a size that's 22 bytes too short and then > > > > the code execution inside the executable buffer will run past the end > > > > of buffer, resulting in a segfault. > > > > Martin, thanks a lot for debugging! > > > > Acked-by: Petr Vorel <pvorel@suse.cz> > > > > Other option would be to disable this test only for 32bit (keep it for 64bit). > > Li, any change you could have look into it? > > I vaguely recall we already dealt with something similar for a > different test, which > copied function, then changed protection to EXEC and finally ran it. > I'll see if I can find it. Do we need function_size() to return accurate number? In mprotect04 we ended up using CFLAGS += -falign-functions=64 and copying everything until end of page. Function was dummy, so it always fit and it's not an issue if we copied more.
On Tue, Nov 19, 2024 at 9:32 PM Jan Stancek <jstancek@redhat.com> wrote: > On Tue, Nov 19, 2024 at 2:11 PM Jan Stancek <jstancek@redhat.com> wrote: > > > > On Fri, Nov 15, 2024 at 7:58 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > > > Hi Li, Martin, > > > > > > > Sorry, I forgot to CC Li in the patch. > > > > > > > On 15. 11. 24 16:44, Martin Doucha wrote: > > > > > This reverts commit d2b8a476c29d52c484b387454082bbc906b0b4f8. > > > > > > > > Remove the PKEY_DISABLE_EXECUTE subtest. The function_size() code > > > > > is broken in a way that I cannot easily fix. The function tries > > > > > to calculate the size of a function by finding the first RET > > > > > instruction. However, in 32bit LTP builds, the code gets compiled > > > > > to this: > > > > > > > > 0804b690 <function_size>: > > > > > 804b690: 8b 4c 24 04 mov 0x4(%esp),%ecx > > > > > 804b694: 0f b6 01 movzbl (%ecx),%eax > > > > > 804b697: 83 c0 3e add $0x3e,%eax > > > > > 804b69a: 3c 01 cmp $0x1,%al > > > > > 804b69c: 76 1a jbe 804b6b8 > <function_size+0x28> > > > > > 804b69e: 89 c8 mov %ecx,%eax > > > > > 804b6a0: 83 c0 01 add $0x1,%eax > > > > > 804b6a3: 0f b6 10 movzbl (%eax),%edx > > > > > 804b6a6: 83 c2 3e add $0x3e,%edx > > > > > 804b6a9: 80 fa 01 cmp $0x1,%dl > > > > > 804b6ac: 77 f2 ja 804b6a0 > <function_size+0x10> > > > > > 804b6ae: 29 c8 sub %ecx,%eax > > > > > 804b6b0: 83 c0 10 add $0x10,%eax > > > > > 804b6b3: c3 ret > > > > > 804b6b4: 8d 74 26 00 lea > 0x0(%esi,%eiz,1),%esi > > > > > 804b6b8: b8 10 00 00 00 mov $0x10,%eax > > > > > 804b6bd: c3 ret > > > > > 804b6be: 66 90 xchg %ax,%ax > > > > > > > > If you look closely enough, you'll notice a C2 byte in add > $0x3e,%edx > > > > > instruction on address 804b6a6. The function will assume this byte > is > > > > > a RET instruction, return a size that's 22 bytes too short and then > > > > > the code execution inside the executable buffer will run past the > end > > > > > of buffer, resulting in a segfault. > > > > > > Martin, thanks a lot for debugging! > > > > > > Acked-by: Petr Vorel <pvorel@suse.cz> > > > > > > Other option would be to disable this test only for 32bit (keep it for > 64bit). > > > Li, any change you could have look into it? > > > > I vaguely recall we already dealt with something similar for a > > different test, which > > copied function, then changed protection to EXEC and finally ran it. > > I'll see if I can find it. > > Do we need function_size() to return accurate number? In mprotect04 > That is a simple method to check that the buffer can be executed correctly. > we ended up using CFLAGS += -falign-functions=64 and copying everything > until end of page. Function was dummy, so it always fit and it's not an > issue if we copied more. > Yes, this could work as well, thanks for improving it. P.s I am on traveling this week, so I wouldn't find a machine to test the patch.
diff --git a/testcases/kernel/syscalls/pkeys/pkey01.c b/testcases/kernel/syscalls/pkeys/pkey01.c index c041cbcfd..e49e48846 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-2024 Red Hat, Inc. + * Copyright (c) 2019 Red Hat, Inc. */ /*\ @@ -41,7 +41,6 @@ #define PATH_VM_NRHPS "/proc/sys/vm/nr_hugepages" static int size; -static int execute_supported = 1; #define PERM_NAME(x) .access_rights = x, .name = #x static struct tcase { @@ -51,26 +50,14 @@ static struct tcase { } tcases[] = { {PERM_NAME(PKEY_DISABLE_ACCESS)}, {PERM_NAME(PKEY_DISABLE_WRITE)}, - {PERM_NAME(PKEY_DISABLE_EXECUTE)} /* keep it the last */ }; static void setup(void) { - int i, fd, pkey; + int i, fd; check_pkey_support(); - pkey = pkey_alloc(0, PKEY_DISABLE_EXECUTE); - if (pkey == -1) { - if (errno == EINVAL) { - tst_res(TINFO, "PKEY_DISABLE_EXECUTE not implemented"); - 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 @@ -144,17 +131,6 @@ 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); -} - /* * return: 1 if it's safe to quit testing on failure (all following would be * TCONF, O otherwise. @@ -165,13 +141,6 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa) char *buffer; int pkey, status; int fd = mpa->fd; - size_t (*func)(); - size_t func_size = 0; - - if (!execute_supported && (tc->access_rights == PKEY_DISABLE_EXECUTE)) { - tst_res(TCONF, "skip PKEY_DISABLE_EXECUTE test"); - return 1; - } if (!tst_hugepages && (mpa->flags & MAP_HUGETLB)) { tst_res(TCONF, "Skip test on (%s) buffer", flag_to_str(mpa->flags)); @@ -183,11 +152,6 @@ static int 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"); @@ -210,10 +174,6 @@ static int 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, "Execute buffer result = %zi", func(func)); - break; } exit(0); } @@ -238,16 +198,10 @@ static int 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 = %zi", func_size); - else - tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func(func)); - break; } if (fd >= 0)
This reverts commit d2b8a476c29d52c484b387454082bbc906b0b4f8. Remove the PKEY_DISABLE_EXECUTE subtest. The function_size() code is broken in a way that I cannot easily fix. The function tries to calculate the size of a function by finding the first RET instruction. However, in 32bit LTP builds, the code gets compiled to this: 0804b690 <function_size>: 804b690: 8b 4c 24 04 mov 0x4(%esp),%ecx 804b694: 0f b6 01 movzbl (%ecx),%eax 804b697: 83 c0 3e add $0x3e,%eax 804b69a: 3c 01 cmp $0x1,%al 804b69c: 76 1a jbe 804b6b8 <function_size+0x28> 804b69e: 89 c8 mov %ecx,%eax 804b6a0: 83 c0 01 add $0x1,%eax 804b6a3: 0f b6 10 movzbl (%eax),%edx 804b6a6: 83 c2 3e add $0x3e,%edx 804b6a9: 80 fa 01 cmp $0x1,%dl 804b6ac: 77 f2 ja 804b6a0 <function_size+0x10> 804b6ae: 29 c8 sub %ecx,%eax 804b6b0: 83 c0 10 add $0x10,%eax 804b6b3: c3 ret 804b6b4: 8d 74 26 00 lea 0x0(%esi,%eiz,1),%esi 804b6b8: b8 10 00 00 00 mov $0x10,%eax 804b6bd: c3 ret 804b6be: 66 90 xchg %ax,%ax If you look closely enough, you'll notice a C2 byte in add $0x3e,%edx instruction on address 804b6a6. The function will assume this byte is a RET instruction, return a size that's 22 bytes too short and then the code execution inside the executable buffer will run past the end of buffer, resulting in a segfault. Signed-off-by: Martin Doucha <mdoucha@suse.cz> --- testcases/kernel/syscalls/pkeys/pkey01.c | 52 ++---------------------- 1 file changed, 3 insertions(+), 49 deletions(-)