Re: [PATCH v5 02/11] kselftest: arm64: add common utils and one testcase

[Cristian Marussi <cristian.marussi@arm.com>]

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

>> +		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) {
>> +		/*
>> +		 * A SIGILL here while still not initialized means we failed
>> +		 * even to asses the existence of features during init
>> +		 */
>> +		fprintf(stdout,
>> +			"Got SIGILL test_init. Marking ALL features UNSUPPORTED.\n");
>> +		current->feats_supported = 0;
>> +	} else if (current->sig_ok && signum == current->sig_ok) {
>> +		/*
>> +		 * it's a bug in the test code when this assert fail:
>> +		 * if a sig_trig was defined, it must have been used before
>> +		 * arriving here.
>> +		 */
>> +		assert(!current->sig_trig || current->triggered);
>> +		fprintf(stderr,
>> +			"SIG_OK -- SP:0x%llX  si_addr@:%p  si_code:%d  token@:%p  offset:%ld\n",
>> +			((ucontext_t *)uc)->uc_mcontext.sp,
>> +			si->si_addr, si->si_code, current->token,
>> +			current->token - si->si_addr);
>> +		/*
>> +		 * fake_sigreturn tests, which have sanity_enabled=1, set, at
>> +		 * the very last time, the token field to the SP address used
>> +		 * to place the fake sigframe: so token==0 means we never made
>> +		 * it to the end, segfaulting well-before, and the test is
>> +		 * possibly broken.
>> +		 */
>> +		if (!current->sanity_disabled && !current->token) {
>> +			fprintf(stdout,
>> +				"current->token ZEROED...test is probably broken!\n");
>> +			abort();
>> +		}
>> +		/*
>> +		 * Trying to narrow down the SEGV to the ones generated by
>> +		 * Kernel itself via arm64_notify_segfault().
>> +		 * This is a best-effort check anyway, and the si_code check may
>> +		 * need to change if this aspect of the kernel ABI changes.
>> +		 */
>> +		if (current->sig_ok == SIGSEGV && si->si_code != SEGV_ACCERR) {
>> +			fprintf(stdout,
>> +				"si_code != SEGV_ACCERR...test is probably broken!\n");
>> +			abort();
>> +		}
>> +		fprintf(stderr, "Handling SIG_OK\n");
>> +		current->pass = 1;
>> +		/*
>> +		 * Some tests can lead to SEGV loops: in such a case we want
>> +		 * to terminate immediately exiting straight away
>> +		 */
>> +		default_result(current, 1);
>> +	} else {
>> +		if (signum == current->sig_unsupp && !are_feats_ok(current)) {
>> +			fprintf(stderr,
>> +				"-- RX SIG_UNSUPP on unsupported feat...OK\n");
>> +			current->pass = 1;
>> +		} else if (signum == SIGALRM && current->timeout) {
>> +			fprintf(stderr, "-- Timeout !\n");
>> +		} else {
>> +			fprintf(stderr,
>> +				"-- RX UNEXPECTED SIGNAL: %d\n", signum);
>> +		}
>> +		default_result(current, 1);
>> +	}
>> +}
>> +
>> +static int default_setup(struct tdescr *td)
>> +{
>> +	struct sigaction sa;
>> +
>> +	sa.sa_sigaction = default_handler;
>> +	sa.sa_flags = SA_SIGINFO | SA_RESTART;
>> +	sa.sa_flags |= td->sa_flags;
>> +	sigemptyset(&sa.sa_mask);
>> +	/* uncatchable signals naturally skipped ... */
>> +	for (int sig = 1; sig < 32; sig++)
>> +		sigaction(sig, &sa, NULL);
>> +	/*
>> +	 * RT Signals default disposition is Term but they cannot be
>> +	 * generated by the Kernel in response to our tests; so just catch
>> +	 * them all and report them as UNEXPECTED signals.
>> +	 */
>> +	for (int sig = SIGRTMIN; sig <= SIGRTMAX; sig++)
>> +		sigaction(sig, &sa, NULL);
>> +
>> +	/* just in case...unblock explicitly all we need */
>> +	if (td->sig_trig)
>> +		unblock_signal(td->sig_trig);
>> +	if (td->sig_ok)
>> +		unblock_signal(td->sig_ok);
>> +	if (td->sig_unsupp)
>> +		unblock_signal(td->sig_unsupp);
>> +
>> +	if (td->timeout) {
>> +		unblock_signal(SIGALRM);
>> +		alarm(td->timeout);
>> +	}
>> +	fprintf(stderr, "Registered handlers for all signals.\n");
>> +
>> +	return 1;
>> +}
>> +
>> +static inline int default_trigger(struct tdescr *td)
>> +{
>> +	return !raise(td->sig_trig);
>> +}
>> +
>> +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.

