On Mon, Oct 07, 2019 at 07:23:19PM +0100, Cristian Marussi wrote:
> Hi
>
> finally back on this series.
>
> On 17/09/2019 17:05, Dave Martin wrote:
> > On Tue, Sep 10, 2019 at 01:31:05pm +0100, Cristian Marussi wrote:
> >> Add a simple mangle testcase which messes with the ucontext_t from within
> >> the signal handler, trying to set the PSTATE SSBS bit and verify that
> >> SSBS bit set is preserved across sigreturn.
> >> When available, use MRS SBSS support to set/get SSBS bit, otherwise lookup
> >> PSTATE.SSBS directly.
> >>
> >> Additionally, in order to support this test specific needs:
> >> - extend signal testing framework to allow the definition of a custom
> >> per test initialization function to be run at the end of test setup
> >> and before test run routine.
> >> - introduce a set_regval() helper to set system register values in a
> >> toolchain independent way.
> >> - introduce also 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.
> >>
> >> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> >> ---
> >> v5 --> v6
> >> - using SIGTRAP as sig_copyctx for get_current_context()
> >> - get_current_context() is now __always_inline
> >> - last minute check for SSBS cleared
> >> - restore volatile usage, dropping useless DSB
> >> - output clobber on *dest_uc
> >> - no abort() on SSSB not cleared
> >> - refactored/relocated test_init() call to be after test_setup()
> >> [to catch early SIGILL while initializing]
> >> - avoid MRS SSBS when !feats_ok()...use instead PSTATE.SSBS
> >> - refactored SIG_COPYCTX usage to fit new splitted-by-signal layout
[...]
> >> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
[...]
> >> +static __always_inline bool get_current_context(struct tdescr *td,
> >> + ucontext_t *dest_uc)
> >> +{
> >> + static volatile bool seen_already;
> >> +
> >> + assert(td && dest_uc);
> >> + /* it's a genuine invocation..reinit */
> >> + seen_already = 0;
> >
> > Nit: can we have "= 0" as an initializer in the declaration above?
> >
>
> Not sure if you mean to add a zero initialization to the static
> declaration or to fold this apparently redundant
>
> >> + seen_already = 0;
>
> into the above.
>
> If you mean the latter folding I think I cannot for the following reasons:
>
> the static seen_already is placed out of the stack and automatically
> initialized to 0 once for all at program initialization.
>
> >> + static volatile bool seen_already;
>
> After that, seen_already is set to 1 after the context has been
> grabbed using the brk/SIGTRAP trick in order to signify that context
> has been grabbed successfully. But is is set to 1 only after having
> been checked for ZERO in order to detect if we happened to arrive
> here in the middle of this function as the unexpected result of a
> fake_sigreturn using this context.
>
> >> + if (seen_already) {
> >> + fprintf(stdout,
> >> + "Unexpected successful sigreturn detected: live_uc is stale !\n");
> >> + return 0;
> >> + }
> >> + seen_already = 1;
>
> So get_current_context returns 1 on success when a good context has
> been grabbed, while returns 0 when in fact returning from a sigreturn
> using a previously grabbed context (in a highly corrupted stack
> frame...so you'll probably want to exit straight away in the caller
> at that point.)
>
> Now answering finally your (possible) question about the apparently
> redundant
>
> >> seen_already = 0;
>
> what happens if I call regularly two times in sequence this utility
> function in the same program ?
>
> say at first to grab a context to analyze the PSTATE reported there,
> and maybe again later to grab another context to put onto the stack,
> or to recheck the PSTATE ?
>
> I would expect NOT to fail and obtain both times a new valid context,
> but my understanding is that in order to be able to do so, I'll have
> to re-initialize to 0 the seen_already flag explicitly on each
> invocation.
My bad -- I'd confused myself and missed the significance of "static"
here. As you point out, seen_already needs to be static for other
reasons, and reinitialised each time we call this function.
So you were right to ignore this nitpick :)
> >> + td->live_uc_valid = 0;
> >> + td->live_sz = sizeof(*dest_uc);
> >> + memset(dest_uc, 0x00, td->live_sz);
> >> + td->live_uc = dest_uc;
> >> + /*
> >> + * Grab ucontext_t triggering a SIGTRAP.
> >> + *
> >> + * Note that:
> >> + * - live_uc_valid is declared volatile sig_atomic_t in
> >> + * struct tdescr since it will be changed inside the
> >> + * sig_copyctx handler
> >> + * - the additional 'memory' clobber is there to avoid possible
> >> + * compiler's assumption on live_uc_valid, seen-already and
> >
> > I could be wrong about this, but I'm not sure that the "memory" clobber
> > is sufficient for for seen_already, because of the way that variable is
> > scoped to this function; however, you declare seen_already volatile
> > anyway which should be sufficient.
> >
> > I suggest you just omit seen_already from this comment.
> >
>
> Ok
>
>
> >> + * the content pointed by dest_uc, which are all changed inside
> >> + * the signal handler
> >> + * - BRK causes a debug exception which is handled by the Kernel
> >> + * and finally causes the SIGTRAP signal to be delivered to this
> >> + * test thread. Since such delivery happens on the ret_to_user()
> >> + * /do_notify_resume() debug exception return-path, we are sure
> >> + * that the registered SIGTRAP handler has been run to completion
> >> + * before the execution path is restored here: as a consequence
> >> + * we can be sure that the volatile sig_atomic_t live_uc_valid
> >> + * carries a meaningful result. Being in a single thread context
> >> + * we'll also be sure that any access to memory modified by the
> >> + * handler (namely ucontext_t) will be visible once returned.
> >> + * - note that since we are using a breakpoint instruction here
> >> + * to cause a SIGTRAP, the ucontext_t grabbed from the signal
> >> + * handler would naturally contain a PC pointing exactly to this
> >> + * BRK line, which means that, on return from the signal handler,
> >> + * or if we place the ucontext_t on the stack to fake a sigreturn,
> >> + * we'll end up in an infinite loop of BRK-SIGTRAP-handler.
> >> + * For this reason we take care to artificially move forward the
> >> + * PC to the next instruction while inside the signal handler.
> >> + */
> >> + asm volatile ("brk #666"
> >> + : "=m" (*dest_uc)
> >
> > Make this an input-output argument ("+m")? The memset() needs to
> > take effect happen before the asm.
> >
>
> Ok
>
>
> >> + :
> >> + : "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 live_uc does not
> >> + * point to a just acquired copy of ucontext_t...so return 0
> >> + */
> >> + if (seen_already) {
> >> + fprintf(stdout,
> >> + "Unexpected successful sigreturn detected: live_uc is stale !\n");
> >> + return 0;
> >> + }
> >> + seen_already = 1;
> >> +
> >> + return td->live_uc_valid;
> >> +}
> >> #endif
> >> 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..e2b87ea4c11f
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
> >> @@ -0,0 +1,88 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) 2019 ARM Limited
> >> + *
> >> + * Try to mangle the ucontext from inside a signal handler, setting the
> >> + * SSBS bit to 1 and veryfing that such modification is preserved.
> >> + */
> >> +
> >> +#include <stdio.h>
> >> +#include <stdlib.h>
> >> +#include <signal.h>
> >> +#include <ucontext.h>
> >> +
> >> +#include "test_signals_utils.h"
> >> +#include "testcases.h"
> >> +
> >> +static void mangle_invalid_pstate_ssbs_init(struct tdescr *td)
> >> +{
> >> + if (feats_ok(td)) {
> >> + fprintf(stderr, "Clearing SSBS to 0\n");
> >> + set_regval(SSBS_SYSREG, 0);
> >> + }
> >> +}
> >> +
> >> +static int mangle_invalid_pstate_ssbs_run(struct tdescr *td,
> >> + siginfo_t *si, ucontext_t *uc)
> >> +{
> >> + ASSERT_GOOD_CONTEXT(uc);
> >> +
> >> + /*
> >> + * If HW_SSBS is supported but we weren't able to clear SSBS during
> >> + * test_init, or if something has reset SSBS in the meantime, abort.
> >
> > What is "HW_SSBS" ?
> >
>
> HWCAP_SSBS, I'll fix
>
>
> >> + */
> >> + if (feats_ok(td) && (uc->uc_mcontext.pstate & PSR_SSBS_BIT)) {
> >> + fprintf(stderr,
> >> + "SSBS unexpectedly NOT zeroed ! Something's wrong. Abort\n");
> >> + abort();
> >> + }
> >> +
> >> + /* set bit value ... should NOT be cleared by Kernel on sigreturn */
> >> + uc->uc_mcontext.pstate |= PSR_SSBS_BIT;
> >> + fprintf(stderr, "SSBS set to 1 -- PSTATE: 0x%016llX\n",
> >> + uc->uc_mcontext.pstate);
> >> + /* Save after mangling...it should be preserved */
> >> + td->saved_uc = *uc;
> >> +
> >> + return 1;
> >> +}
> >> +
> >> +static void pstate_ssbs_bit_checks(struct tdescr *td)
> >> +{
> >> + uint64_t val = 0;
> >> + ucontext_t uc;
> >> +
> >> + /* This check reports some result even if MRS SSBS unsupported */
> >> + if (get_current_context(td, &uc))
> >> + fprintf(stderr,
> >> + "INFO: live_uc - got PSTATE: 0x%016llX -> SSBS %s\n",
> >> + uc.uc_mcontext.pstate,
> >> + (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT) ==
> >> + (uc.uc_mcontext.pstate & PSR_SSBS_BIT) ?
> >> + "PRESERVED" : "CLEARED");
> >
> > Don't we subsequently refer to uc even if get_current_context() failed
> > here?
> >
>
> Yes, but in fact in this invocation it cannot fail, it can fail only
> once a sigreturn with a previously grabbed context uses it and unexpectedly
> returns.
> Even though the if is redundant in fact here, it is misleading as used now,
> I'll fix checking the retvalue and bailing out with abort() in case
> get_curent_context unexpectedly failed in this conetxt.
OK, that sounds like a cleaner approach, even though I guess you're
correct that the failure is impossible here.
>
>
> >> + /* Choose check method depending of supported features */
> >> + if (feats_ok(td)) {
> >> + fprintf(stderr, "Checking with MRS SSBS...\n");
> >> + get_regval(SSBS_SYSREG, val);
> >> + fprintf(stderr, "INFO: MRS SSBS - got: 0x%016lX\n", val);
> >> + } else {
> >> + fprintf(stderr, "Checking with PSTATE.SSBS...\n");
> >> + val = uc.uc_mcontext.pstate;
> >> + }
> >> + /* pass when preserved */
> >> + td->pass = (val & PSR_SSBS_BIT) ==
> >> + (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT);
> >
> > Does this mean the test fails when SSBS isn't supported at all? That
> > doesn't seem right.
> >
> > I think trying to handle both levels of SSBS support within the same
> > test is complicating things here. The purpose of this is to check
> > that sigreturn doesn't mask out the SSBS bit when it shouldn't: we don't
> > care whether the SSBS architecture feature actually works.
> >
> > So, would it be simpler to drop the MSR/MRS direct access to the SSBS
> > bit?
> >
> > Instead, we could predicate this test on whether ID_AA64PFR1_EL1.SSBS
> >> = 1 instead of HWCAP_SSBS, and we could just check that run() can
> > successfully _toggle_ uc->uc_mcontext.pstate ^= PSR_SSBS_BIT, with the
> > change checked via a subsequent get_current_context().
> >
>
> Ok I'll check on ID_AA64PFR1_EL1.SSBS >= 1 instead of
> HWCAP_SSBS,since it is all I need to be able to use PSTATE.SSBS (with
> .SSBS=2 I've got also MRS/MSR which does NOT hurt)
>
> I'll SKIP if support is missing returning KSFT_SKIP (extending
> test_init related logic for this)
>
> > In other words, we no longer try to initialise SSBS to a particular
> > value. Instead, we just check the we can change the bit.
> >
> > Does that make sense?
> >
>
> Yes but I'll have anyway to test by setting to SSBS in the ucontext
> (only when SSBS supported as said), and verify that is NOT cleared, because
> if I toggle the bit 1-->0 then I would have nothing to check really.
> (I made this error already in a previous iteration...)
Hmmm, OK, I'll take a another look when reviewing v7.
Cheers
---Dave
