* [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE"
@ 2024-11-15 15:44 Martin Doucha
2024-11-15 16:06 ` Martin Doucha
0 siblings, 1 reply; 12+ messages in thread
From: Martin Doucha @ 2024-11-15 15:44 UTC (permalink / raw)
To: ltp
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)
--
2.46.0
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE"
2024-11-15 15:44 [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE" Martin Doucha
@ 2024-11-15 16:06 ` Martin Doucha
2024-11-15 18:58 ` Petr Vorel
0 siblings, 1 reply; 12+ messages in thread
From: Martin Doucha @ 2024-11-15 16:06 UTC (permalink / raw)
To: ltp
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)
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE"
2024-11-15 16:06 ` Martin Doucha
@ 2024-11-15 18:58 ` Petr Vorel
2024-11-19 13:11 ` Jan Stancek
0 siblings, 1 reply; 12+ messages in thread
From: Petr Vorel @ 2024-11-15 18:58 UTC (permalink / raw)
To: Martin Doucha; +Cc: ltp
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
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE"
2024-11-15 18:58 ` Petr Vorel
@ 2024-11-19 13:11 ` Jan Stancek
2024-11-19 13:31 ` Jan Stancek
0 siblings, 1 reply; 12+ messages in thread
From: Jan Stancek @ 2024-11-19 13:11 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp
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
>
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE"
2024-11-19 13:11 ` Jan Stancek
@ 2024-11-19 13:31 ` Jan Stancek
2024-11-19 15:47 ` [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size Jan Stancek
2024-11-20 12:57 ` [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE" Li Wang
0 siblings, 2 replies; 12+ messages in thread
From: Jan Stancek @ 2024-11-19 13:31 UTC (permalink / raw)
To: Petr Vorel; +Cc: 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.
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
* [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size
2024-11-19 13:31 ` Jan Stancek
@ 2024-11-19 15:47 ` Jan Stancek
2024-11-20 14:56 ` Jan Stancek
` (2 more replies)
2024-11-20 12:57 ` [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE" Li Wang
1 sibling, 3 replies; 12+ messages in thread
From: Jan Stancek @ 2024-11-19 15:47 UTC (permalink / raw)
To: ltp
As Martin found:
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.
Use a dummy function and copy entire page, similar to what we do
in mprotect04.
Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
testcases/kernel/syscalls/pkeys/Makefile | 2 ++
testcases/kernel/syscalls/pkeys/pkey01.c | 31 ++++++++++--------------
2 files changed, 15 insertions(+), 18 deletions(-)
Notes:
This could be an alternative to reverting PKEY_DISABLE_EXECUTE test.
I haven't tested it yet on 32bit, but since it doesn't rely on any
instruction codes I don't expect it to break. An important test
would also be ppc64le.
diff --git a/testcases/kernel/syscalls/pkeys/Makefile b/testcases/kernel/syscalls/pkeys/Makefile
index 9ee2c2ea57b0..814593f3c720 100644
--- a/testcases/kernel/syscalls/pkeys/Makefile
+++ b/testcases/kernel/syscalls/pkeys/Makefile
@@ -5,4 +5,6 @@ top_srcdir ?= ../../../..
include $(top_srcdir)/include/mk/testcases.mk
+pkey01: CFLAGS += -falign-functions=64
+
include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/pkeys/pkey01.c b/testcases/kernel/syscalls/pkeys/pkey01.c
index c041cbcfd969..b10760b5bd2f 100644
--- a/testcases/kernel/syscalls/pkeys/pkey01.c
+++ b/testcases/kernel/syscalls/pkeys/pkey01.c
@@ -144,15 +144,9 @@ static char *flag_to_str(int flags)
}
}
-static size_t function_size(void (*func)(void))
+static long __attribute__ ((noinline)) dummy_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 0xdead;
}
/*
@@ -165,8 +159,11 @@ 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;
+ long (*func)(void) = 0;
+ uintptr_t page_mask = ~(getpagesize() - 1);
+ uintptr_t offset_mask = (getpagesize() - 1);
+ uintptr_t func_page_offset = (uintptr_t)&dummy_func & offset_mask;
+ void *page_to_copy = (void *)((uintptr_t)&dummy_func & page_mask);
if (!execute_supported && (tc->access_rights == PKEY_DISABLE_EXECUTE)) {
tst_res(TCONF, "skip PKEY_DISABLE_EXECUTE test");
@@ -184,8 +181,8 @@ 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);
+ memcpy(buffer, page_to_copy, getpagesize());
+ func = (long (*)(void))(buffer + func_page_offset);
}
pkey = pkey_alloc(tc->flags, tc->access_rights);
@@ -211,8 +208,7 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa)
"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));
+ tst_res(TFAIL | TERRNO, "Execute buffer result = %ld", func());
break;
}
exit(0);
@@ -242,11 +238,10 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa)
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);
+ if (dummy_func() == func())
+ tst_res(TPASS, "Execute buffer success, result = %ld", dummy_func());
else
- tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func(func));
+ tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func());
break;
}
--
2.43.0
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE"
2024-11-19 13:31 ` Jan Stancek
2024-11-19 15:47 ` [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size Jan Stancek
@ 2024-11-20 12:57 ` Li Wang
1 sibling, 0 replies; 12+ messages in thread
From: Li Wang @ 2024-11-20 12:57 UTC (permalink / raw)
To: Jan Stancek; +Cc: ltp
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.
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size
2024-11-19 15:47 ` [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size Jan Stancek
@ 2024-11-20 14:56 ` Jan Stancek
2024-11-20 17:07 ` Petr Vorel
2024-11-25 3:43 ` Li Wang
2024-11-25 16:20 ` Martin Doucha
2 siblings, 1 reply; 12+ messages in thread
From: Jan Stancek @ 2024-11-20 14:56 UTC (permalink / raw)
To: ltp, Li Wang, Petr Vorel, Martin Doucha
On Tue, Nov 19, 2024 at 4:48 PM Jan Stancek <jstancek@redhat.com> wrote:
>
> As Martin found:
> 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.
>
> Use a dummy function and copy entire page, similar to what we do
> in mprotect04.
>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
> testcases/kernel/syscalls/pkeys/Makefile | 2 ++
> testcases/kernel/syscalls/pkeys/pkey01.c | 31 ++++++++++--------------
> 2 files changed, 15 insertions(+), 18 deletions(-)
>
> Notes:
>
> This could be an alternative to reverting PKEY_DISABLE_EXECUTE test.
> I haven't tested it yet on 32bit, but since it doesn't rely on any
> instruction codes I don't expect it to break. An important test
> would also be ppc64le.
I tested 32bit and ppc64le, both worked fine, so I propose we give this
a consideration as alternative to Martin's revert.
>
> diff --git a/testcases/kernel/syscalls/pkeys/Makefile b/testcases/kernel/syscalls/pkeys/Makefile
> index 9ee2c2ea57b0..814593f3c720 100644
> --- a/testcases/kernel/syscalls/pkeys/Makefile
> +++ b/testcases/kernel/syscalls/pkeys/Makefile
> @@ -5,4 +5,6 @@ top_srcdir ?= ../../../..
>
> include $(top_srcdir)/include/mk/testcases.mk
>
> +pkey01: CFLAGS += -falign-functions=64
> +
> include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/pkeys/pkey01.c b/testcases/kernel/syscalls/pkeys/pkey01.c
> index c041cbcfd969..b10760b5bd2f 100644
> --- a/testcases/kernel/syscalls/pkeys/pkey01.c
> +++ b/testcases/kernel/syscalls/pkeys/pkey01.c
> @@ -144,15 +144,9 @@ static char *flag_to_str(int flags)
> }
> }
>
> -static size_t function_size(void (*func)(void))
> +static long __attribute__ ((noinline)) dummy_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 0xdead;
> }
>
> /*
> @@ -165,8 +159,11 @@ 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;
> + long (*func)(void) = 0;
> + uintptr_t page_mask = ~(getpagesize() - 1);
> + uintptr_t offset_mask = (getpagesize() - 1);
> + uintptr_t func_page_offset = (uintptr_t)&dummy_func & offset_mask;
> + void *page_to_copy = (void *)((uintptr_t)&dummy_func & page_mask);
>
> if (!execute_supported && (tc->access_rights == PKEY_DISABLE_EXECUTE)) {
> tst_res(TCONF, "skip PKEY_DISABLE_EXECUTE test");
> @@ -184,8 +181,8 @@ 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);
> + memcpy(buffer, page_to_copy, getpagesize());
> + func = (long (*)(void))(buffer + func_page_offset);
> }
>
> pkey = pkey_alloc(tc->flags, tc->access_rights);
> @@ -211,8 +208,7 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa)
> "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));
> + tst_res(TFAIL | TERRNO, "Execute buffer result = %ld", func());
> break;
> }
> exit(0);
> @@ -242,11 +238,10 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa)
> 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);
> + if (dummy_func() == func())
> + tst_res(TPASS, "Execute buffer success, result = %ld", dummy_func());
> else
> - tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func(func));
> + tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func());
> break;
> }
>
> --
> 2.43.0
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size
2024-11-20 14:56 ` Jan Stancek
@ 2024-11-20 17:07 ` Petr Vorel
0 siblings, 0 replies; 12+ messages in thread
From: Petr Vorel @ 2024-11-20 17:07 UTC (permalink / raw)
To: Jan Stancek; +Cc: ltp
Hi Jan,
...
> > This could be an alternative to reverting PKEY_DISABLE_EXECUTE test.
> > I haven't tested it yet on 32bit, but since it doesn't rely on any
> > instruction codes I don't expect it to break. An important test
> > would also be ppc64le.
Reviewed-by: Petr Vorel <pvorel@suse.cz>
LGTM.
NOTE: we have hackweek this week, I guess you get Martin's feedback next week.
> I tested 32bit and ppc64le, both worked fine, so I propose we give this
> a consideration as alternative to Martin's revert.
Thanks!
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size
2024-11-19 15:47 ` [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size Jan Stancek
2024-11-20 14:56 ` Jan Stancek
@ 2024-11-25 3:43 ` Li Wang
2024-11-25 16:20 ` Martin Doucha
2 siblings, 0 replies; 12+ messages in thread
From: Li Wang @ 2024-11-25 3:43 UTC (permalink / raw)
To: Jan Stancek; +Cc: ltp
On Tue, Nov 19, 2024 at 11:48 PM Jan Stancek <jstancek@redhat.com> wrote:
> As Martin found:
> 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.
>
> Use a dummy function and copy entire page, similar to what we do
> in mprotect04.
>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
>
This way is better than reverting, thanks!
Reviewed-by: Li Wang <liwang@redhat.com>
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size
2024-11-19 15:47 ` [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size Jan Stancek
2024-11-20 14:56 ` Jan Stancek
2024-11-25 3:43 ` Li Wang
@ 2024-11-25 16:20 ` Martin Doucha
2024-11-26 8:05 ` Jan Stancek
2 siblings, 1 reply; 12+ messages in thread
From: Martin Doucha @ 2024-11-25 16:20 UTC (permalink / raw)
To: Jan Stancek, ltp
Hi,
I've successfully tested your patch on both x86 (both 32 and 64bit) and
PPC64. The extra alignment should ensure that the dummy function doesn't
get split between two pages so this is a good fix. There's just one
minor printf formatting issue at the end that can be fixed during merge.
Reviewed-by: Martin Doucha <mdoucha@suse.cz>
On 19. 11. 24 16:47, Jan Stancek wrote:
> As Martin found:
> 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.
>
> Use a dummy function and copy entire page, similar to what we do
> in mprotect04.
>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
> testcases/kernel/syscalls/pkeys/Makefile | 2 ++
> testcases/kernel/syscalls/pkeys/pkey01.c | 31 ++++++++++--------------
> 2 files changed, 15 insertions(+), 18 deletions(-)
>
> Notes:
>
> This could be an alternative to reverting PKEY_DISABLE_EXECUTE test.
> I haven't tested it yet on 32bit, but since it doesn't rely on any
> instruction codes I don't expect it to break. An important test
> would also be ppc64le.
>
> diff --git a/testcases/kernel/syscalls/pkeys/Makefile b/testcases/kernel/syscalls/pkeys/Makefile
> index 9ee2c2ea57b0..814593f3c720 100644
> --- a/testcases/kernel/syscalls/pkeys/Makefile
> +++ b/testcases/kernel/syscalls/pkeys/Makefile
> @@ -5,4 +5,6 @@ top_srcdir ?= ../../../..
>
> include $(top_srcdir)/include/mk/testcases.mk
>
> +pkey01: CFLAGS += -falign-functions=64
> +
> include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/pkeys/pkey01.c b/testcases/kernel/syscalls/pkeys/pkey01.c
> index c041cbcfd969..b10760b5bd2f 100644
> --- a/testcases/kernel/syscalls/pkeys/pkey01.c
> +++ b/testcases/kernel/syscalls/pkeys/pkey01.c
> @@ -144,15 +144,9 @@ static char *flag_to_str(int flags)
> }
> }
>
> -static size_t function_size(void (*func)(void))
> +static long __attribute__ ((noinline)) dummy_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 0xdead;
> }
>
> /*
> @@ -165,8 +159,11 @@ 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;
> + long (*func)(void) = 0;
> + uintptr_t page_mask = ~(getpagesize() - 1);
> + uintptr_t offset_mask = (getpagesize() - 1);
> + uintptr_t func_page_offset = (uintptr_t)&dummy_func & offset_mask;
> + void *page_to_copy = (void *)((uintptr_t)&dummy_func & page_mask);
>
> if (!execute_supported && (tc->access_rights == PKEY_DISABLE_EXECUTE)) {
> tst_res(TCONF, "skip PKEY_DISABLE_EXECUTE test");
> @@ -184,8 +181,8 @@ 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);
> + memcpy(buffer, page_to_copy, getpagesize());
> + func = (long (*)(void))(buffer + func_page_offset);
> }
>
> pkey = pkey_alloc(tc->flags, tc->access_rights);
> @@ -211,8 +208,7 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa)
> "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));
> + tst_res(TFAIL | TERRNO, "Execute buffer result = %ld", func());
> break;
> }
> exit(0);
> @@ -242,11 +238,10 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa)
> 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);
> + if (dummy_func() == func())
> + tst_res(TPASS, "Execute buffer success, result = %ld", dummy_func());
> else
> - tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func(func));
> + tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func());
The format here should be %ld.
> break;
> }
>
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size
2024-11-25 16:20 ` Martin Doucha
@ 2024-11-26 8:05 ` Jan Stancek
0 siblings, 0 replies; 12+ messages in thread
From: Jan Stancek @ 2024-11-26 8:05 UTC (permalink / raw)
To: Martin Doucha, Petr Vorel, Li Wang; +Cc: ltp
On Mon, Nov 25, 2024 at 5:21 PM Martin Doucha <mdoucha@suse.cz> wrote:
>
> Hi,
> I've successfully tested your patch on both x86 (both 32 and 64bit) and
> PPC64. The extra alignment should ensure that the dummy function doesn't
> get split between two pages so this is a good fix. There's just one
> minor printf formatting issue at the end that can be fixed during merge.
>
> Reviewed-by: Martin Doucha <mdoucha@suse.cz>
>
> On 19. 11. 24 16:47, Jan Stancek wrote:
> > As Martin found:
> > 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.
> >
> > Use a dummy function and copy entire page, similar to what we do
> > in mprotect04.
> >
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > ---
> > testcases/kernel/syscalls/pkeys/Makefile | 2 ++
> > testcases/kernel/syscalls/pkeys/pkey01.c | 31 ++++++++++--------------
> > 2 files changed, 15 insertions(+), 18 deletions(-)
> >
> > Notes:
> >
> > This could be an alternative to reverting PKEY_DISABLE_EXECUTE test.
> > I haven't tested it yet on 32bit, but since it doesn't rely on any
> > instruction codes I don't expect it to break. An important test
> > would also be ppc64le.
> >
> > diff --git a/testcases/kernel/syscalls/pkeys/Makefile b/testcases/kernel/syscalls/pkeys/Makefile
> > index 9ee2c2ea57b0..814593f3c720 100644
> > --- a/testcases/kernel/syscalls/pkeys/Makefile
> > +++ b/testcases/kernel/syscalls/pkeys/Makefile
> > @@ -5,4 +5,6 @@ top_srcdir ?= ../../../..
> >
> > include $(top_srcdir)/include/mk/testcases.mk
> >
> > +pkey01: CFLAGS += -falign-functions=64
> > +
> > include $(top_srcdir)/include/mk/generic_leaf_target.mk
> > diff --git a/testcases/kernel/syscalls/pkeys/pkey01.c b/testcases/kernel/syscalls/pkeys/pkey01.c
> > index c041cbcfd969..b10760b5bd2f 100644
> > --- a/testcases/kernel/syscalls/pkeys/pkey01.c
> > +++ b/testcases/kernel/syscalls/pkeys/pkey01.c
> > @@ -144,15 +144,9 @@ static char *flag_to_str(int flags)
> > }
> > }
> >
> > -static size_t function_size(void (*func)(void))
> > +static long __attribute__ ((noinline)) dummy_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 0xdead;
> > }
> >
> > /*
> > @@ -165,8 +159,11 @@ 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;
> > + long (*func)(void) = 0;
> > + uintptr_t page_mask = ~(getpagesize() - 1);
> > + uintptr_t offset_mask = (getpagesize() - 1);
> > + uintptr_t func_page_offset = (uintptr_t)&dummy_func & offset_mask;
> > + void *page_to_copy = (void *)((uintptr_t)&dummy_func & page_mask);
> >
> > if (!execute_supported && (tc->access_rights == PKEY_DISABLE_EXECUTE)) {
> > tst_res(TCONF, "skip PKEY_DISABLE_EXECUTE test");
> > @@ -184,8 +181,8 @@ 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);
> > + memcpy(buffer, page_to_copy, getpagesize());
> > + func = (long (*)(void))(buffer + func_page_offset);
> > }
> >
> > pkey = pkey_alloc(tc->flags, tc->access_rights);
> > @@ -211,8 +208,7 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa)
> > "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));
> > + tst_res(TFAIL | TERRNO, "Execute buffer result = %ld", func());
> > break;
> > }
> > exit(0);
> > @@ -242,11 +238,10 @@ static int pkey_test(struct tcase *tc, struct mmap_param *mpa)
> > 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);
> > + if (dummy_func() == func())
> > + tst_res(TPASS, "Execute buffer success, result = %ld", dummy_func());
> > else
> > - tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func(func));
> > + tst_res(TFAIL, "Execute buffer with unexpected result: %zi", func());
>
> The format here should be %ld.
Pushed with formatting fixed.
Thanks,
Jan
>
> > break;
> > }
> >
>
> --
> Martin Doucha mdoucha@suse.cz
> SW Quality Engineer
> SUSE LINUX, s.r.o.
> CORSO IIa
> Krizikova 148/34
> 186 00 Prague 8
> Czech Republic
>
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-11-26 8:06 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-15 15:44 [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE" Martin Doucha
2024-11-15 16:06 ` Martin Doucha
2024-11-15 18:58 ` Petr Vorel
2024-11-19 13:11 ` Jan Stancek
2024-11-19 13:31 ` Jan Stancek
2024-11-19 15:47 ` [LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size Jan Stancek
2024-11-20 14:56 ` Jan Stancek
2024-11-20 17:07 ` Petr Vorel
2024-11-25 3:43 ` Li Wang
2024-11-25 16:20 ` Martin Doucha
2024-11-26 8:05 ` Jan Stancek
2024-11-20 12:57 ` [LTP] [PATCH] Revert "pkey01: Adding test for PKEY_DISABLE_EXECUTE" Li Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.