diff mbox series

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

Message ID 20231214101719.18770-3-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series xen/x86: add testing for self modifying code and livepatch | expand

Commit Message

Roger Pau Monné Dec. 14, 2023, 10:17 a.m. UTC
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 'A').

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 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-smoc.h | 18 ++++++++
 xen/arch/x86/setup.c                 |  3 ++
 xen/arch/x86/sysctl.c                |  9 ++++
 xen/arch/x86/test/Makefile           |  1 +
 xen/arch/x86/test/smoc.c             | 68 ++++++++++++++++++++++++++++
 xen/common/kernel.c                  |  5 +-
 xen/include/public/sysctl.h          | 10 ++++
 xen/include/xen/lib.h                |  1 +
 11 files changed, 130 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/x86/include/asm/test-smoc.h
 create mode 100644 xen/arch/x86/test/Makefile
 create mode 100644 xen/arch/x86/test/smoc.c

Comments

Jan Beulich Dec. 14, 2023, 11:55 a.m. UTC | #1
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?

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

> --- 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?

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1180,6 +1180,7 @@ struct xen_sysctl_cpu_policy {
>  };
>  typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> +
>  #endif
>  
>  #if defined(__arm__) || defined (__aarch64__)

Stray change (perhaps leftover from moving code around)?

Jan
Roger Pau Monné Dec. 14, 2023, 1:47 p.m. UTC | #2
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.

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

That would be OK by me.

> > --- 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?

> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -1180,6 +1180,7 @@ struct xen_sysctl_cpu_policy {
> >  };
> >  typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> > +
> >  #endif
> >  
> >  #if defined(__arm__) || defined (__aarch64__)
> 
> Stray change (perhaps leftover from moving code around)?

Indeed, sorry.

Thanks, Roger.
Jan Beulich Dec. 14, 2023, 1:57 p.m. UTC | #3
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
Roger Pau Monné Dec. 14, 2023, 3:28 p.m. UTC | #4
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.
Jan Beulich Dec. 14, 2023, 3:33 p.m. UTC | #5
On 14.12.2023 16:28, Roger Pau Monné wrote:
> 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().

That's precisely why I said "risk of causing scope creep, but I'd still
like to at least ask". I'm okay-ish, as long as it's clear that this
way more code churn may happen down the road. Same ...

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

... here - of course we can also rename later.

Jan
diff mbox series

Patch

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-smoc.h b/xen/arch/x86/include/asm/test-smoc.h
new file mode 100644
index 000000000000..2547b925d291
--- /dev/null
+++ b/xen/arch/x86/include/asm/test-smoc.h
@@ -0,0 +1,18 @@ 
+#ifndef _ASM_X86_TEST_SMC_H_
+#define _ASM_X86_TEST_SMC_H_
+
+#include <xen/types.h>
+
+int test_smoc(uint32_t selection, uint32_t *results);
+
+#endif	/* _ASM_X86_TEST_SMC_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..e026b0ea5adc 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-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);
+
     /*
      * 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..918e56631b94 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-smoc.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..e7529f937a5a
--- /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);
+    }
+
+    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..8ddfd309f65e 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_SMOC ? 'A' : ' ');
     }
     else
     {
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 9b19679caeb1..4b17f1344732 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1180,6 +1180,7 @@  struct xen_sysctl_cpu_policy {
 };
 typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
+
 #endif
 
 #if defined(__arm__) || defined (__aarch64__)
@@ -1201,6 +1202,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 +1240,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 +1268,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..575431e4bd38 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_SMOC                (1U << 6)
 extern unsigned int tainted;
 #define TAINT_STRING_MAX_LEN            20
 extern char *print_tainted(char *str);