[LTP] [PATCH] syscalls/pkeys01: use a dummy function instead of function_size
Jan Stancek
jstancek@redhat.com
Tue Nov 26 09:05:44 CET 2024
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
>
More information about the ltp
mailing list