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

[Jan Beulich <jbeulich@xxxxxxxx>]

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().

>>> --- /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.

>>> --- 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).

Jan