>> +			!feats_ok ? "NOT " : "");
>> +	}
>> +
>> +	td->initialized = 1;
>> +	return 1;
>> +}
>> +
>> +int test_setup(struct tdescr *td)
>> +{
>> +	/* assert core invariants symptom of a rotten testcase */
>> +	assert(current);
>> +	assert(td);
>> +	assert(td->name);
>> +	assert(td->run);
>> +
>> +	if (!test_init(td))
>> +		return 0;
>> +
>> +	if (td->setup)
>> +		return td->setup(td);
>> +	else
>> +		return default_setup(td);
>> +}
>> +
>> +int test_run(struct tdescr *td)
>> +{
>> +	if (td->sig_trig) {
>> +		if (td->trigger)
>> +			return td->trigger(td);
>> +		else
>> +			return default_trigger(td);
>> +	} else {
>> +		return td->run(td, NULL, NULL);
>> +	}
>> +}
>> +
>> +void test_result(struct tdescr *td)
>> +{
>> +	if (td->check_result)
>> +		td->check_result(td);
>> +	default_result(td, 0);
>> +}
>> +
>> +void test_cleanup(struct tdescr *td)
>> +{
>> +	if (td->cleanup)
>> +		td->cleanup(td);
>> +}
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> new file mode 100644
>> index 000000000000..8658d1a7d4b9
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2019 ARM Limited */
>> +
>> +#ifndef __TEST_SIGNALS_UTILS_H__
>> +#define __TEST_SIGNALS_UTILS_H__
>> +
>> +#include "test_signals.h"
>> +
>> +int test_setup(struct tdescr *td);
>> +void test_cleanup(struct tdescr *td);
>> +int test_run(struct tdescr *td);
>> +void test_result(struct tdescr *td);
>> +#endif
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c
>> new file mode 100644
>> index 000000000000..2cb118b0ba05
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 ARM Limited
>> + *
>> + * Try to mangle the ucontext from inside a signal handler, toggling
>> + * the execution state bit: this attempt must be spotted by Kernel and
>> + * the test case is expected to be terminated via SEGV.
>> + */
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +static int mangle_invalid_pstate_run(struct tdescr *td, siginfo_t *si,
>> +				     ucontext_t *uc)
>> +{
>> +	ASSERT_GOOD_CONTEXT(uc);
>> +
>> +	/* This config should trigger a SIGSEGV by Kernel */
>> +	uc->uc_mcontext.pstate ^= PSR_MODE32_BIT;
>> +
>> +	return 1;
>> +}
>> +
>> +struct tdescr tde = {
>> +		.sanity_disabled = true,
>> +		.name = "MANGLE_PSTATE_INVALID_STATE_TOGGLE",
>> +		.descr = "Mangling uc_mcontext with INVALID STATE_TOGGLE",
>> +		.sig_trig = SIGUSR1,
>> +		.sig_ok = SIGSEGV,
>> +		.run = mangle_invalid_pstate_run,
>> +};
>> 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)

>> +
>> +	return found;
>> +}
>> +
>> +bool validate_extra_context(struct extra_context *extra, char **err)
>> +{
>> +	struct _aarch64_ctx *term;
>> +
>> +	if (!extra || !err)
>> +		return false;
>> +
>> +	fprintf(stderr, "Validating EXTRA...\n");
>> +	term = GET_RESV_NEXT_HEAD(extra);
>> +	if (!term || term->magic || term->size) {
>> +		*err = "Missing terminator after EXTRA context";
>> +		return false;
>> +	}
>> +	if (extra->datap & 0x0fUL)
>> +		*err = "Extra DATAP misaligned";
>> +	else if (extra->size & 0x0fUL)
>> +		*err = "Extra SIZE misaligned";
>> +	else if (extra->datap != (uint64_t)term + sizeof(*term))
>> +		*err = "Extra DATAP misplaced (not contiguos)";
>> +	if (*err)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +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.

> [...]
> 
> Cheers
> ---Dave
> 

Cheers

Cristian