On Thu, Dec 14, 2023 at 02:57:11PM +0100, Jan Beulich wrote:
> On 14.12.2023 14:47, Roger Pau Monné wrote:
> > On Thu, Dec 14, 2023 at 12:55:22PM +0100, Jan Beulich wrote:
> >> On 14.12.2023 11:17, Roger Pau Monne wrote:
> >>> --- 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-smoc.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();
> >>>
> >>> + test_smoc(XEN_SYSCTL_TEST_SMOC_ALL, NULL);
> >>
> >> I realize I'm at risk of causing scope creep, but I'd still like to at
> >> least ask: As further self-tests are added, we likely don't want to
> >> alter __start_xen() every time. Should there perhaps better be a wrapper
> >> (going forward: multiple ones, depending on the time tests want invoking),
> >> together with a Kconfig control to allow suppressing all of these tests in
> >> at least release builds?
> >
> > Right now I only had in mind that livepatch related tests won't be
> > executed as part of the call in __start_xen(), but all the other ones
> > would, and hence wasn't expecting the code to change from the form in
> > the next patch.
>
> Well, I was thinking of there more stuff appearing in test/, not self-
> modifying-code related, and hence needing further test_*() alongside.
> test_smoc().
Oh, I see. I think it might be best to introduce such wrapper when we
have at least 2 different self tests? Otherwise it would be weird IMO
to have another function (ie: execute_self_tests()?) that's just a
wrapper around test_smoc().
> >>> --- /dev/null
> >>> +++ b/xen/arch/x86/test/smoc.c
> >>> @@ -0,0 +1,68 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>> +
> >>> +#include <xen/errno.h>
> >>> +
> >>> +#include <asm/alternative.h>
> >>> +#include <asm/cpufeature.h>
> >>> +#include <asm/test-smoc.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;
> >>> +
> >>> + printk(XENLOG_INFO "Checking Self Modify Code\n");
> >>> +
> >>> + 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;
> >>> + }
> >>> +
> >>> + add_taint(TAINT_ERROR_SMOC);
> >>> + printk(XENLOG_ERR "%s test failed\n", tests[i].name);
> >>
> >> Do we really want both of these even when coming here from the sysctl?
> >
> > So only print the messages if system_state < SYS_STATE_active?
>
> Yes. Nor tainting the system.
OK.
> >>> --- 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_SMOC ? 'A' : ' ');
> >>
> >> How well is this going to scale as other selftests are added? IOW should
> >> this taint really be self-modifying-code-specific?
> >
> > I'm afraid I'm not sure I'm following. Would you instead like to make
> > the taint per-test selectable?
>
> The other way around actually: Taint generally for failed selftests,
> not just for the self-modifying-code one (which ends up being the only
> one right now).
So the suggestion would be to use TAINT_ERROR_SELFTEST instead of
TAINT_ERROR_SMOC? I can do that, but it might also be more
appropriate when there are more self tests.
Thanks, Roger.
