Hi
On 04/09/2019 12:48, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:27pm +0100, Cristian Marussi wrote:
>> Add a simple fake_sigreturn testcase which builds a ucontext_t with a bad
>> magic header and place it onto the stack. Expects a SIGSEGV on test PASS.
>>
>> Introduce a common utility assembly trampoline function to invoke a
>> sigreturn while placing the provided sigframe at wanted alignment and
>> also an helper to make space when needed inside the sigframe reserved
>> area.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
>> ---
>> v3 --> v4
>> - fix commit
>> - fix signal.S, handle misalign requests too
>> - remove unneeded comments
>> - add signal.h include
>> - added get_starting_head() helper
>> - added test description
>> ---
>> tools/testing/selftests/arm64/signal/Makefile | 2 +-
>> .../testing/selftests/arm64/signal/signals.S | 62 +++++++++++++++++++
>> .../arm64/signal/test_signals_utils.h | 1 +
>> .../testcases/fake_sigreturn_bad_magic.c | 54 ++++++++++++++++
>> .../arm64/signal/testcases/testcases.c | 28 +++++++++
>> .../arm64/signal/testcases/testcases.h | 4 ++
>> 6 files changed, 150 insertions(+), 1 deletion(-)
>> create mode 100644 tools/testing/selftests/arm64/signal/signals.S
>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/Makefile b/tools/testing/selftests/arm64/signal/Makefile
>> index f78f5190e3d4..b497cfea4643 100644
>> --- a/tools/testing/selftests/arm64/signal/Makefile
>> +++ b/tools/testing/selftests/arm64/signal/Makefile
>> @@ -28,5 +28,5 @@ clean:
>> # Common test-unit targets to build common-layout test-cases executables
>> # Needs secondary expansion to properly include the testcase c-file in pre-reqs
>> .SECONDEXPANSION:
>> -$(PROGS): test_signals.c test_signals_utils.c testcases/testcases.c $$@.c test_signals.h test_signals_utils.h testcases/testcases.h
>> +$(PROGS): test_signals.c test_signals_utils.c testcases/testcases.c signals.S $$@.c test_signals.h test_signals_utils.h testcases/testcases.h
>> $(CC) $(CFLAGS) $^ -o $@
>> diff --git a/tools/testing/selftests/arm64/signal/signals.S b/tools/testing/selftests/arm64/signal/signals.S
>> new file mode 100644
>> index 000000000000..b89fec0d5ba0
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/signals.S
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2019 ARM Limited */
>> +
>> +#include <asm/unistd.h>
>> +
>> +.section .rodata, "a"
>> +call_fmt:
>> + .asciz "Calling sigreturn with fake sigframe sized:%zd at SP @%08lX\n"
>> +
>> +.text
>> +
>> +.globl fake_sigreturn
>> +
>> +/* fake_sigreturn x0:&sigframe, x1:sigframe_size, x2:misalign_bytes */
>> +fake_sigreturn:
>
> Nit: the "bl printf" later on destroys lr.
>
> This isn't a problem, since the function never tries to return anyway --
> if things go wrong you just "b .".
>
> But it may be helpful for debug purposes to at least create a frame
> record, e.g.:
>
> stp x29, x30, [sp, #-16]!
> mov x29, sp
>
> before doing anything else.
>
ok
>> + mov x20, x0
>> + mov x21, x1
>> + mov x22, x2
>> + mov x23, sp
>
> Nit: to follow the conventional asm style for arm64 kernel code, can you
> format lines as
>
> <TAB> op<TAB> operands
>
ok
>> +
>> + /* create space on the stack for fake sigframe 16 bytes-aligned */
>> + add x0, x21, #16
>> + bic x0, x0, #15
>> + sub x23, x23, x0
>> + /* any misalignment requested ? */
>> + add x23, x23, x22
>
> Aren't we actually reducing the allocation here, rather than increasing it?
>
For the misalignment bytes if any yes...
> Doing something like this may work to allocate guaranteed sufficient
> space:
>
> add x0, x21, x22
> add x0, x0, #15
> bic x0, x0, #15 /* round_up(sigframe_size + misglian_bytes, 16) */
> sub sp, sp, x0
> add x23, sp, x22 /* new sigframe base with misaligment */
>
> (You can drop the mov into x23 above in your function prologue if you
> code it this way.)
>
ok...but...shouldn't be
add x0, x0, #16
before
bic x-, x0, #15
instead of adding #15 ?
>> +
>> + ldr x0, =call_fmt
>> + mov x1, x21
>> + mov x2, x23
>> + bl printf
>> +
>> + mov sp, x23
>
> AAPCS64 requires sp to be 16-byte aligned at function boundaries, so
> we may get stack alignments faults in mempcy() here. Possibly these
> can be confused with test failure SEGVs (I can't remember offhand how
> stack alignment faults are supported).
>
Didn't know about function boundaries requirements.
> Coding something like what I have above to guarantee stack alignment
> should avoid this.
>
Nice I'll do.
>> + /* now fill it with the provided content... */
>> + mov x0, sp
>
> With my version this would be mov x0, x23
>
ok
>> + mov x1, x20
>> + mov x2, x21
>> + bl memcpy
>> +
>> + /*
>> + * Here saving a last minute SP to current->token acts as a marker:
>> + * if we got here, we are successfully faking a sigreturn; in other
>> + * words we are sure no bad fatal signal has been raised till now
>> + * for unrelated reasons, so we should consider the possibly observed
>> + * fatal signal like SEGV coming from Kernel restore_sigframe() and
>> + * triggered as expected from our test-case.
>> + * For simplicity this assumes that current field 'token' is laid out
>> + * as first in struct tdescr
>> + */
>> + ldr x0, current
>
> Nit: it probably doesn't matter since this will be a small binary
> after linking, but to avoid possible fixup errors during linking you
> could also do:
>
> adrp x0, current
> ldr x0, [x0, #:lo12:current]
>
> This raises the addressing range from 0.5 MB or so to a few GB, making
> link errors much more unlikely.
>
Ok...I'll fix this nitpick once I'll have understood it :D
>> + str x23, [x0]
>> + /* SP is already pointing back to the just built fake sigframe here */
>> + mov x8, #__NR_rt_sigreturn
>
> And finally we would mov sp, x23 here.
>
Yes
>> + svc #0
>> +
>> + /*
>> + * Above sigreturn should not return...looping here leads to a timeout
>> + * and ensure proper and clean test failure, instead of jumping around
>> + * on a potentially corrupted stack.
>> + */
>> + b .
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> index ce35be8ebc8e..68930f1e46e5 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> @@ -12,4 +12,5 @@ int test_run(struct tdescr *td);
>> void test_result(struct tdescr *td);
>>
>> bool get_current_context(struct tdescr *td, ucontext_t *dest_uc);
>> +int fake_sigreturn(void *sigframe, size_t sz, int misalign_bytes);
>> #endif
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
>> new file mode 100644
>> index 000000000000..7fb700b9801b
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
>> @@ -0,0 +1,54 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 ARM Limited
>> + *
>> + * Place a fake sigframe on the stack including a BAD Unknown magic
>> + * record: on sigreturn Kernel must spot this attempt and the test
>> + * case is expected to be terminated via SEGV.
>> + */
>> +
>> +#include <signal.h>
>> +#include <ucontext.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +struct fake_sigframe sf;
>> +
>> +static int fake_sigreturn_bad_magic_run(struct tdescr *td,
>> + siginfo_t *si, ucontext_t *uc)
>> +{
>> + size_t resv_sz, need_sz;
>> + struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
>> +
>> + /* just to fill the ucontext_t with something real */
>> + if (!get_current_context(td, &sf.uc))
>> + return 1;
>> +
>> + resv_sz = GET_SF_RESV_SIZE(sf);
>> + /* need at least 2*HDR_SZ space: KSFT_BAD_MAGIC + terminator. */
>> + need_sz = HDR_SZ * 2;
>> + head = get_starting_head(shead, need_sz, resv_sz, NULL);
>
> Nit: are the need_sz and resv_sz variables required?
>
> Maybe they help to highlight what these expressions mean in the
> get_starting_head() call though. I'm happy either way.
Not really required...I'll remove. (probably in other tests were and they
landed here too...)
>
>> + if (head) {
>> + /*
>> + * use a well known NON existent bad magic...something
>> + * we should pretty sure won't be ever defined in Kernel
>> + */
>> + head->magic = KSFT_BAD_MAGIC;
>> + head->size = HDR_SZ;
>> + write_terminator_record(GET_RESV_NEXT_HEAD(head));
>> +
>> + ASSERT_BAD_CONTEXT(&sf.uc);
>> + fake_sigreturn(&sf, sizeof(sf), 0);
>> + }
>> +
>> + return 1;
>> +}
>> +
>> +struct tdescr tde = {
>> + .name = "FAKE_SIGRETURN_BAD_MAGIC",
>> + .descr = "Trigger a sigreturn with a sigframe with a bad magic",
>> + .sig_ok = SIGSEGV,
>> + .timeout = 3,
>> + .run = fake_sigreturn_bad_magic_run,
>> +};
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c
>> index 72e3f482b177..2effb8ded935 100644
>> --- a/tools/testing/selftests/arm64/signal/testcases/testcases.c
>> +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c
>> @@ -149,3 +149,31 @@ bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err)
>>
>> return true;
>> }
>> +
>
> Maybe add a comment saying what this function does.
>
> To check my understanding:
> The purpose is to find a place to append a new record, right?
> By default we append at the end (i.e., at the terminator), but
> because extra_context is optional we replace that instead if
> there isn't sufficient space after the terminator in __reserved[].
>
Yes I'll add a comment.
>> +struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,
>> + size_t need_sz, size_t resv_sz,
>> + size_t *offset)
>> +{
>> + size_t offs = 0;
>> + struct _aarch64_ctx *head;
>> +
>> + head = get_terminator(shead, resv_sz, &offs);
>> + /* not found a terminator...no need to update offset if any */
>> + if (!head)
>> + return head;
>> + if (resv_sz - offs < need_sz) {
>> + fprintf(stderr, "Low on space:%zd. Discarding extra_context.\n",
>> + resv_sz - offs);
>> + head = get_header(shead, EXTRA_MAGIC, resv_sz, &offs);
>> + if (!head || resv_sz - offs < need_sz) {
>> + fprintf(stderr,
>> + "Failed to reclaim space on sigframe.\n");
>> + return NULL;
>> + }
>> + }
>> +
>> + fprintf(stderr, "Available space:%zd\n", resv_sz - offs);
>> + if (offset)
>> + *offset = offs;
>> + return head;
>> +}
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.h b/tools/testing/selftests/arm64/signal/testcases/testcases.h
>> index 00618c3202bb..7653f8a64b3d 100644
>> --- a/tools/testing/selftests/arm64/signal/testcases/testcases.h
>> +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.h
>> @@ -83,4 +83,8 @@ static inline void write_terminator_record(struct _aarch64_ctx *tail)
>> tail->size = 0;
>> }
>> }
>> +
>> +struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,
>> + size_t need_sz, size_t resv_sz,
>> + size_t *offset);
>> #endif
>
> Apart from the comments above, this looks reasonable.
>
> Cheers
> ---Dave
>
Cheers
Cristian
