| Message ID | 20231215111842.8009-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/x86: add testing for self modifying code and livepatch | expand |
On 15.12.2023 12:18, 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'). > > A new sysctl is also introduced to run the tests on demand. While there are no > current users introduced here, further changes will introduce those, and it's > helpful to have the interface defined in the sysctl header from the start. > > Note the sysctl visibility is not limited to x86, albeit the only > implementation is for x86. It's expected that other architectures can reuse > the same sysctl and structure, with possibly different tests. Leave adjusting > those to when support for a different architecture is introduced, as the > sysctl interface is not stable anyway. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com>
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
On Tue, Dec 19, 2023 at 08:31:29PM +0000, Andrew Cooper wrote: > 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. I see, so introduce the taint in a previous patch and use it in stub_selftest() failure, > > 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. This was requested by Jan, he was concerned that more of such tests would appear. It's new in v4 IIRC. > > 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. But for the livepatch case for example it's interesting to patch functions that have the ENDBR prefix. I do like having all the tests in an array, as then adding new ones is trivial. > 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"); There's already a message printed below, that's currently limited to system_state < SYS_STATE_active, but I would be fine with printing unconditionally that prints which test failed in a human readable form: printk(XENLOG_ERR "%s test failed\n", tests[i].name); So that would print: "alternative instruction replacement test failed" on the Xen dmesg. On one of the first versions test functions did return a value, but I ended up switching to this boolean version because I didn't see much value in returning anything that's not success or failure from the tests. I can switch back to returning a value, and then the array of tests will also store the expected returned value. > 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" ? Well, for one livepatch tests will fail if the livepatch hasn't been applied yet. > > + > > + 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 output mask just maps the input tests into output results. For example given the case you want to execute all tests (~0), and the livepatch replacements haven't been applied yet, the altinstructions test will succeed, but the livepatch ones will fail (as expected), we need a way to report this back to the caller. > > 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. Livepatch tests (at least the one I have implemented in patch 3) is expected to fail until a livepatch is applied to make it succeed. We do care about checking that it first fails, then we upload the livepatch and it succeeds, and that reverting the livepatch makes it fail again. > 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. So for altinstructions it's true that the expectation is for them to always succeed, that's not the case for livepatch ones, where it's useful to explicitly test for failure, hence we need a fine grained way to report failure of specific tests. > > 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. This was requested by Jan, and indeed my original intention was to unconditionally print the messages, as I think they are helpful. Thanks, Roger.
On 19.12.2023 21:31, Andrew Cooper wrote: > On 15/12/2023 11:18 am, Roger Pau Monne wrote: >> + 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. Since it was me who asked to suppress this when invoked through sysctl: I can live with an unconditional printk() here, but I think this goes against the "what can be done in user space should be done there" principle: If enough information is propagated back, user space should be able to provide all necessary output without even a need for the observer to run "xl dmesg". Jan
On 20.12.2023 10:08, Roger Pau Monné wrote: > On Tue, Dec 19, 2023 at 08:31:29PM +0000, Andrew Cooper wrote: >> On 15/12/2023 11:18 am, Roger Pau Monne wrote: >>> --- /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. > > This was requested by Jan, he was concerned that more of such tests > would appear. It's new in v4 IIRC. If the two of you want it without such a wrapper, so be it. I will admit I was a little puzzled to find it implemented as inline function, but I didn't see a strong need to comment on that. Jan
On Wed, Dec 20, 2023 at 10:12:15AM +0100, Jan Beulich wrote: > On 20.12.2023 10:08, Roger Pau Monné wrote: > > On Tue, Dec 19, 2023 at 08:31:29PM +0000, Andrew Cooper wrote: > >> On 15/12/2023 11:18 am, Roger Pau Monne wrote: > >>> --- /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. > > > > This was requested by Jan, he was concerned that more of such tests > > would appear. It's new in v4 IIRC. > > If the two of you want it without such a wrapper, so be it. I will admit > I was a little puzzled to find it implemented as inline function, but I > didn't see a strong need to comment on that. It felt a bit misplaced to put such generic selftest function in a smoc.c file, and I didn't feel like introducing a new test.c file just for that. I don't have a strong opinion however, if you think it's better to go in smoc.c I'm happy with that. Thanks, Roger.
On 20.12.2023 10:25, Roger Pau Monné wrote: > On Wed, Dec 20, 2023 at 10:12:15AM +0100, Jan Beulich wrote: >> On 20.12.2023 10:08, Roger Pau Monné wrote: >>> On Tue, Dec 19, 2023 at 08:31:29PM +0000, Andrew Cooper wrote: >>>> On 15/12/2023 11:18 am, Roger Pau Monne wrote: >>>>> --- /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. >>> >>> This was requested by Jan, he was concerned that more of such tests >>> would appear. It's new in v4 IIRC. >> >> If the two of you want it without such a wrapper, so be it. I will admit >> I was a little puzzled to find it implemented as inline function, but I >> didn't see a strong need to comment on that. > > It felt a bit misplaced to put such generic selftest function in a > smoc.c file, and I didn't feel like introducing a new test.c file just > for that. I don't have a strong opinion however, if you think it's > better to go in smoc.c I'm happy with that. My view: smoc.c would be wrong. Then it's better to have no wrapper (for now). Jan
On Wed, Dec 20, 2023 at 10:09:12AM +0100, Jan Beulich wrote: > On 19.12.2023 21:31, Andrew Cooper wrote: > > On 15/12/2023 11:18 am, Roger Pau Monne wrote: > >> + 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. > > Since it was me who asked to suppress this when invoked through sysctl: > I can live with an unconditional printk() here, but I think this goes > against the "what can be done in user space should be done there" > principle: If enough information is propagated back, user space should > be able to provide all necessary output without even a need for the > observer to run "xl dmesg". But propagating such information to user-space is also not trivial, and involves more logic in the hypervisor. I think the point of "what can be done in user space should be done there" is to avoid adding unnecessary code to Xen, but in this case printing such detailed information in user-space would require adding more code to Xen in order to propagate it. Thanks, Roger.
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index 2ef8b4e05422..0af796ae84e8 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -2658,6 +2658,8 @@ int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, uint32_t overlay_fdt_size, uint8_t overlay_op); #endif +int xc_test_smoc(xc_interface *xch, uint32_t tests, uint32_t *result); + /* Compat shims */ #include "xenctrl_compat.h" diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c index 5ecdfa2c7934..1d3d5929cf96 100644 --- a/tools/libs/ctrl/xc_misc.c +++ b/tools/libs/ctrl/xc_misc.c @@ -1021,6 +1021,20 @@ int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint32 return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REPLACE, timeout, flags); } +int xc_test_smoc(xc_interface *xch, uint32_t tests, uint32_t *result) +{ + struct xen_sysctl sysctl = { + .cmd = XEN_SYSCTL_test_smoc, + .u.test_smoc.tests = tests, + }; + int rc = do_sysctl(xch, &sysctl); + + if ( !rc ) + *result = sysctl.u.test_smoc.results; + + return rc; +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index f3abdf9cd111..ad5112b03c64 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_HVM) += hvm/ obj-y += mm/ obj-$(CONFIG_XENOPROF) += oprofile/ obj-$(CONFIG_PV) += pv/ +obj-y += test/ obj-y += x86_64/ obj-y += x86_emulate/ 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) +{ + const uint32_t exec_mask = XEN_SYSCTL_TEST_SMOC_ALL; + uint32_t result; + int rc; + + printk(XENLOG_INFO "Checking Self Modify Code\n"); + rc = test_smoc(exec_mask, &result); + if ( rc || (result & exec_mask) != exec_mask ) + add_taint(TAINT_ERROR_SELFTEST); +} + +#endif /* _ASM_X86_TEST_H_ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 3cba2be0af6c..e974a8f8d725 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -58,6 +58,7 @@ #include <asm/microcode.h> #include <asm/prot-key.h> #include <asm/pv/domain.h> +#include <asm/test.h> /* opt_nosmp: If true, secondary processors are ignored. */ static bool __initdata opt_nosmp; @@ -1951,6 +1952,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) alternative_branches(); + execute_selftests(); + /* * NB: when running as a PV shim VCPUOP_up/down is wired to the shim * physical cpu_add/remove functions, so launch the guest with only diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c index 1d40d82c5ad2..a61a99f8696a 100644 --- a/xen/arch/x86/sysctl.c +++ b/xen/arch/x86/sysctl.c @@ -25,6 +25,7 @@ #include <asm/processor.h> #include <asm/setup.h> #include <asm/smp.h> +#include <asm/test.h> #include <asm/numa.h> #include <xen/nodemask.h> #include <xen/cpu.h> @@ -423,6 +424,14 @@ long arch_do_sysctl( break; } + case XEN_SYSCTL_test_smoc: + ret = test_smoc(sysctl->u.test_smoc.tests, + &sysctl->u.test_smoc.results); + if ( !ret && __copy_field_to_guest(u_sysctl, sysctl, + u.test_smoc.results) ) + ret = -EFAULT; + break; + default: ret = -ENOSYS; break; diff --git a/xen/arch/x86/test/Makefile b/xen/arch/x86/test/Makefile new file mode 100644 index 000000000000..b504b8196659 --- /dev/null +++ b/xen/arch/x86/test/Makefile @@ -0,0 +1 @@ +obj-y += smoc.o 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" }, + }; + unsigned int i; + + if ( selection & ~XEN_SYSCTL_TEST_SMOC_ALL ) + return -EINVAL; + + 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; + continue; + } + + if ( system_state < SYS_STATE_active ) + printk(XENLOG_ERR "%s test failed\n", tests[i].name); + } + + return 0; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 08dbaa2a054c..77f93f137cac 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -386,13 +386,14 @@ char *print_tainted(char *str) { if ( tainted ) { - snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c%c", + snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c%c%c", tainted & TAINT_MACHINE_INSECURE ? 'I' : ' ', tainted & TAINT_MACHINE_CHECK ? 'M' : ' ', tainted & TAINT_SYNC_CONSOLE ? 'C' : ' ', tainted & TAINT_ERROR_INJECT ? 'E' : ' ', tainted & TAINT_HVM_FEP ? 'H' : ' ', - tainted & TAINT_CPU_OUT_OF_SPEC ? 'S' : ' '); + tainted & TAINT_CPU_OUT_OF_SPEC ? 'S' : ' ', + tainted & TAINT_ERROR_SELFTEST ? 'T' : ' '); } else { diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 9b19679caeb1..d015a490da38 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -1201,6 +1201,13 @@ struct xen_sysctl_dt_overlay { }; #endif +struct xen_sysctl_test_smoc { + uint32_t tests; /* IN: bitmap with selected tests to execute. */ +#define XEN_SYSCTL_TEST_SMOC_INSN_REPL (1U << 0) +#define XEN_SYSCTL_TEST_SMOC_ALL (XEN_SYSCTL_TEST_SMOC_INSN_REPL) + uint32_t results; /* OUT: test result: 1 -> success, 0 -> failure. */ +}; + struct xen_sysctl { uint32_t cmd; #define XEN_SYSCTL_readconsole 1 @@ -1232,6 +1239,7 @@ struct xen_sysctl { /* #define XEN_SYSCTL_set_parameter 28 */ #define XEN_SYSCTL_get_cpu_policy 29 #define XEN_SYSCTL_dt_overlay 30 +#define XEN_SYSCTL_test_smoc 31 uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */ union { struct xen_sysctl_readconsole readconsole; @@ -1259,6 +1267,7 @@ struct xen_sysctl { struct xen_sysctl_cpu_levelling_caps cpu_levelling_caps; struct xen_sysctl_cpu_featureset cpu_featureset; struct xen_sysctl_livepatch_op livepatch; + struct xen_sysctl_test_smoc test_smoc; #if defined(__i386__) || defined(__x86_64__) struct xen_sysctl_cpu_policy cpu_policy; #endif diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 1793be5b6b89..dcb42a5b64c9 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -167,6 +167,7 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c); #define TAINT_HVM_FEP (1u << 3) #define TAINT_MACHINE_INSECURE (1u << 4) #define TAINT_CPU_OUT_OF_SPEC (1u << 5) +#define TAINT_ERROR_SELFTEST (1U << 6) extern unsigned int tainted; #define TAINT_STRING_MAX_LEN 20 extern char *print_tainted(char *str);
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'). A new sysctl is also introduced to run the tests on demand. While there are no current users introduced here, further changes will introduce those, and it's helpful to have the interface defined in the sysctl header from the start. Note the sysctl visibility is not limited to x86, albeit the only implementation is for x86. It's expected that other architectures can reuse the same sysctl and structure, with possibly different tests. Leave adjusting those to when support for a different architecture is introduced, as the sysctl interface is not stable anyway. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v3: - Rename taint variable. - Introduce a wrapper to run all selftests. - Only print messages and taint the hypervisor when tests are executed on boot. Changes since v2: - Rename to smoc and place in test/smoc* - fix inline assembly. Changes since v1: - Rework test and interface. --- tools/include/xenctrl.h | 2 + tools/libs/ctrl/xc_misc.c | 14 +++++++ xen/arch/x86/Makefile | 1 + xen/arch/x86/include/asm/test.h | 30 +++++++++++++++ xen/arch/x86/setup.c | 3 ++ xen/arch/x86/sysctl.c | 9 +++++ xen/arch/x86/test/Makefile | 1 + xen/arch/x86/test/smoc.c | 66 +++++++++++++++++++++++++++++++++ xen/common/kernel.c | 5 ++- xen/include/public/sysctl.h | 9 +++++ xen/include/xen/lib.h | 1 + 11 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 xen/arch/x86/include/asm/test.h create mode 100644 xen/arch/x86/test/Makefile create mode 100644 xen/arch/x86/test/smoc.c