Re: [PATCH v2 3/5] xen/x86: introduce self modifying code test

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 28 Nov 2023, 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 '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.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> 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-smc.h | 18 ++++++++
>  xen/arch/x86/setup.c                |  3 ++
>  xen/arch/x86/sysctl.c               |  7 +++
>  xen/arch/x86/test-smc.c             | 68 +++++++++++++++++++++++++++++

If possible, could we name this differently?

I am asking because of these:
https://documentation-service.arm.com/static/5f8ea482f86e16515cdbe3c6?token=
https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/vsmc.c?ref_type=heads



>  xen/common/kernel.c                 |  5 ++-
>  xen/include/public/sysctl.h         |  9 ++++
>  xen/include/xen/lib.h               |  1 +
>  10 files changed, 126 insertions(+), 2 deletions(-)
>  create mode 100644 xen/arch/x86/include/asm/test-smc.h
>  create mode 100644 xen/arch/x86/test-smc.c
> 
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 2ef8b4e05422..0f87ffa4affd 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_smc(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..7f7ece589cc2 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_smc(xc_interface *xch, uint32_t tests, uint32_t *result)
> +{
> +    struct xen_sysctl sysctl = {
> +        .cmd = XEN_SYSCTL_test_smc,
> +        .u.smc.tests = tests,
> +    };
> +    int rc = do_sysctl(xch, &sysctl);
> +
> +    if ( !rc )
> +        *result = sysctl.u.smc.results;
> +
> +    return rc;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index f629157086d0..bdd2183a2fd7 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -65,6 +65,7 @@ obj-y += smpboot.o
>  obj-y += spec_ctrl.o
>  obj-y += srat.o
>  obj-y += string.o
> +obj-y += test-smc.o
>  obj-y += time.o
>  obj-y += traps.o
>  obj-y += tsx.o
> diff --git a/xen/arch/x86/include/asm/test-smc.h 
> b/xen/arch/x86/include/asm/test-smc.h
> new file mode 100644
> index 000000000000..18b23dbdbf2d
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/test-smc.h
> @@ -0,0 +1,18 @@
> +#ifndef _ASM_X86_TEST_SMC_H_
> +#define _ASM_X86_TEST_SMC_H_
> +
> +#include <xen/types.h>
> +
> +int test_smc(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 f6b8a3efd752..1f90d30204fe 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-smc.h>
>  
>  /* opt_nosmp: If true, secondary processors are ignored. */
>  static bool __initdata opt_nosmp;
> @@ -1952,6 +1953,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      alternative_branches();
>  
> +    test_smc(XEN_SYSCTL_TEST_SMC_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..77d091f4bd59 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-smc.h>
>  #include <asm/numa.h>
>  #include <xen/nodemask.h>
>  #include <xen/cpu.h>
> @@ -423,6 +424,12 @@ long arch_do_sysctl(
>          break;
>      }
>  
> +    case XEN_SYSCTL_test_smc:
> +        ret = test_smc(sysctl->u.smc.tests, &sysctl->u.smc.results);
> +        if ( !ret && __copy_field_to_guest(u_sysctl, sysctl, u.smc.results) )
> +            ret = -EFAULT;
> +        break;
> +
>      default:
>          ret = -ENOSYS;
>          break;
> diff --git a/xen/arch/x86/test-smc.c b/xen/arch/x86/test-smc.c
> new file mode 100644
> index 000000000000..8916c185d60a
> --- /dev/null
> +++ b/xen/arch/x86/test-smc.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-smc.h>
> +
> +static bool cf_check test_insn_replacement(void)
> +{
> +#define EXPECTED_VALUE 2
> +    unsigned int r = ~EXPECTED_VALUE;
> +
> +    alternative_io("", "mov $" STR(EXPECTED_VALUE) ", %0",
> +                   X86_FEATURE_ALWAYS, "=r"(r));
> +
> +    return r == EXPECTED_VALUE;
> +#undef EXPECTED_VALUE
> +}
> +
> +int test_smc(uint32_t selection, uint32_t *results)
> +{
> +    struct {
> +        unsigned int mask;
> +        bool (*test)(void);
> +        const char *name;
> +    } static const tests[] = {
> +        { XEN_SYSCTL_TEST_SMC_INSN_REPL, &test_insn_replacement,
> +          "alternative instruction replacement" },
> +    };
> +    unsigned int i;
> +
> +    if ( selection & ~XEN_SYSCTL_TEST_SMC_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_SMC);
> +        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..fed7ed0d587f 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_SMC ? 'A' : ' ');
>      }
>      else
>      {
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 9b19679caeb1..94287009387c 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_smc {
> +    uint32_t tests;     /* IN: bitmap with selected tests to execute. */
> +#define XEN_SYSCTL_TEST_SMC_INSN_REPL   (1U << 0)
> +#define XEN_SYSCTL_TEST_SMC_ALL         (XEN_SYSCTL_TEST_SMC_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_smc                      31
>      uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>      union {
>          struct xen_sysctl_readconsole       readconsole;
> @@ -1261,6 +1269,7 @@ struct xen_sysctl {
>          struct xen_sysctl_livepatch_op      livepatch;
>  #if defined(__i386__) || defined(__x86_64__)
>          struct xen_sysctl_cpu_policy        cpu_policy;
> +        struct xen_sysctl_test_smc          smc;
>  #endif
>  
>  #if defined(__arm__) || defined (__aarch64__)
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 1793be5b6b89..1bec6a01b18a 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_SMC                 (1U << 6)
>  extern unsigned int tainted;
>  #define TAINT_STRING_MAX_LEN            20
>  extern char *print_tainted(char *str);
> -- 
> 2.43.0
>