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@arm.com
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 andI 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