On Fri, Sep 06, 2019 at 11:26:29AM +0100, Cristian Marussi wrote:
> Hi
>
> On 04/09/2019 12:47, Dave Martin wrote:
> > ^Nit: "add one testcase" doesn't really describe what is being added here.
> >
>
> Yep I know...I was trying to stay under first commit line length limitations
>
> > Maybe the following would work as the subject line:
> >
> > --8<--
> > kselftest: arm64: mangle_pstate_invalid_compat_toggle and common utils
> > -->8--
> >
> I'll grab it
>
>
> > The remainder of the commit message looks fine.
> >
> > On Mon, Sep 02, 2019 at 12:29:23pm +0100, Cristian Marussi wrote:
> >> Add some arm64/signal specific boilerplate and utility code to help
> >> further testcases' development.
> >>
> >> Introduce also one simple testcase mangle_pstate_invalid_compat_toggle
> >> and some related helpers: it is a simple mangle testcase which messes
> >> with the ucontext_t from within the signal handler, trying to toggle
> >> PSTATE state bits to switch the system between 32bit/64bit execution
> >> state. Expects SIGSEGV on test PASS.
> >>
> >> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> >> ---
> >> v4 --> v5
> >> - moved kernel headers include search to top level KSFT arm64 Makefile
> >> - removed warning about kernel headers not found
> >> - moved testcases/.gitignore up one level
> >> v3 --> v4
> >> - removed standalone mode
> >> - fixed arm64/signal/README
> >> - add file level comments: test layout / test description
> >> - reduced verbosity
> >> - removed spurious headers includes
> >> - reviewed ID_AA64MMFR[1,2]_EL1 macros
> >> - removed unused feats_ok
> >> - simplified CPU features gathering
> >> - reviewed included headers
> >> - fixed/refactored get_header() and validation routines
> >> - added test description
> >> ---
> >
> > [...]
> >
> >> diff --git a/tools/testing/selftests/arm64/signal/test_signals.c b/tools/testing/selftests/arm64/signal/test_signals.c
> >> new file mode 100644
> >> index 000000000000..f05c6dbf8659
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/arm64/signal/test_signals.c
> >> @@ -0,0 +1,29 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) 2019 ARM Limited
> >> + *
> >> + * Generic test wrapper for arm64 signal tests.
> >> + *
> >> + * Each test provides its own tde struct tddescr descriptor to link with
> >
> > Typo? tdescr
> >
> Yes
>
> > [...]
> >
> >> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
> >> new file mode 100644
> >> index 000000000000..e2a5f37e6ad3
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c
> >> @@ -0,0 +1,269 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Copyright (C) 2019 ARM Limited */
> >> +
> >> +#include <stdio.h>
> >> +#include <stdlib.h>
> >> +#include <signal.h>
> >> +#include <string.h>
> >> +#include <unistd.h>
> >> +#include <assert.h>
> >> +#include <sys/auxv.h>
> >> +#include <linux/auxvec.h>
> >> +#include <ucontext.h>
> >> +
> >> +#include "test_signals.h"
> >> +#include "test_signals_utils.h"
> >> +#include "testcases/testcases.h"
> >> +
> >> +extern struct tdescr *current;
> >> +
> >> +static char *feats_store[FMAX_END] = {
> >
> > Nit: can we call this feat_names[]?ok
> >
> > "store" makes me think of loads and stores...
> >
> > Also, nit: can this be static const char *const []?
> >
> > String literals are immutable anyway, and I guess we don't intend too
> > modify the pointers to the strings either...
> >
> Yes of course.
>
> >> + " SSBS ",
> >> + " PAN ",
> >> + " UAO ",
> >> +};
> >> +
> >> +#define MAX_FEATS_SZ 128
> >> +static char feats_string[MAX_FEATS_SZ];
> >> +
> >> +static inline char *feats_to_string(unsigned long feats)
> >> +{
> >> + size_t flen = MAX_FEATS_SZ - 1;
> >> +
> >> + for (int i = 0; i < FMAX_END; i++) {
> >> + if (feats & 1UL << i) {
> >
> > Nit: maybe have () around (1UL << i), though I think it makes no
> > difference.
>
> Yes it's better, I feared that, being not required, was frown upon.
>
> >
> >> + size_t tlen = strlen(feats_store[i]);
> >> +
> >> + assert(flen > tlen);
> >> + flen -= tlen;
> >> + strncat(feats_string, feats_store[i], flen);
> >> + }
> >> + }
> >> +
> >> + return feats_string;
> >> +}
> >> +
> >> +static void unblock_signal(int signum)
> >> +{
> >> + sigset_t sset;
> >> +
> >> + sigemptyset(&sset);
> >> + sigaddset(&sset, signum);
> >> + sigprocmask(SIG_UNBLOCK, &sset, NULL);
> >> +}
> >> +
> >> +static void default_result(struct tdescr *td, bool force_exit)
> >> +{
> >> + if (td->pass)
> >> + fprintf(stderr, "==>> completed. PASS(1)\n");
> >> + else
> >> + fprintf(stdout, "==>> completed. FAIL(0)\n");
> >> + if (force_exit)
> >> + exit(td->pass ? EXIT_SUCCESS : EXIT_FAILURE);
> >> +}
> >> +
> >> +static inline bool are_feats_ok(struct tdescr *td)
> >> +{
> >> + return (td->feats_required & td->feats_supported) == td->feats_required;
> >> +}
> >> +
> >> +static void default_handler(int signum, siginfo_t *si, void *uc)
> >> +{
> >> + if (current->sig_trig && signum == current->sig_trig) {
> >
> > (Thinking about it, signum is never 0 because there is no signal 0.
> > So we could write if (signum == current->sig_trig). But I think your
> > code makes the intention clearer -- so no need to change it.)
> >
>
> Yes, in fact that's the reason I left it even if unneeded.
Ok, sounds fine.
> >> + fprintf(stderr, "Handling SIG_TRIG\n");
> >> + current->triggered = 1;
> >> + /* ->run was asserted NON-NULL in test_setup() already */
> >> + current->run(current, si, uc);
> >> + } else if (signum == SIGILL && !current->initialized) {
[...]
> >> +static int test_init(struct tdescr *td)
> >> +{
> >> + td->minsigstksz = getauxval(AT_MINSIGSTKSZ);
> >> + if (!td->minsigstksz)
> >> + td->minsigstksz = MINSIGSTKSZ;
> >> + fprintf(stderr, "Detected MINSTKSIGSZ:%d\n", td->minsigstksz);
> >> +
> >> + if (td->feats_required) {
> >> + bool feats_ok = false;
> >> +
> >> + td->feats_supported = 0;
> >> + /*
> >> + * Checking for CPU required features using both the
> >> + * auxval and the arm64 MRS Emulation to read sysregs.
> >> + */
> >> + if (getauxval(AT_HWCAP) & HWCAP_CPUID) {
> >> + uint64_t val = 0;
> >> +
> >> + /* Uses HWCAP to check capability */
> >> + if (getauxval(AT_HWCAP) & HWCAP_SSBS)
> >> + td->feats_supported |= FEAT_SSBS;
> >
> > Should this be outside the HWCAP_CPUID check?
>
> Right.
>
> >
> > It's only the get_regval(SYS_ID_foo) based checks that depend on
> > HWCAP_CPUID.
> >
> >> + /* Uses MRS emulation to check capability */
> >> + get_regval(SYS_ID_AA64MMFR1_EL1, val);
> >> + if (ID_AA64MMFR1_EL1_PAN_SUPPORTED(val))
> >> + td->feats_supported |= FEAT_PAN;
> >> + /* Uses MRS emulation to check capability */
> >> + get_regval(SYS_ID_AA64MMFR2_EL1, val);
> >> + if (ID_AA64MMFR2_EL1_UAO_SUPPORTED(val))
> >> + td->feats_supported |= FEAT_UAO;
> >> + } else {
> >> + fprintf(stderr,
> >> + "HWCAP_CPUID NOT available. Mark ALL feats UNSUPPORTED.\n");
> >> + }
> >> + feats_ok = are_feats_ok(td);
> >> + fprintf(stderr,
> >> + "Required Features: [%s] %ssupported\n",
> >> + feats_ok ? feats_to_string(td->feats_supported) :
> >> + feats_to_string(td->feats_required ^
> >> + td->feats_supported),
> >
> > Should this be something like:
> > td->feats_required & ~td->feats_supported ?
> >
> > Otherwise we'll include features that are supported but not required,
> > when printing the features that are NOT supported.
> >
> > Alternatively, we could just print out the required and supported sets
> > separately and leave it up to the user to obverse how they are
> > different.
> >
> > (Watch out for calling feats_to_string() twice in the same printf() call
> > though.)
> >
> Ok. Reported information was poor in fact.
[..]
> >> diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c
> >> new file mode 100644
> >> index 000000000000..72e3f482b177
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c
> >> @@ -0,0 +1,151 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Copyright (C) 2019 ARM Limited */
> >> +#include "testcases.h"
> >> +
> >> +struct _aarch64_ctx *get_header(struct _aarch64_ctx *head, uint32_t magic,
> >> + size_t resv_sz, size_t *offset)
> >> +{
> >> + size_t offs = 0;
> >> + struct _aarch64_ctx *found = NULL;
> >> +
> >> + if (!head)
> >> + return found;
> >> +
> >
> > I suggest you also check for resv_sz < HDR_SZ, since the while()
> > condition assumes that resv_sz - HDR_SZ doesn't underflow.
> >
> > For now, I think resv_sz is already sizeof(__reserved) so this is never
> > true, but I suspect we will want to reuse this code eventually to looko
> > at the contents of extra_context. Then, resv_sz would be the
> > extra_context size rather than a fixed constant.
> >
>
> Ok....in fact I think I removed recently such check...not sure why o_O
> I'll fix it.
>
>
> >> + while (offs <= resv_sz - HDR_SZ &&
> >> + head->magic != magic && head->magic) {
> >> + offs += head->size;
> >> + head = GET_RESV_NEXT_HEAD(head);
> >> + }
> >> + if (head->magic == magic) {
> >> + found = head;
> >> + if (offset)
> >> + *offset = offs;
> >> + }
> >
> > Although there appears to be some code duplication here, I guess you
> > need things this way to do the right thing if called with magic==0.
> >
> > So I guess this is fine.
> >
> Yes that was exactly the point, and it seemed to me that removing further
> duplication would have made the code more complex and unreadable.
>
> > Ultimately it would be good to have GET_RESV_NEXT_HEAD() work more
> > like an iterator, doing integrity bounds/alignment checks and updating
> > offs as it goes, but for now I think the code is sufficient. We can
> > always beef it up later to catch more kinds of error from the kernel.
> >
> Yes I remember you told me that on a previous iteration, but for now I left the
> GET_RESV_NEXT_HEAD() as it was without embedding the bounds checking logic
> because it is indirectly used also by the validation function that I use in
> the ASSERT_GOOD/BAD_CONTEXT() macros, so it should be able to handle
> artficially badly formed and corrupted frames without bailing out: it just walks
> and any kind of logic is handled outside...but maybe I'm overthinking
> (certainly I have not explained this reasons anywhere...I'll add a comment)
Sure, I think that's sufficient.
We could always improve this later, if needed.
[...]
> >> +bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err)
> >> +{
> >> + bool terminated = false;
> >> + size_t offs = 0;
> >> + int flags = 0;
> >> + struct extra_context *extra = NULL;
> >> + struct _aarch64_ctx *head =
> >> + (struct _aarch64_ctx *)uc->uc_mcontext.__reserved;
> >> +
> >> + if (!err)
> >> + return false;
> >> + /* Walk till the end terminator verifying __reserved contents */
> >> + while (head && !terminated && offs < resv_sz) {
> >> + if ((uint64_t)head & 0x0fUL) {
> >> + *err = "Misaligned HEAD";
> >> + return false;
> >> + }
> >> +
> >> + switch (head->magic) {
> >> + case 0:
> >> + if (head->size)
> >> + *err = "Bad size for terminator";
> >> + else
> >> + terminated = true;
> >> + break;
> >> + case FPSIMD_MAGIC:
> >> + if (flags & FPSIMD_CTX)
> >> + *err = "Multiple FPSIMD_MAGIC";
> >> + else if (head->size !=
> >> + sizeof(struct fpsimd_context))
> >> + *err = "Bad size for fpsimd_context";
> >> + flags |= FPSIMD_CTX;
> >> + break;
> >> + case ESR_MAGIC:
> >> + if (head->size != sizeof(struct esr_context))
> >> + fprintf(stderr,
> >> + "Bad size for esr_context is not an error...just ignore.\n");
> >> + break;
> >
> > Although it's not essential, I'd prefer that we enforce the correct
> > size here. All records, including esr_context are intended to be
> > fixed-size.
> >
> > In the kernel we check a bit more loosely -- this allows userspace to
> > delete a record using head->size += next_head->size. This way no
> > memmove() is needed to shuffle subsequent records down. I don't know
> > whether any userspace code makes use of this -- prior to SVE there were
> > no optional records except for esr_context, and sigreturn ignores that
> > in any case so deleting it is pointless.
> >
>
> > The kernel should never insert extra padding between records though,
> > so I think it makes sense to have strict size checks in this test.
> >
>
> Ok, I'll do. I kept it loose as it is in Kernel, because in some past tests
> (now removed) I used to play also with esr_context size to build easily an inflated
> fake sigframe (but good) and adding some badness on top of it.
OK, I guess that would be a problem if we want to use the same validity
checker for kernel-generated and user-generated signal frames.
I'm happy either way ... I'll take a look at v6 and comment.
Cheers
---Dave
