Hi
On 13/08/2019 17:25, Dave Martin wrote:
> On Fri, Aug 02, 2019 at 06:02:54PM +0100, Cristian Marussi wrote:
>> Added a simple mangle testcase which messes with the ucontext_t
>
> Add
>
>> from within the sig_handler, trying to toggle PSTATE SSBS bit.
>
> signal handler
>
Ok.
>> Expect SIGILL if SSBS feature unsupported or that the value set in
>> PSTATE.SSBS is preserved on test PASS.
>
> The test doesn't set PSTATE.SSBS directly.
>
> Maybe something like: "Expect SIGILL if the SSBS feature is unsupported.
> Otherwise, expect sigreturn to set PSTATE.SSBS from the corresponding
> bit in pstate in the signal frame."
>
Ok
>> This commit also introduces a new common utility function:
>> get_current_context() which can be used to grab a ucontext without
>> the help of libc, and detect if such ucontext has been actively used
>> to jump back into it.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
>> ---
>> .../selftests/arm64/signal/test_signals.h | 4 +
>> .../arm64/signal/test_signals_utils.c | 93 +++++++++++++++++++
>> .../arm64/signal/test_signals_utils.h | 2 +
>> .../arm64/signal/testcases/.gitignore | 1 +
>> .../testcases/mangle_pstate_ssbs_regs.c | 56 +++++++++++
>> 5 files changed, 156 insertions(+)
>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals.h b/tools/testing/selftests/arm64/signal/test_signals.h
>> index 85db3ac44b32..37bed0590226 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals.h
>> +++ b/tools/testing/selftests/arm64/signal/test_signals.h
>> @@ -116,6 +116,10 @@ struct tdescr {
>> /* optional sa_flags for the installed handler */
>> int sa_flags;
>> ucontext_t saved_uc;
>> + /* used by get_current_ctx() */
>> + size_t live_sz;
>> + ucontext_t *live_uc;
>> + volatile bool live_uc_valid;
>>
>> /* a setup function to be called before test starts */
>> int (*setup)(struct tdescr *td);
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> index ac0055f6340b..faf55ba99d58 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> @@ -11,12 +11,16 @@
>> #include <linux/auxvec.h>
>> #include <ucontext.h>
>>
>> +#include <asm/unistd.h>
>> +
>> #include "test_signals.h"
>> #include "test_signals_utils.h"
>> #include "testcases/testcases.h"
>>
>> extern struct tdescr *current;
>>
>> +static int sig_copyctx = SIGUSR2;
>> +
>> static char *feats_store[FMAX_END] = {
>> "SSBS",
>> "PAN",
>> @@ -37,6 +41,85 @@ static inline char *feats_to_string(unsigned long feats)
>> return feats_string;
>> }
>>
>> +/*
>> + * Obtaining a valid and full-blown ucontext_t from userspace is tricky:
>> + * libc getcontext does() not save all the regs and messes with some of
>> + * them (pstate value in particular is not reliable).
>> + * Here we use a service signal to grab the ucontext_t from inside a
>> + * dedicated signal handler, since there, it is populated by Kernel
>> + * itself in setup_sigframe(). The grabbed context is then stored and
>> + * made available in td->live_uc.
>> + *
>> + * Anyway this function really serves a dual purpose:
>> + *
>> + * 1. grab a valid sigcontext into td->live_uc for result analysis: in
>> + * such case it returns 1.
>> + *
>> + * 2. detect if somehow a previously grabbed live_uc context has been
>> + * used actively with a sigreturn: in such a case the execution would have
>> + * magically resumed in the middle of the function itself (seen_already==1):
>> + * in such a case return 0, since in fact we have not just simply grabbed
>> + * the context.
>> + *
>> + * This latter case is useful to detect when a fake_sigreturn test-case has
>> + * unexpectedly survived without hittig a SEGV.
>> + */
>> +bool get_current_context(struct tdescr *td, ucontext_t *dest_uc)
>> +{
>> + static volatile sig_atomic_t seen_already;
>> +
>> + if (!td || !dest_uc) {
>> + fprintf(stdout, "Signal-based Context dumping NOT available\n");
>
> Should this ever happen inless there is a test bug?
>
> Maybe this should just be an assert.
Yes definitely better.
>
>> + return 0;
>> + }
>> +
>> + /* it's a genuine invokation..reinit */
>> + seen_already = 0;
>> + td->live_uc_valid = 0;
>> + td->live_sz = sizeof(*dest_uc);
>> + memset(dest_uc, 0x00, td->live_sz);
>
> Eventually we will need to examine the signal frame to determine its
> size, but for now this is fine.
>
> It will start to matter for SVE.
Yes this function has been reworked in the SVE signal frame patches to
dynamically detect runtime signal frame and be able to optionally grab
a sigframe containing SVE material (avoiding the kill() syscall to trigger
a signal in favour of a brk instruction to cause a SIGTRAP without passing
via the syscall machinery which would zero the SVE sigframe stuff)
Such patch, which depends on this series, is still not published.
>
>> + td->live_uc = dest_uc;
>> + /*
>> + * Grab ucontext_t triggering a signal...
>> + * ASM equivalent of raise(sig_copyctx);
>> + *
>> + * Note that:
>> + * - live_uc_valid is declared volatile in struct tdescr
>> + * since it will be changed inside the sig_copyctx handler.
>> + * - the kill() syscall invocation returns only after any possible
>> + * registered sig_handler for the invoked signal has returned,
>
> sig_handler looks like the name of some function of variable, but I
> can't find it. Did I miss something?
No I'll replace the comment with "signal handler"
>
>> + * so that live_uc_valid flag is surely up to date when this
>> + * function return it.
>> + * - the additional 'memory' clobber is there to avoid possible
>> + * compiler's assumption on the content pointed by dest_uc, which
>> + * is changed inside the handler, but not referenced here anyway.
>> + */
>> + asm volatile ("mov x8, %0\n\t"
>> + "svc #0\n\t"
>> + "mov x1, %1\n\t"
>> + "mov x8, %2\n\t"
>> + "svc #0"
>> + :
>> + : "i" (__NR_getpid),
>> + "r" (sig_copyctx),
>> + "i" (__NR_kill)
>> + : "x1","x8","x0","memory");
>> + /*
>> + * If we get here with seen_already==1 it implies the td->live_uc
>> + * context has been used to get back here....this probably means
>> + * a test has failed to cause a SEGV...anyway the live_uc has not
>> + * just been acquired...so return 0
>> + */
>> + if (seen_already) {
>> + fprintf(stdout,
>> + "Successful sigreturn detected: live_uc is stale !\n");
>> + return 0;
>> + }
>> + seen_already = 1;
>> +
>> + return td->live_uc_valid;
>> +}
>> +
>> static void unblock_signal(int signum)
>> {
>> sigset_t sset;
>> @@ -112,6 +195,12 @@ static void default_handler(int signum, siginfo_t *si, void *uc)
>> * to terminate immediately exiting straight away
>> */
>> default_result(current, 1);
>> + } else if (signum == sig_copyctx && current->live_uc) {
>> + memcpy(current->live_uc, uc, current->live_sz);
>> + ASSERT_GOOD_CONTEXT(current->live_uc);
>> + current->live_uc_valid = 1;
>> + fprintf(stderr,
>> + "GOOD CONTEXT grabbed from sig_copyctx handler\n");
>> } else {
>> if (signum == current->sig_unsupp && !are_feats_ok(current)) {
>> fprintf(stderr, "-- RX SIG_UNSUPP on unsupported feature...OK\n");
>> @@ -214,6 +303,10 @@ static int test_init(struct tdescr *td)
>> !feats_ok ? "NOT " : "");
>> }
>>
>> + if (td->sig_trig == sig_copyctx)
>> + sig_copyctx = SIGUSR1;
>
> What's this for? What if we have the same signal for sig_trig and
> sig_copyctx?
>
To avoid that a user defined sig_trig equal to sig_copyctx can fool this function.
In SVE signal frame patch (not in this series), I'll anyway switch to use a distinct
SIGTRAP sig_trig.
>> + unblock_signal(sig_copyctx);
>> +
>> td->initialized = 1;
>> return 1;
>> }
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> index 8658d1a7d4b9..ce35be8ebc8e 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> @@ -10,4 +10,6 @@ int test_setup(struct tdescr *td);
>> void test_cleanup(struct tdescr *td);
>> int test_run(struct tdescr *td);
>> void test_result(struct tdescr *td);
>> +
>> +bool get_current_context(struct tdescr *td, ucontext_t *dest_uc);
>> #endif
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/.gitignore b/tools/testing/selftests/arm64/signal/testcases/.gitignore
>> index 226bb179b673..a48a118b1a1a 100644
>> --- a/tools/testing/selftests/arm64/signal/testcases/.gitignore
>> +++ b/tools/testing/selftests/arm64/signal/testcases/.gitignore
>> @@ -3,3 +3,4 @@ mangle_pstate_invalid_daif_bits
>> mangle_pstate_invalid_mode_el1
>> mangle_pstate_invalid_mode_el2
>> mangle_pstate_invalid_mode_el3
>> +mangle_pstate_ssbs_regs
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>> new file mode 100644
>> index 000000000000..a399d9aa40d5
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>> @@ -0,0 +1,56 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2019 ARM Limited */
>> +
>> +#include <stdio.h>
>> +#include <ucontext.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +static int mangle_invalid_pstate_ssbs_run(struct tdescr *td,
>> + siginfo_t *si, ucontext_t *uc)
>> +{
>> + ASSERT_GOOD_CONTEXT(uc);
>> +
>> + /* set bit value */
>
> Should we clear SSBS in the test setup (using MSR), to make sure that
> sigreturn really succeeds in _changing_ the bit to 1?
>
Yes. I introduced a set_regval() asm helper and added a new .init signal-test
framework entry in tdescr to be able to call per-test specific initialization.
I took the chance also to remove the remaining volatile qualifiers from signal
handling code to please checkpatch, and add a dsb barrier to ensure the writes
by the signal handler.
>> + uc->uc_mcontext.pstate |= PSR_SSBS_BIT;
>> + fprintf(stderr, "SSBS set to 1 -- PSTATE: 0x%016lX\n",
>> + uc->uc_mcontext.pstate);
>> + /* Save after mangling...it should be preserved */
>> + td->saved_uc = *uc;
>> +
>> + return 1;
>> +}
>
> [...]
>
> Cheers
> ---Dave
>
Cheers
Cristian
