On Tue, Jul 02, 2019 at 04:51:38PM +0100, Cristian Marussi wrote:
> Hi
>
> On 6/21/19 11:35 AM, Dave Martin wrote:
> > On Thu, Jun 13, 2019 at 12:13:30PM +0100, Cristian Marussi wrote:
> >> Added a simple mangle testcase which messes with the ucontext_t
> >> from within the sig_handler, trying to toggle PSTATE SSBS bit.
> >> Expect SIGILL if SSBS feature unsupported or that the value set in
> >> PSTATE.SSBS is preserved on test PASS.
> >>
> >> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> >> ---
> >> .../arm64/signal/testcases/.gitignore | 1 +
> >> .../testcases/mangle_pstate_ssbs_regs.c | 41 +++++++++++++++++++
> >> 2 files changed, 42 insertions(+)
> >> create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
> >>
> >> diff --git a/tools/testing/selftests/arm64/signal/testcases/.gitignore b/tools/testing/selftests/arm64/signal/testcases/.gitignore
> >> index e7a1d998b650..c2972c3f33ca 100644
> >> --- a/tools/testing/selftests/arm64/signal/testcases/.gitignore
> >> +++ b/tools/testing/selftests/arm64/signal/testcases/.gitignore
> >> @@ -5,3 +5,4 @@ mangle_pstate_invalid_state_toggle
> >> 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..d997ebf742d9
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
> >> @@ -0,0 +1,41 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/* Copyright (C) 2019 ARM Limited */
> >> +
> >> +#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);
> >> +
> >> + /* toggle bit value */
> >> + uc->uc_mcontext.pstate ^= PSR_SSBS_BIT;
> >> + /* Save after mangling...it should be preserved */
> >> + td->saved_uc = *uc;
> >> +
> >> + return 1;
> >> +}
> >> +
> >> +static int pstate_ssbs_bit_checks(struct tdescr *td)
> >> +{
> >> + uint64_t val = 0;
> >> +
> >> + get_regval(MRS_SSBS_SYSREG, val);
> >> + /* pass when preserved */
> >> + td->pass = (!!(val & MRS_SSBS_BIT) ==
> >> + !!(td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT));
> >
> > Nit: there's a redundant level of ! here, and the outer () are unnecessary:
> >
> > (!!a == !!b) -> !a == !b
> >
>
> This was me badly convinced (not sure where I got this) that the bitpos
> of PSR_SSBS_BIT in pstate was different from the bitpos as reported in
> the output of MRS SSBS, so I was trying to normalize the comparison to 1
> == 1 or 0 == 0
>
> ...but in fact bitpos is the same between PSTATE and MSR SSBS so it can
> be compared directly.
Since the #defines are separate, it may be more readable to avoid
assuming that they have the same value (even if they do).
But I'm happy either way.
> > [...]
> >
> > Can we trigger a second signal after the first returns, to grab the
> > updated ucontext and check SSBS in there directly?
> >
> > Checking that the updated value is _also_ visible via MRS remains
> > useful though, so we should keep that.
> >
>
> I have added an informational message that reports the PSTATE and the
> status of SSBS as grabbed from uc via an induced SIGUSR2.
> Test outcome is anyway determined on MRS SSBS result.
OK, sounds reasonable.
> If HWCAP_SSBS is available the feature is considered available and so
> MRS SSBS MUST work.
Yup
> If instead feature is NOT supported as stated in HWCAP_SSBS the test is
> anyway run but the MRS SSBS is expected to cause a SIGILL (COULD not
> SHOULD...since HW_CAP could be reporting wrong caps and so MRS SSBS will
> still work)
Probably reasonable. I don't recall whether the architecture actually
guarantees a SIGILL though, or whether other things could happen (such
as executing as a NOP, or reading a fixed value such as 0).
Can this MRS be trapped? I'm wondering whether it will still just
execute normally if running on hardware that has it, but on an old
kernel that doesn't report HWCAP_SSBS. Some of the other tests may
fail on such a kernel though -- valid_user_regs() would probaly
mask the bit out when setting the register through sigreturn or ptrace.
> Moreover I fixed a small glitch: I was toggling the SSBS bit in uc
> PSTATE, BUT this make no sense...toggling to zero there's NO way I can
> check if Kernel preserve it to zero...so now I'm setting to 1 and then
> check if it has been preserved by Kernel
Well, I guess you can check both ways, but the bit was previously RES0,
so if the kernel masks it out it should be stuck at 0.
So agreed, attempting to set it to 1 is the more interesting test.
Cheers
---Dave
