[LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size
Martin Doucha
mdoucha@suse.cz
Mon Nov 25 17:20:49 CET 2024
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
More information about the ltp
mailing list