Re: [PATCH v4 2/4] xen/x86: introduce self modifying code test

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/12/2023 11:18 am, Roger Pau Monne wrote:
> Introduce a helper to perform checks related to self modifying code, and start
> by creating a simple test to check that alternatives have been applied.
>
> Such test is hooked into the boot process and called just after alternatives
> have been applied.  In case of failure a message is printed, and the 
> hypervisor
> is tainted as not having passed the tests, this does require introducing a new
> taint bit (printed as 'T').

We've got stub_selftest() in extable.c which currently does an ah-hoc
form of this taint via warning_add().

Nothing else comes to mind, but I would suggest breaking out the new
taint into an earlier patch, as this one is complicated enough anyway.

> diff --git a/xen/arch/x86/include/asm/test.h b/xen/arch/x86/include/asm/test.h
> new file mode 100644
> index 000000000000..e96e709c6a52
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/test.h
> @@ -0,0 +1,30 @@
> +#ifndef _ASM_X86_TEST_H_
> +#define _ASM_X86_TEST_H_
> +
> +#include <xen/types.h>
> +
> +int test_smoc(uint32_t selection, uint32_t *results);
> +
> +static inline void execute_selftests(void)

IMO run_selftests() would be better, but this is already not all of our
selftests, and this particular function really doesn't warrant being
static inline.

But I'm also not sure what this is liable to contain other than
test_smoc() so I'm not sure why we want it.

> diff --git a/xen/arch/x86/test/smoc.c b/xen/arch/x86/test/smoc.c
> new file mode 100644
> index 000000000000..09db5cee9ae2
> --- /dev/null
> +++ b/xen/arch/x86/test/smoc.c
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <xen/errno.h>
> +
> +#include <asm/alternative.h>
> +#include <asm/cpufeature.h>
> +#include <asm/test.h>
> +
> +static bool cf_check test_insn_replacement(void)
> +{
> +#define EXPECTED_VALUE 2
> +    unsigned int r = ~EXPECTED_VALUE;
> +
> +    alternative_io("", "mov %1, %0", X86_FEATURE_ALWAYS,
> +                   "+r" (r), "i" (EXPECTED_VALUE));
> +
> +    return r == EXPECTED_VALUE;
> +#undef EXPECTED_VALUE
> +}
> +
> +int test_smoc(uint32_t selection, uint32_t *results)
> +{
> +    struct {
> +        unsigned int mask;
> +        bool (*test)(void);
> +        const char *name;
> +    } static const tests[] = {
> +        { XEN_SYSCTL_TEST_SMOC_INSN_REPL, &test_insn_replacement,
> +          "alternative instruction replacement" },
> +    };

Ah.  I realise I said "like XTF", but I meant "checking one thing at a
time".

While this pattern for tests[] is very convenient in XTF, it has one
major downside in Xen, and that's the proliferation of ENDBR's in the
running binary.

Also (see below), returning bool isn't ok.  In the case of a failure, we
need:

printk(XENLOG_ERR "%s() Failure: Expected $FOO, got $BAR\n");

because that's what a human needs to know in order to fix the issue, not
a generic "something failed".

> +    unsigned int i;
> +
> +    if ( selection & ~XEN_SYSCTL_TEST_SMOC_ALL )
> +        return -EINVAL;

I'm not sure this is sensible.  It's a testing hypercall, so why
shouldn't I be able to pass ~0 to mean "test everything the hypervisor
knows about" ?

> +
> +    if ( results )
> +        *results = 0;
> +
> +    for ( i = 0; i < ARRAY_SIZE(tests); i++ )
> +    {
> +        if ( !(selection & tests[i].mask) )
> +            continue;
> +
> +        if ( tests[i].test() )
> +        {
> +            if ( results )
> +                *results |= tests[i].mask;

How is results supposed to be used?

XEN_SYSCTL_TEST_SMOC_INSN_REPL covers about 15 things we want to test,
making this output mask useless.


The selftests, like the exception fixup ones, are supposed to be
guarantee pass.  Failure is an exceptional case, and is only expected to
be found with new compilers and new SMC development.

I can kind of see how an input mask might be useful, although I wouldn't
have had one myself.  With correct diagnostics, running the hypercall
multiple times isn't useful to debugging, and without correct
diagnostics, the feedback provided by this is useless.

So honestly, I think this "results" output is overengineered and doesn't
help the cases where it is actually going to matter.


Remember most of all that self-modifying code which are going to cause
failures here have a high chance of crashing Xen outright.  And we're
deliberately trying to make this happen in CI and before a breaking
change gets out into releases.

> +            continue;
> +        }
> +
> +        if ( system_state < SYS_STATE_active )
> +            printk(XENLOG_ERR "%s test failed\n", tests[i].name);

This is a test hypercall, for the purpose of running testing, in
combination with test livepatches.  Eliding the diagnostics isn't ok.

Logspam concerns aren't an issue.  If the user runs `while :; do
xen-test-smc; done` in dom0 then they get to have a full dmesg ring.

Don't let that get in the way of having a sensible time figuring out
what went wrong.

~Andrew