[PATCH v3 0/4] xen/x86: add testing for self modifying code and livepatch

[PATCH v3 0/4] xen/x86: add testing for self modifying code and livepatch

[Roger Pau Monne]

Hello,

The following series contains a misc set of fixes and improvements.

There's one improvement for the hypervisor to set function alignment for
livepatch builds in order to make sure there's always enough space in a
function to be live-patched.

Following patches attempt to introduce a set of tests for self modifying
code, currently one test using the alternatives framework, and one test
for livepatch.

Last patch hooks the newly introduced livepatch test into the gitlab CI
using QEMU and an Alpine Linux dom0:

https://gitlab.com/xen-project/people/royger/xen/-/pipelines/1106713873

Roger Pau Monne (4):
  x86/livepatch: align functions to ensure minimal distance between
    entry points
  xen/x86: introduce self modifying code test
  x86/livepatch: introduce a basic live patch test to gitlab CI
  automation: add x86-64 livepatching test

 automation/gitlab-ci/build.yaml               |  9 +++
 automation/gitlab-ci/test.yaml                |  8 ++
 automation/scripts/build-livepatch            | 27 +++++++
 .../scripts/qemu-alpine-x86_64-livepatch.sh   | 68 ++++++++++++++++
 tools/include/xenctrl.h                       |  2 +
 tools/libs/ctrl/xc_misc.c                     | 14 ++++
 tools/misc/xen-livepatch.c                    | 29 +++++++
 xen/Kconfig                                   | 18 +++++
 xen/Makefile                                  |  3 +
 xen/arch/arm/livepatch.c                      |  2 +
 xen/arch/arm/xen.lds.S                        |  4 +
 xen/arch/x86/Makefile                         |  1 +
 xen/arch/x86/include/asm/test-smoc.h          | 22 ++++++
 xen/arch/x86/livepatch.c                      |  4 +
 xen/arch/x86/setup.c                          |  3 +
 xen/arch/x86/sysctl.c                         |  9 +++
 xen/arch/x86/test/Makefile                    |  3 +
 xen/arch/x86/test/smoc-lp-alt.c               | 24 ++++++
 xen/arch/x86/test/smoc-lp.c                   | 24 ++++++
 xen/arch/x86/test/smoc.c                      | 77 +++++++++++++++++++
 xen/arch/x86/xen.lds.S                        |  4 +
 xen/common/Kconfig                            |  5 +-
 xen/common/kernel.c                           |  5 +-
 xen/include/public/sysctl.h                   | 14 ++++
 xen/include/xen/lib.h                         |  1 +
 25 files changed, 377 insertions(+), 3 deletions(-)
 create mode 100755 automation/scripts/build-livepatch
 create mode 100755 automation/scripts/qemu-alpine-x86_64-livepatch.sh
 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-lp-alt.c
 create mode 100644 xen/arch/x86/test/smoc-lp.c
 create mode 100644 xen/arch/x86/test/smoc.c

-- 
2.43.0

[PATCH v3 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monne]

The minimal function size requirements for livepatch are either 5 bytes (for
jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
that distance between functions entry points is always at least of the minimal
required size for livepatch instruction replacement to be successful.

Add an additional align directive to the linker script, in order to ensure that
the next section placed after the .text.* (per-function sections) is also
aligned to the required boundary, so that the distance of the last function
entry point with the next symbol is also of minimal size.

Note that it's possible for the compiler to end up using a higher function
alignment regardless of the passed value, so this change just make sure that
the minimum required for livepatch to work is present.

The compiler option -falign-functions is not available on at least clang 3.8,
so introduce a Kconfig check for it and make the livepatch option depend on the
compiler supporting the option.

The naming of the option(s) CONFIG_FUNCTION_ALIGNMENT is explicitly not
mentioning CC in preparation for the option also being used by assembly code.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Add Arm side.
 - Align end of section in the linker script to ensure enough padding for the
   last function.
 - Expand commit message and subject.
 - Rework Kconfig options.
 - Check that the compiler supports the option.

Changes since v1:
 - New in this version.
---
 xen/Kconfig              | 18 ++++++++++++++++++
 xen/Makefile             |  3 +++
 xen/arch/arm/livepatch.c |  2 ++
 xen/arch/arm/xen.lds.S   |  4 ++++
 xen/arch/x86/livepatch.c |  4 ++++
 xen/arch/x86/xen.lds.S   |  4 ++++
 xen/common/Kconfig       |  5 ++++-
 7 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 134e6e68ad84..7a61e130771c 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -37,6 +37,24 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
 config CC_SPLIT_SECTIONS
 	bool
 
+# Set function alignment.
+#
+# Allow setting on a boolean basis, and then convert such selection to an
+# integer for the build system and code to consume more easily.
+config CC_HAS_FUNCTION_ALIGNMENT
+	def_bool $(cc-option,-falign-functions=8)
+config FUNCTION_ALIGNMENT_4B
+	bool
+config FUNCTION_ALIGNMENT_8B
+	bool
+config FUNCTION_ALIGNMENT_16B
+	bool
+config FUNCTION_ALIGNMENT
+	int
+	default 16 if FUNCTION_ALIGNMENT_16B
+	default  8 if  FUNCTION_ALIGNMENT_8B
+	default  4 if  FUNCTION_ALIGNMENT_4B
+
 source "arch/$(SRCARCH)/Kconfig"
 
 config DEFCONFIG_LIST
diff --git a/xen/Makefile b/xen/Makefile
index 21832d640225..162cb2bda1c5 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -390,6 +390,9 @@ CFLAGS += -fomit-frame-pointer
 endif
 
 CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
+ifdef CONFIG_FUNCTION_ALIGNMENT
+CFLAGS += -falign-functions=$(CONFIG_FUNCTION_ALIGNMENT)
+endif
 
 CFLAGS += -nostdinc -fno-builtin -fno-common
 CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index bbca1e5a5ed3..aa8ae8c38d28 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -68,6 +68,8 @@ void arch_livepatch_revive(void)
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
 {
+    BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > CONFIG_FUNCTION_ALIGNMENT);
+
     /* If NOPing only do up to maximum amount we can put in the ->opaque. */
     if ( !func->new_addr && (func->new_size > LIVEPATCH_OPAQUE_SIZE ||
          func->new_size % ARCH_PATCH_INSN_SIZE) )
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 59b80d122fd0..afaf1e996b0e 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -44,6 +44,10 @@ SECTIONS
 #ifdef CONFIG_CC_SPLIT_SECTIONS
        *(.text.*)
 #endif
+#ifdef CONFIG_FUNCTION_ALIGNMENT
+       /* Ensure enough distance with the next placed section. */
+       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
+#endif
 
        *(.fixup)
        *(.gnu.warning)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index ee539f001b73..b00ad7120da9 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -109,6 +109,10 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
          */
         uint8_t needed = ARCH_PATCH_INSN_SIZE;
 
+        BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE +
+                     (IS_ENABLED(CONIFG_XEN_IBT) ? ENDBR64_LEN : 0) >
+                     CONFIG_FUNCTION_ALIGNMENT);
+
         if ( is_endbr64(func->old_addr) || is_endbr64_poison(func->old_addr) )
             needed += ENDBR64_LEN;
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 8930e14fc40e..5b3332300d44 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -99,6 +99,10 @@ SECTIONS
        *(.text)
 #ifdef CONFIG_CC_SPLIT_SECTIONS
        *(.text.*)
+#endif
+#ifdef CONFIG_FUNCTION_ALIGNMENT
+       /* Ensure enough distance with the next placed section. */
+       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
 #endif
        *(.text.__x86_indirect_thunk_*)
 
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 310ad4229cdf..c9a21c3c8a07 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -395,8 +395,11 @@ config CRYPTO
 config LIVEPATCH
 	bool "Live patching support"
 	default X86
-	depends on "$(XEN_HAS_BUILD_ID)" = "y"
+	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
 	select CC_SPLIT_SECTIONS
+	select FUNCTION_ALIGNMENT_16B if XEN_IBT
+	select FUNCTION_ALIGNMENT_8B  if X86
+	select FUNCTION_ALIGNMENT_4B
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
 	  binary patches without rebooting. This is primarily used to binarily
-- 
2.43.0

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

[Roger Pau Monne]

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

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);
-- 
2.43.0

[PATCH v3 3/4] x86/livepatch: introduce a basic live patch test to gitlab CI

[Roger Pau Monne]

Introduce a basic livepatch test using the interface to run self modifying
tests.  The introduced test relies on changing a function from returning false
to returning true.

To simplify the burden of keeping a patch that can be provided to
livepatch-build-tools, introduce two new files: one containing the unpatched
test functions, and another one that contains the patched forms of such
functions.  Note that only the former is linked into the Xen image, the latter
is built but the object file is not consumed afterwards.  Do this to assert
that the file containing the patched functions continues to build.

Since livepatch testing will ensure that the functions are not patched previous
the applying the livepatch, allow the livepatch related tests to fail without
tainting the hypervisor.

Note the livepatch tests are not run as part of the self modifying checks
executed during boot, as they would obviously fail.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Clarify comment about xor vs mov instructions for return false/true
   encodings.

Changes since v1:
 - New interface & test.
---
 tools/misc/xen-livepatch.c           | 29 ++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/test-smoc.h |  4 ++++
 xen/arch/x86/setup.c                 |  2 +-
 xen/arch/x86/test/Makefile           |  2 ++
 xen/arch/x86/test/smoc-lp-alt.c      | 24 +++++++++++++++++++++++
 xen/arch/x86/test/smoc-lp.c          | 24 +++++++++++++++++++++++
 xen/arch/x86/test/smoc.c             | 11 ++++++++++-
 xen/include/public/sysctl.h          |  6 +++++-
 8 files changed, 99 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/x86/test/smoc-lp-alt.c
 create mode 100644 xen/arch/x86/test/smoc-lp.c

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 5bf9d9a32b65..4ebd1b4e936d 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -37,6 +37,7 @@ void show_help(void)
             "  replace <name>         apply <name> patch and revert all others.\n"
             "  unload <name>          unload name <name> patch.\n"
             "  load <file> [flags]    upload and apply <file> with name as the <file> name\n"
+            "  test                   execute self modifying code livepatch hypervisor tests\n"
             "    Supported flags:\n"
             "      --nodeps           Disable inter-module buildid dependency check.\n"
             "                         Check only against hypervisor buildid.\n",
@@ -542,6 +543,33 @@ error:
     return rc;
 }
 
+static int test_func(int argc, char *argv[])
+{
+    uint32_t results = 0;
+    int rc;
+
+    if ( argc != 0 )
+    {
+        show_help();
+        return -1;
+    }
+
+    rc = xc_test_smoc(xch, XEN_SYSCTL_TEST_SMOC_LP, &results);
+    if ( rc )
+    {
+        fprintf(stderr, "test operation failed: %s\n", strerror(errno));
+        return -1;
+    }
+    if ( (results & XEN_SYSCTL_TEST_SMOC_LP) != XEN_SYSCTL_TEST_SMOC_LP )
+    {
+        fprintf(stderr, "some tests failed: %#x (expected %#x)\n",
+                results, XEN_SYSCTL_TEST_SMOC_LP);
+        return -1;
+    }
+
+    return 0;
+}
+
 /*
  * These are also functions in action_options that are called in case
  * none of the ones in main_options match.
@@ -554,6 +582,7 @@ struct {
     { "list", list_func },
     { "upload", upload_func },
     { "load", load_func },
+    { "test", test_func },
 };
 
 int main(int argc, char *argv[])
diff --git a/xen/arch/x86/include/asm/test-smoc.h b/xen/arch/x86/include/asm/test-smoc.h
index 2547b925d291..d369cf39687c 100644
--- a/xen/arch/x86/include/asm/test-smoc.h
+++ b/xen/arch/x86/include/asm/test-smoc.h
@@ -5,6 +5,10 @@
 
 int test_smoc(uint32_t selection, uint32_t *results);
 
+#ifdef CONFIG_LIVEPATCH
+bool cf_check test_lp_insn_replacement(void);
+#endif
+
 #endif	/* _ASM_X86_TEST_SMC_H_ */
 
 /*
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index e026b0ea5adc..b97faa7a3e97 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1952,7 +1952,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
 
     alternative_branches();
 
-    test_smoc(XEN_SYSCTL_TEST_SMOC_ALL, NULL);
+    test_smoc(XEN_SYSCTL_TEST_SMOC_ALL & ~XEN_SYSCTL_TEST_SMOC_LP, NULL);
 
     /*
      * NB: when running as a PV shim VCPUOP_up/down is wired to the shim
diff --git a/xen/arch/x86/test/Makefile b/xen/arch/x86/test/Makefile
index b504b8196659..3a5a0a98e4db 100644
--- a/xen/arch/x86/test/Makefile
+++ b/xen/arch/x86/test/Makefile
@@ -1 +1,3 @@
 obj-y += smoc.o
+obj-$(CONFIG_LIVEPATCH)   += smoc-lp.o     # for livepatch testing
+extra-$(CONFIG_LIVEPATCH) += smoc-lp-alt.o
diff --git a/xen/arch/x86/test/smoc-lp-alt.c b/xen/arch/x86/test/smoc-lp-alt.c
new file mode 100644
index 000000000000..d054e9bedc51
--- /dev/null
+++ b/xen/arch/x86/test/smoc-lp-alt.c
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/test-smoc.h>
+
+/*
+ * Interesting case because `return false` can be encoded as `xor %eax, %eax`,
+ * which is shorter than `return true` which is encoded as a `mov $1, %eax`
+ * instruction (based on code generated by GCC 13.2 at -O2), and also shorter
+ * than the replacement `jmp` instruction.
+ */
+bool cf_check test_lp_insn_replacement(void)
+{
+    return true;
+}
+
+/*
+ * 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/test/smoc-lp.c b/xen/arch/x86/test/smoc-lp.c
new file mode 100644
index 000000000000..e972791480b5
--- /dev/null
+++ b/xen/arch/x86/test/smoc-lp.c
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/test-smoc.h>
+
+/*
+ * Interesting case because `return false` can be encoded as `xor %eax, %eax`,
+ * which is shorter than `return true` which is encoded as a `mov $1, %eax`
+ * instruction (based on code generated by GCC 13.2 at -O2), and also shorter
+ * than the replacement `jmp` instruction.
+ */
+bool cf_check test_lp_insn_replacement(void)
+{
+    return false;
+}
+
+/*
+ * 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/test/smoc.c b/xen/arch/x86/test/smoc.c
index e7529f937a5a..8e74d26432ee 100644
--- a/xen/arch/x86/test/smoc.c
+++ b/xen/arch/x86/test/smoc.c
@@ -27,6 +27,10 @@ int test_smoc(uint32_t selection, uint32_t *results)
     } static const tests[] = {
         { XEN_SYSCTL_TEST_SMOC_INSN_REPL, &test_insn_replacement,
           "alternative instruction replacement" },
+#ifdef CONFIG_LIVEPATCH
+        { XEN_SYSCTL_TEST_SMOC_LP_INSN, &test_lp_insn_replacement,
+          "livepatch instruction replacement" },
+#endif
     };
     unsigned int i;
 
@@ -50,7 +54,12 @@ int test_smoc(uint32_t selection, uint32_t *results)
             continue;
         }
 
-        add_taint(TAINT_ERROR_SMOC);
+        /*
+         * livepatch related tests don't taint the hypervisor because we also
+         * want to check the failing case.
+         */
+        if ( !(tests[i].mask & XEN_SYSCTL_TEST_SMOC_LP) )
+            add_taint(TAINT_ERROR_SMOC);
         printk(XENLOG_ERR "%s test failed\n", tests[i].name);
     }
 
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 4b17f1344732..aa4068d6996b 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1205,7 +1205,11 @@ struct xen_sysctl_dt_overlay {
 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)
+#define XEN_SYSCTL_TEST_SMOC_LP_INSN     (1U << 1)
+#define XEN_SYSCTL_TEST_SMOC_ALL         (XEN_SYSCTL_TEST_SMOC_INSN_REPL | \
+                                         XEN_SYSCTL_TEST_SMOC_LP_INSN)
+#define XEN_SYSCTL_TEST_SMOC_LP          (XEN_SYSCTL_TEST_SMOC_LP_INSN)
+
     uint32_t results;   /* OUT: test result: 1 -> success, 0 -> failure. */
 };
 
-- 
2.43.0

[PATCH v3 4/4] automation: add x86-64 livepatching test

[Roger Pau Monne]

Introduce a new gitlab tests for livepatching, using livepatch-build-tools,
which better reflects how downstreams build live patches rather than the
in-tree tests.

The tests applies the dummy in-tree patch example, checks that the patch is
applied correctly and then reverts and unloads it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Split livepatch build into a separate script.
---
 automation/gitlab-ci/build.yaml               |  9 +++
 automation/gitlab-ci/test.yaml                |  8 +++
 automation/scripts/build-livepatch            | 27 ++++++++
 .../scripts/qemu-alpine-x86_64-livepatch.sh   | 68 +++++++++++++++++++
 4 files changed, 112 insertions(+)
 create mode 100755 automation/scripts/build-livepatch
 create mode 100755 automation/scripts/qemu-alpine-x86_64-livepatch.sh

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index 32af30ccedc9..d770bffb845e 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -358,6 +358,15 @@ alpine-3.18-gcc-debug:
   variables:
     CONTAINER: alpine:3.18
 
+alpine-3.18-gcc-livepatch:
+  extends: .gcc-x86-64-build
+  script:
+    - ./automation/scripts/build-livepatch 2>&1 | tee build.log
+  variables:
+    CONTAINER: alpine:3.18
+    EXTRA_XEN_CONFIG: |
+      CONFIG_LIVEPATCH=y
+
 debian-stretch-gcc-debug:
   extends: .gcc-x86-64-build-debug
   variables:
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 6aabdb9d156f..58a90be5ed0e 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -459,3 +459,11 @@ qemu-smoke-ppc64le-powernv9-gcc:
   needs:
     - qemu-system-ppc64-8.1.0-ppc64-export
     - debian-bullseye-gcc-ppc64le-debug
+
+qemu-alpine-x86_64-gcc-livepatch:
+  extends: .qemu-x86-64
+  script:
+    - ./automation/scripts/qemu-alpine-x86_64-livepatch.sh 2>&1 | tee ${LOGFILE}
+  needs:
+    - *x86-64-test-needs
+    - alpine-3.18-gcc-livepatch
diff --git a/automation/scripts/build-livepatch b/automation/scripts/build-livepatch
new file mode 100755
index 000000000000..ac86b17ae5e4
--- /dev/null
+++ b/automation/scripts/build-livepatch
@@ -0,0 +1,27 @@
+#!/bin/bash -ex
+
+# Do a regular build first
+./automation/scripts/build
+
+# Build a test livepatch using livepatch-build-tools.
+
+if [[ "$XEN_TARGET_ARCH" != "x86_64" ]]; then
+    exit 1
+fi
+
+BASE=xen/arch/x86/test/smoc-lp.c
+ALT=xen/arch/x86/test/smoc-lp-alt.c
+
+[[ -f $BASE && -f $ALT ]]
+
+# git diff --no-index returns 0 if no differences, otherwise 1.
+git diff --no-index --output=test.patch $BASE $ALT && exit 1
+
+BUILDID=$(readelf -Wn xen/xen-syms | sed -n -e 's/^.*Build ID: //p')
+
+git clone https://xenbits.xen.org/git-http/livepatch-build-tools.git
+cd livepatch-build-tools
+make
+./livepatch-build -s ../ -p ../test.patch -o out -c ../xen/.config \
+    --depends $BUILDID --xen-depends $BUILDID
+cp out/test.livepatch ../binaries/test.livepatch
diff --git a/automation/scripts/qemu-alpine-x86_64-livepatch.sh b/automation/scripts/qemu-alpine-x86_64-livepatch.sh
new file mode 100755
index 000000000000..da478cac4376
--- /dev/null
+++ b/automation/scripts/qemu-alpine-x86_64-livepatch.sh
@@ -0,0 +1,68 @@
+#!/bin/bash
+
+set -ex
+
+cd binaries
+# initrd.tar.gz is Dom0 rootfs
+mkdir -p rootfs
+cd rootfs
+tar xvzf ../initrd.tar.gz
+mkdir proc
+mkdir run
+mkdir srv
+mkdir sys
+rm var/run
+cp -ar ../dist/install/* .
+cp ../test.livepatch ./root/
+cat << "EOF" >> etc/local.d/xen-lp.start
+#!/bin/bash
+
+set -ex
+
+trap poweroff EXIT
+
+export LD_LIBRARY_PATH=/usr/local/lib
+
+xen-livepatch test && exit 1 || true
+
+xen-livepatch load /root/test.livepatch
+
+# Cannot fail now
+xen-livepatch test
+
+xen-livepatch revert test
+xen-livepatch unload test
+
+xen-livepatch test && exit 1 || true
+
+echo "SUCCESS"
+EOF
+chmod +x etc/local.d/xen-lp.start
+echo "rc_verbose=yes" >> etc/rc.conf
+# rebuild Dom0 rootfs
+find . |cpio -H newc -o|gzip > ../xen-rootfs.cpio.gz
+cd ../..
+
+cat >> binaries/pxelinux.0 << EOF
+#!ipxe
+
+kernel xen console=com1 console_timestamps=boot
+module bzImage console=hvc0
+module xen-rootfs.cpio.gz
+boot
+EOF
+
+# Run the test
+rm -f smoke.serial
+timeout -k 1 360 \
+qemu-system-x86_64 \
+    -cpu qemu64,+svm \
+    -m 2G -smp 2 \
+    -monitor none -serial stdio \
+    -nographic \
+    -device virtio-net-pci,netdev=n0 \
+    -netdev user,id=n0,tftp=binaries,bootfile=/pxelinux.0 |& \
+        tee smoke.serial | sed 's/\r//'
+
+grep -q "SUCCESS" smoke.serial
+exit 0
-- 
2.43.0

Re: [PATCH v3 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich]

On 14.12.2023 11:17, Roger Pau Monne wrote:
> The minimal function size requirements for livepatch are either 5 bytes (for
> jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
> that distance between functions entry points is always at least of the minimal
> required size for livepatch instruction replacement to be successful.
> 
> Add an additional align directive to the linker script, in order to ensure that
> the next section placed after the .text.* (per-function sections) is also
> aligned to the required boundary, so that the distance of the last function
> entry point with the next symbol is also of minimal size.
> 
> Note that it's possible for the compiler to end up using a higher function
> alignment regardless of the passed value, so this change just make sure that
> the minimum required for livepatch to work is present.

That's a possibility which we don't need to be concerned about. Yet isn't it
also possible that we override a larger, deemed better (e.g. performance-wise)
value? I'm somewhat concerned of that case. IOW is -falign-functions= really
the right option to use here? (There may not be one which we would actually
prefer to use.) Specifically -falign-functions (without a value, i.e. using a
machine dependent default) is implied by -O2 and -O3, as per 13.2 gcc doc.

> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -37,6 +37,24 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
>  config CC_SPLIT_SECTIONS
>  	bool
>  
> +# Set function alignment.
> +#
> +# Allow setting on a boolean basis, and then convert such selection to an
> +# integer for the build system and code to consume more easily.
> +config CC_HAS_FUNCTION_ALIGNMENT
> +	def_bool $(cc-option,-falign-functions=8)

How does this check make sure 4- or 16-byte alignment are also accepted as
valid? (Requesting 8-byte alignment would be at least bogus on e.g. IA-64.)

> +config FUNCTION_ALIGNMENT_4B
> +	bool
> +config FUNCTION_ALIGNMENT_8B
> +	bool
> +config FUNCTION_ALIGNMENT_16B
> +	bool
> +config FUNCTION_ALIGNMENT
> +	int
> +	default 16 if FUNCTION_ALIGNMENT_16B
> +	default  8 if  FUNCTION_ALIGNMENT_8B
> +	default  4 if  FUNCTION_ALIGNMENT_4B

While for the immediate purpose here no "depends on CC_HAS_FUNCTION_ALIGNMENT"
is okay, I still wonder if it wouldn't better be added in case further
"select"s appear.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -390,6 +390,9 @@ CFLAGS += -fomit-frame-pointer
>  endif
>  
>  CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
> +ifdef CONFIG_FUNCTION_ALIGNMENT

... e.g. this meaningless? Or is the lack of a default value leading to
the option remaining undefined (rather than assigning some "default"
default, e.g. 0)?

> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -44,6 +44,10 @@ SECTIONS
>  #ifdef CONFIG_CC_SPLIT_SECTIONS
>         *(.text.*)
>  #endif
> +#ifdef CONFIG_FUNCTION_ALIGNMENT
> +       /* Ensure enough distance with the next placed section. */
> +       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
> +#endif
>  
>         *(.fixup)

Seeing .fixup nicely in context - can't that (and other specialized
sections containing code) also be patched?

Jan

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

[Jan Beulich]

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

Re: [PATCH v3 3/4] x86/livepatch: introduce a basic live patch test to gitlab CI

[Jan Beulich]

On 14.12.2023 11:17, Roger Pau Monne wrote:
> Introduce a basic livepatch test using the interface to run self modifying
> tests.  The introduced test relies on changing a function from returning false
> to returning true.
> 
> To simplify the burden of keeping a patch that can be provided to
> livepatch-build-tools, introduce two new files: one containing the unpatched
> test functions, and another one that contains the patched forms of such
> functions.  Note that only the former is linked into the Xen image, the latter
> is built but the object file is not consumed afterwards.  Do this to assert
> that the file containing the patched functions continues to build.
> 
> Since livepatch testing will ensure that the functions are not patched previous
> the applying the livepatch, allow the livepatch related tests to fail without
> tainting the hypervisor.
> 
> Note the livepatch tests are not run as part of the self modifying checks
> executed during boot, as they would obviously fail.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

I'm still not overly happy with those comments though, but yes - -O2 is likely
the only thing that matters in practice, until such time that we have a way to
allow use of -Os without any Makefile changes or other custom overrides.

Jan

Re: [PATCH v3 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monné]

On Thu, Dec 14, 2023 at 12:18:13PM +0100, Jan Beulich wrote:
> On 14.12.2023 11:17, Roger Pau Monne wrote:
> > The minimal function size requirements for livepatch are either 5 bytes (for
> > jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
> > that distance between functions entry points is always at least of the minimal
> > required size for livepatch instruction replacement to be successful.
> > 
> > Add an additional align directive to the linker script, in order to ensure that
> > the next section placed after the .text.* (per-function sections) is also
> > aligned to the required boundary, so that the distance of the last function
> > entry point with the next symbol is also of minimal size.
> > 
> > Note that it's possible for the compiler to end up using a higher function
> > alignment regardless of the passed value, so this change just make sure that
> > the minimum required for livepatch to work is present.
> 
> That's a possibility which we don't need to be concerned about. Yet isn't it
> also possible that we override a larger, deemed better (e.g. performance-wise)
> value?

I'm kind of confused, the compiler will always choose the higher
alignment.  For example non-debug builds on my box end up with
function sections aligned to 16 instead of the 8 passed in the
-falign-functions= parameter:

$ clang -MMD -MP -MF arch/x86/.traps.o.d -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -Werror=unknown-warning-option -O2 -fomit-frame-pointer -falign-functions=8 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith -Wdeclaration-after-statement -Wvla -pipe -D__XEN__ -include ./include/xen/config.h -ffunction-sections -fdata-sections -mretpoline-external-thunk  -I./include -I./arch/x86/include -I./arch/x86/include/generated -I./arch/x86/include/asm/mach-generic -I./arch/x86/include/asm/mach-default -DXEN_IMG_OFFSET=0x200000 -msoft-float -fno-pie -fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables -Wnested-externs -DHAVE_AS_VMX -DHAVE_AS_SSE4_2 -DHAVE_AS_EPT -DHAVE_AS_RDRAND -DHAVE_AS_FSGSBASE -DHAVE_AS_XSAVEOPT -DHAVE_AS_RDSEED -DHAVE_AS_CLAC_STAC -DHAVE_AS_CLWB -DHAVE_AS_QUOTED_SYM -DHAVE_AS_INVPCID -DHAVE_AS_MOVDIR -DHAVE_AS_ENQCMD -mno-red-zone -fpic -mno-mmx -mno-sse -mskip-rax-setup -fcf-protection=none -Wa,-I./include -Wa,-I./include '-D__OBJECT_LABEL__=arch/x86/traps.o'   -DXEN_BUILD_EFI -DBUILD_ID_EFI -c arch/x86/traps.c -o arch/x86/.traps.o.tmp -MQ arch/x86/traps.o

$ readelf -WS xen/arch/x86/traps.o

There are 107 section headers, starting at offset 0xe2e0:

Section Headers:
  [Nr] Name              Type            Addr             Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        0000000000000000 000040 000000 00  AX  0   0  4
  [ 2] .text.show_code   PROGBITS        0000000000000000 000040 000287 00  AX  0   0 16
  [ 3] .rela.text.show_code RELA            0000000000000000 008520 000450 18   I 104   2  8
  [ 4] .altinstructions  PROGBITS        0000000000000000 0002c7 00024c 00   A  0   0  1
  [ 5] .rela.altinstructions RELA            0000000000000000 008970 0007e0 18   I 104   4  8
  [ 6] .discard          PROGBITS        0000000000000000 000513 000054 00   A  0   0  1
  [ 7] .altinstr_replacement PROGBITS        0000000000000000 000567 000018 00  AX  0   0  1
  [ 8] .ex_table         PROGBITS        0000000000000000 000580 000028 00   A  0   0  4
  [ 9] .rela.ex_table    RELA            0000000000000000 009150 0000f0 18   I 104   8  8
  [10] .text.get_stack_trace_bottom PROGBITS        0000000000000000 0005b0 000046 00  AX  0   0 16
  [11] .text.get_stack_dump_bottom PROGBITS        0000000000000000 000600 00003d 00  AX  0   0 16
  [12] .text.show_stack_overflow PROGBITS        0000000000000000 000640 000158 00  AX  0   0 16
[...]

> I'm somewhat concerned of that case. IOW is -falign-functions= really
> the right option to use here? (There may not be one which we would actually
> prefer to use.) Specifically -falign-functions (without a value, i.e. using a
> machine dependent default) is implied by -O2 and -O3, as per 13.2 gcc doc.

Right, and that still works fine AFAICT, see how in the example above
functions ended up aligned to 16 even when -falign-functions=8 was
provided on the command line.

> > --- a/xen/Kconfig
> > +++ b/xen/Kconfig
> > @@ -37,6 +37,24 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
> >  config CC_SPLIT_SECTIONS
> >  	bool
> >  
> > +# Set function alignment.
> > +#
> > +# Allow setting on a boolean basis, and then convert such selection to an
> > +# integer for the build system and code to consume more easily.
> > +config CC_HAS_FUNCTION_ALIGNMENT
> > +	def_bool $(cc-option,-falign-functions=8)
> 
> How does this check make sure 4- or 16-byte alignment are also accepted as
> valid? (Requesting 8-byte alignment would be at least bogus on e.g. IA-64.)

I was confused and expected the compiler to round up to the next
supported value by the arch, but that doesn't seem to be written down
in the GCC manual at least.

One option would be to simply test for -falign-functions with no
specific alignment, and leave Kconfig code to set a suitable value on
a per-arch basis.

> > +config FUNCTION_ALIGNMENT_4B
> > +	bool
> > +config FUNCTION_ALIGNMENT_8B
> > +	bool
> > +config FUNCTION_ALIGNMENT_16B
> > +	bool
> > +config FUNCTION_ALIGNMENT
> > +	int
> > +	default 16 if FUNCTION_ALIGNMENT_16B
> > +	default  8 if  FUNCTION_ALIGNMENT_8B
> > +	default  4 if  FUNCTION_ALIGNMENT_4B
> 
> While for the immediate purpose here no "depends on CC_HAS_FUNCTION_ALIGNMENT"
> is okay, I still wonder if it wouldn't better be added in case further
> "select"s appear.

Will adjust in next version.

> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -390,6 +390,9 @@ CFLAGS += -fomit-frame-pointer
> >  endif
> >  
> >  CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
> > +ifdef CONFIG_FUNCTION_ALIGNMENT
> 
> ... e.g. this meaningless? Or is the lack of a default value leading to
> the option remaining undefined (rather than assigning some "default"
> default, e.g. 0)?

If no default value the option remain undefined, and -falign-functions
is not passed on the compiler command line.  This is required in case
the compiler doesn't support -falign-functions.

> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -44,6 +44,10 @@ SECTIONS
> >  #ifdef CONFIG_CC_SPLIT_SECTIONS
> >         *(.text.*)
> >  #endif
> > +#ifdef CONFIG_FUNCTION_ALIGNMENT
> > +       /* Ensure enough distance with the next placed section. */
> > +       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
> > +#endif
> >  
> >         *(.fixup)
> 
> Seeing .fixup nicely in context - can't that (and other specialized
> sections containing code) also be patched?

The current livepatch-build-tools logic doesn't seem to detect changes
to .fixup, so I've added this to my list of stuff to fix for
livepatch.  I do see the livepatch code in the hypervisor has support
for loading extra .ex_table sections, so I assume at some point it was
considered to add support for .fixup.  My current thinking is that
.fixup itself won't be changed, and that instead a new .fixup will be
loaded, and the newly loaded .ex_table will reference such section.

Thanks, Roger.

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

[Roger Pau Monné]

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.

Re: [PATCH v3 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich]

On 14.12.2023 14:37, Roger Pau Monné wrote:
> On Thu, Dec 14, 2023 at 12:18:13PM +0100, Jan Beulich wrote:
>> On 14.12.2023 11:17, Roger Pau Monne wrote:
>>> The minimal function size requirements for livepatch are either 5 bytes (for
>>> jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
>>> that distance between functions entry points is always at least of the minimal
>>> required size for livepatch instruction replacement to be successful.
>>>
>>> Add an additional align directive to the linker script, in order to ensure that
>>> the next section placed after the .text.* (per-function sections) is also
>>> aligned to the required boundary, so that the distance of the last function
>>> entry point with the next symbol is also of minimal size.
>>>
>>> Note that it's possible for the compiler to end up using a higher function
>>> alignment regardless of the passed value, so this change just make sure that
>>> the minimum required for livepatch to work is present.
>>
>> That's a possibility which we don't need to be concerned about. Yet isn't it
>> also possible that we override a larger, deemed better (e.g. performance-wise)
>> value?
> 
> I'm kind of confused, the compiler will always choose the higher
> alignment.

Will it? Before writing the reply I went through gcc's respective doc
section, without finding such a guarantee.

>  For example non-debug builds on my box end up with
> function sections aligned to 16 instead of the 8 passed in the
> -falign-functions= parameter:
> 
> $ clang -MMD -MP -MF arch/x86/.traps.o.d -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -Werror=unknown-warning-option -O2 -fomit-frame-pointer -falign-functions=8 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith -Wdeclaration-after-statement -Wvla -pipe -D__XEN__ -include ./include/xen/config.h -ffunction-sections -fdata-sections -mretpoline-external-thunk  -I./include -I./arch/x86/include -I./arch/x86/include/generated -I./arch/x86/include/asm/mach-generic -I./arch/x86/include/asm/mach-default -DXEN_IMG_OFFSET=0x200000 -msoft-float -fno-pie -fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables -Wnested-externs -DHAVE_AS_VMX -DHAVE_AS_SSE4_2 -DHAVE_AS_EPT -DHAVE_AS_RDRAND -DHAVE_AS_FSGSBASE -DHAVE_AS_XSAVEOPT -DHAVE_AS_RDSEED -DHAVE_AS_CLAC_STAC -DHAVE_AS_CLWB -DHAVE_AS_QUOTED_SYM -DHAVE_AS_INVPCID -DHAVE_AS_MOVDIR -DHAVE_AS_ENQCMD -mno-red-zone -fpic -mno-mmx -mno-sse -mskip-rax-setup -fcf-protection=none -Wa,-I./include -Wa,-I./include '-D__OBJECT_LABEL__=arch/x86/traps.o'   -DXEN_BUILD_EFI -DBUILD_ID_EFI -c arch/x86/traps.c -o arch/x86/.traps.o.tmp -MQ arch/x86/traps.o
> 
> $ readelf -WS xen/arch/x86/traps.o
> 
> There are 107 section headers, starting at offset 0xe2e0:
> 
> Section Headers:
>   [Nr] Name              Type            Addr             Off    Size   ES Flg Lk Inf Al
>   [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
>   [ 1] .text             PROGBITS        0000000000000000 000040 000000 00  AX  0   0  4
>   [ 2] .text.show_code   PROGBITS        0000000000000000 000040 000287 00  AX  0   0 16
>   [ 3] .rela.text.show_code RELA            0000000000000000 008520 000450 18   I 104   2  8
>   [ 4] .altinstructions  PROGBITS        0000000000000000 0002c7 00024c 00   A  0   0  1
>   [ 5] .rela.altinstructions RELA            0000000000000000 008970 0007e0 18   I 104   4  8
>   [ 6] .discard          PROGBITS        0000000000000000 000513 000054 00   A  0   0  1
>   [ 7] .altinstr_replacement PROGBITS        0000000000000000 000567 000018 00  AX  0   0  1
>   [ 8] .ex_table         PROGBITS        0000000000000000 000580 000028 00   A  0   0  4
>   [ 9] .rela.ex_table    RELA            0000000000000000 009150 0000f0 18   I 104   8  8
>   [10] .text.get_stack_trace_bottom PROGBITS        0000000000000000 0005b0 000046 00  AX  0   0 16
>   [11] .text.get_stack_dump_bottom PROGBITS        0000000000000000 000600 00003d 00  AX  0   0 16
>   [12] .text.show_stack_overflow PROGBITS        0000000000000000 000640 000158 00  AX  0   0 16
> [...]
> 
>> I'm somewhat concerned of that case. IOW is -falign-functions= really
>> the right option to use here? (There may not be one which we would actually
>> prefer to use.) Specifically -falign-functions (without a value, i.e. using a
>> machine dependent default) is implied by -O2 and -O3, as per 13.2 gcc doc.
> 
> Right, and that still works fine AFAICT, see how in the example above
> functions ended up aligned to 16 even when -falign-functions=8 was
> provided on the command line.
> 
>>> --- a/xen/Kconfig
>>> +++ b/xen/Kconfig
>>> @@ -37,6 +37,24 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
>>>  config CC_SPLIT_SECTIONS
>>>  	bool
>>>  
>>> +# Set function alignment.
>>> +#
>>> +# Allow setting on a boolean basis, and then convert such selection to an
>>> +# integer for the build system and code to consume more easily.
>>> +config CC_HAS_FUNCTION_ALIGNMENT
>>> +	def_bool $(cc-option,-falign-functions=8)
>>
>> How does this check make sure 4- or 16-byte alignment are also accepted as
>> valid? (Requesting 8-byte alignment would be at least bogus on e.g. IA-64.)
> 
> I was confused and expected the compiler to round up to the next
> supported value by the arch, but that doesn't seem to be written down
> in the GCC manual at least.
> 
> One option would be to simply test for -falign-functions with no
> specific alignment, and leave Kconfig code to set a suitable value on
> a per-arch basis.

Perhaps; this ...

>>> +config FUNCTION_ALIGNMENT_4B
>>> +	bool
>>> +config FUNCTION_ALIGNMENT_8B
>>> +	bool
>>> +config FUNCTION_ALIGNMENT_16B
>>> +	bool
>>> +config FUNCTION_ALIGNMENT
>>> +	int
>>> +	default 16 if FUNCTION_ALIGNMENT_16B
>>> +	default  8 if  FUNCTION_ALIGNMENT_8B
>>> +	default  4 if  FUNCTION_ALIGNMENT_4B

... makes sure the highest alignment ever selected from anywhere will be
used (should an arch need to select any of these).

>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -390,6 +390,9 @@ CFLAGS += -fomit-frame-pointer
>>>  endif
>>>  
>>>  CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
>>> +ifdef CONFIG_FUNCTION_ALIGNMENT
>>
>> ... e.g. this meaningless? Or is the lack of a default value leading to
>> the option remaining undefined (rather than assigning some "default"
>> default, e.g. 0)?
> 
> If no default value the option remain undefined, and -falign-functions
> is not passed on the compiler command line.  This is required in case
> the compiler doesn't support -falign-functions.

Oh, sorry, I meant to delete that comment, which really was only the 2nd
half of something I had before actually deciding to try it out (see the
unmotivated ... at the beginning).

>>> --- a/xen/arch/arm/xen.lds.S
>>> +++ b/xen/arch/arm/xen.lds.S
>>> @@ -44,6 +44,10 @@ SECTIONS
>>>  #ifdef CONFIG_CC_SPLIT_SECTIONS
>>>         *(.text.*)
>>>  #endif
>>> +#ifdef CONFIG_FUNCTION_ALIGNMENT
>>> +       /* Ensure enough distance with the next placed section. */
>>> +       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
>>> +#endif
>>>  
>>>         *(.fixup)
>>
>> Seeing .fixup nicely in context - can't that (and other specialized
>> sections containing code) also be patched?
> 
> The current livepatch-build-tools logic doesn't seem to detect changes
> to .fixup, so I've added this to my list of stuff to fix for
> livepatch.  I do see the livepatch code in the hypervisor has support
> for loading extra .ex_table sections, so I assume at some point it was
> considered to add support for .fixup.  My current thinking is that
> .fixup itself won't be changed, and that instead a new .fixup will be
> loaded, and the newly loaded .ex_table will reference such section.

Hmm, yes, that's a fair explanation for .fixup not needing special
handling. Yet then I would still be worried of other snippets, e.g.
stuff ending up in e.g. .text.cold or .text.unlikely. Would they all
also be dealt with in similar ways?

Jan

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

[Jan Beulich]

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

Re: [PATCH v3 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monné]

On Thu, Dec 14, 2023 at 02:53:13PM +0100, Jan Beulich wrote:
> On 14.12.2023 14:37, Roger Pau Monné wrote:
> > On Thu, Dec 14, 2023 at 12:18:13PM +0100, Jan Beulich wrote:
> >> On 14.12.2023 11:17, Roger Pau Monne wrote:
> >>> The minimal function size requirements for livepatch are either 5 bytes (for
> >>> jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
> >>> that distance between functions entry points is always at least of the minimal
> >>> required size for livepatch instruction replacement to be successful.
> >>>
> >>> Add an additional align directive to the linker script, in order to ensure that
> >>> the next section placed after the .text.* (per-function sections) is also
> >>> aligned to the required boundary, so that the distance of the last function
> >>> entry point with the next symbol is also of minimal size.
> >>>
> >>> Note that it's possible for the compiler to end up using a higher function
> >>> alignment regardless of the passed value, so this change just make sure that
> >>> the minimum required for livepatch to work is present.
> >>
> >> That's a possibility which we don't need to be concerned about. Yet isn't it
> >> also possible that we override a larger, deemed better (e.g. performance-wise)
> >> value?
> > 
> > I'm kind of confused, the compiler will always choose the higher
> > alignment.
> 
> Will it? Before writing the reply I went through gcc's respective doc
> section, without finding such a guarantee.

Hm, yes, checked with godbolt now and GCC behaves the opposite of
clang, and will always attempt to honor the alignment passed in
falign-functions.

Maybe for release builds we should select a 16b alignment on x86?

Arm seems to use 4 byte alignment by default when using -O2.

> >  For example non-debug builds on my box end up with
> > function sections aligned to 16 instead of the 8 passed in the
> > -falign-functions= parameter:
> > 
> > $ clang -MMD -MP -MF arch/x86/.traps.o.d -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -Werror=unknown-warning-option -O2 -fomit-frame-pointer -falign-functions=8 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith -Wdeclaration-after-statement -Wvla -pipe -D__XEN__ -include ./include/xen/config.h -ffunction-sections -fdata-sections -mretpoline-external-thunk  -I./include -I./arch/x86/include -I./arch/x86/include/generated -I./arch/x86/include/asm/mach-generic -I./arch/x86/include/asm/mach-default -DXEN_IMG_OFFSET=0x200000 -msoft-float -fno-pie -fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables -Wnested-externs -DHAVE_AS_VMX -DHAVE_AS_SSE4_2 -DHAVE_AS_EPT -DHAVE_AS_RDRAND -DHAVE_AS_FSGSBASE -DHAVE_AS_XSAVEOPT -DHAVE_AS_RDSEED -DHAVE_AS_CLAC_STAC -DHAVE_AS_CLWB -DHAVE_AS_QUOTED_SYM -DHAVE_AS_INVPCID -DHAVE_AS_MOVDIR -DHAVE_AS_ENQCMD -mno-red-zone -fpic -mno-mmx -mno-sse -mskip-rax-setup -fcf-protection=none -Wa,-I./include -Wa,-I./include '-D__OBJECT_LABEL__=arch/x86/traps.o'   -DXEN_BUILD_EFI -DBUILD_ID_EFI -c arch/x86/traps.c -o arch/x86/.traps.o.tmp -MQ arch/x86/traps.o
> > 
> > $ readelf -WS xen/arch/x86/traps.o
> > 
> > There are 107 section headers, starting at offset 0xe2e0:
> > 
> > Section Headers:
> >   [Nr] Name              Type            Addr             Off    Size   ES Flg Lk Inf Al
> >   [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
> >   [ 1] .text             PROGBITS        0000000000000000 000040 000000 00  AX  0   0  4
> >   [ 2] .text.show_code   PROGBITS        0000000000000000 000040 000287 00  AX  0   0 16
> >   [ 3] .rela.text.show_code RELA            0000000000000000 008520 000450 18   I 104   2  8
> >   [ 4] .altinstructions  PROGBITS        0000000000000000 0002c7 00024c 00   A  0   0  1
> >   [ 5] .rela.altinstructions RELA            0000000000000000 008970 0007e0 18   I 104   4  8
> >   [ 6] .discard          PROGBITS        0000000000000000 000513 000054 00   A  0   0  1
> >   [ 7] .altinstr_replacement PROGBITS        0000000000000000 000567 000018 00  AX  0   0  1
> >   [ 8] .ex_table         PROGBITS        0000000000000000 000580 000028 00   A  0   0  4
> >   [ 9] .rela.ex_table    RELA            0000000000000000 009150 0000f0 18   I 104   8  8
> >   [10] .text.get_stack_trace_bottom PROGBITS        0000000000000000 0005b0 000046 00  AX  0   0 16
> >   [11] .text.get_stack_dump_bottom PROGBITS        0000000000000000 000600 00003d 00  AX  0   0 16
> >   [12] .text.show_stack_overflow PROGBITS        0000000000000000 000640 000158 00  AX  0   0 16
> > [...]
> > 
> >> I'm somewhat concerned of that case. IOW is -falign-functions= really
> >> the right option to use here? (There may not be one which we would actually
> >> prefer to use.) Specifically -falign-functions (without a value, i.e. using a
> >> machine dependent default) is implied by -O2 and -O3, as per 13.2 gcc doc.
> > 
> > Right, and that still works fine AFAICT, see how in the example above
> > functions ended up aligned to 16 even when -falign-functions=8 was
> > provided on the command line.
> > 
> >>> --- a/xen/Kconfig
> >>> +++ b/xen/Kconfig
> >>> @@ -37,6 +37,24 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
> >>>  config CC_SPLIT_SECTIONS
> >>>  	bool
> >>>  
> >>> +# Set function alignment.
> >>> +#
> >>> +# Allow setting on a boolean basis, and then convert such selection to an
> >>> +# integer for the build system and code to consume more easily.
> >>> +config CC_HAS_FUNCTION_ALIGNMENT
> >>> +	def_bool $(cc-option,-falign-functions=8)
> >>
> >> How does this check make sure 4- or 16-byte alignment are also accepted as
> >> valid? (Requesting 8-byte alignment would be at least bogus on e.g. IA-64.)
> > 
> > I was confused and expected the compiler to round up to the next
> > supported value by the arch, but that doesn't seem to be written down
> > in the GCC manual at least.
> > 
> > One option would be to simply test for -falign-functions with no
> > specific alignment, and leave Kconfig code to set a suitable value on
> > a per-arch basis.
> 
> Perhaps; this ...
> 
> >>> +config FUNCTION_ALIGNMENT_4B
> >>> +	bool
> >>> +config FUNCTION_ALIGNMENT_8B
> >>> +	bool
> >>> +config FUNCTION_ALIGNMENT_16B
> >>> +	bool
> >>> +config FUNCTION_ALIGNMENT
> >>> +	int
> >>> +	default 16 if FUNCTION_ALIGNMENT_16B
> >>> +	default  8 if  FUNCTION_ALIGNMENT_8B
> >>> +	default  4 if  FUNCTION_ALIGNMENT_4B
> 
> ... makes sure the highest alignment ever selected from anywhere will be
> used (should an arch need to select any of these).

That's the intention, that the highest selected FUNCTION_ALIGNMENT_X
ends up defining the value of FUNCTION_ALIGNMENT.

> >>> --- a/xen/arch/arm/xen.lds.S
> >>> +++ b/xen/arch/arm/xen.lds.S
> >>> @@ -44,6 +44,10 @@ SECTIONS
> >>>  #ifdef CONFIG_CC_SPLIT_SECTIONS
> >>>         *(.text.*)
> >>>  #endif
> >>> +#ifdef CONFIG_FUNCTION_ALIGNMENT
> >>> +       /* Ensure enough distance with the next placed section. */
> >>> +       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
> >>> +#endif
> >>>  
> >>>         *(.fixup)
> >>
> >> Seeing .fixup nicely in context - can't that (and other specialized
> >> sections containing code) also be patched?
> > 
> > The current livepatch-build-tools logic doesn't seem to detect changes
> > to .fixup, so I've added this to my list of stuff to fix for
> > livepatch.  I do see the livepatch code in the hypervisor has support
> > for loading extra .ex_table sections, so I assume at some point it was
> > considered to add support for .fixup.  My current thinking is that
> > .fixup itself won't be changed, and that instead a new .fixup will be
> > loaded, and the newly loaded .ex_table will reference such section.
> 
> Hmm, yes, that's a fair explanation for .fixup not needing special
> handling. Yet then I would still be worried of other snippets, e.g.
> stuff ending up in e.g. .text.cold or .text.unlikely. Would they all
> also be dealt with in similar ways?

Yes, livepatch-build-tools requires functions to be in separate
sections in order to be able to patch them.  Contents of .text.cold or
.text.unlikely won't be detected by the patch generation scripts as
far as I'm aware (like it doesn't detect changes to .fixup currently).

There might be ways to create patches for those manually by building a
custom livepatch payload, but in that case the function size is left
to the user to calculate.

Thanks, Roger.

Re: [PATCH v3 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich]

On 14.12.2023 16:20, Roger Pau Monné wrote:
> On Thu, Dec 14, 2023 at 02:53:13PM +0100, Jan Beulich wrote:
>> On 14.12.2023 14:37, Roger Pau Monné wrote:
>>> On Thu, Dec 14, 2023 at 12:18:13PM +0100, Jan Beulich wrote:
>>>> On 14.12.2023 11:17, Roger Pau Monne wrote:
>>>>> The minimal function size requirements for livepatch are either 5 bytes (for
>>>>> jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
>>>>> that distance between functions entry points is always at least of the minimal
>>>>> required size for livepatch instruction replacement to be successful.
>>>>>
>>>>> Add an additional align directive to the linker script, in order to ensure that
>>>>> the next section placed after the .text.* (per-function sections) is also
>>>>> aligned to the required boundary, so that the distance of the last function
>>>>> entry point with the next symbol is also of minimal size.
>>>>>
>>>>> Note that it's possible for the compiler to end up using a higher function
>>>>> alignment regardless of the passed value, so this change just make sure that
>>>>> the minimum required for livepatch to work is present.
>>>>
>>>> That's a possibility which we don't need to be concerned about. Yet isn't it
>>>> also possible that we override a larger, deemed better (e.g. performance-wise)
>>>> value?
>>>
>>> I'm kind of confused, the compiler will always choose the higher
>>> alignment.
>>
>> Will it? Before writing the reply I went through gcc's respective doc
>> section, without finding such a guarantee.
> 
> Hm, yes, checked with godbolt now and GCC behaves the opposite of
> clang, and will always attempt to honor the alignment passed in
> falign-functions.
> 
> Maybe for release builds we should select a 16b alignment on x86?

Might make sense, yes. Iirc there was a time where 16-byte alignment was
the default for functions in gcc.

Jan

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

Re: [PATCH v3 4/4] automation: add x86-64 livepatching test

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4972 bytes --]

On Thu, 13 Dec 2023, Roger Pau Monne wrote:
> Introduce a new gitlab tests for livepatching, using livepatch-build-tools,
> which better reflects how downstreams build live patches rather than the
> in-tree tests.
> 
> The tests applies the dummy in-tree patch example, checks that the patch is
> applied correctly and then reverts and unloads it.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changes since v2:
>  - Split livepatch build into a separate script.
> ---
>  automation/gitlab-ci/build.yaml               |  9 +++
>  automation/gitlab-ci/test.yaml                |  8 +++
>  automation/scripts/build-livepatch            | 27 ++++++++
>  .../scripts/qemu-alpine-x86_64-livepatch.sh   | 68 +++++++++++++++++++
>  4 files changed, 112 insertions(+)
>  create mode 100755 automation/scripts/build-livepatch
>  create mode 100755 automation/scripts/qemu-alpine-x86_64-livepatch.sh
> 
> diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
> index 32af30ccedc9..d770bffb845e 100644
> --- a/automation/gitlab-ci/build.yaml
> +++ b/automation/gitlab-ci/build.yaml
> @@ -358,6 +358,15 @@ alpine-3.18-gcc-debug:
>    variables:
>      CONTAINER: alpine:3.18
>  
> +alpine-3.18-gcc-livepatch:
> +  extends: .gcc-x86-64-build
> +  script:
> +    - ./automation/scripts/build-livepatch 2>&1 | tee build.log
> +  variables:
> +    CONTAINER: alpine:3.18
> +    EXTRA_XEN_CONFIG: |
> +      CONFIG_LIVEPATCH=y
> +
>  debian-stretch-gcc-debug:
>    extends: .gcc-x86-64-build-debug
>    variables:
> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index 6aabdb9d156f..58a90be5ed0e 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -459,3 +459,11 @@ qemu-smoke-ppc64le-powernv9-gcc:
>    needs:
>      - qemu-system-ppc64-8.1.0-ppc64-export
>      - debian-bullseye-gcc-ppc64le-debug
> +
> +qemu-alpine-x86_64-gcc-livepatch:
> +  extends: .qemu-x86-64
> +  script:
> +    - ./automation/scripts/qemu-alpine-x86_64-livepatch.sh 2>&1 | tee ${LOGFILE}
> +  needs:
> +    - *x86-64-test-needs
> +    - alpine-3.18-gcc-livepatch
> diff --git a/automation/scripts/build-livepatch b/automation/scripts/build-livepatch
> new file mode 100755
> index 000000000000..ac86b17ae5e4
> --- /dev/null
> +++ b/automation/scripts/build-livepatch
> @@ -0,0 +1,27 @@
> +#!/bin/bash -ex
> +
> +# Do a regular build first
> +./automation/scripts/build
> +
> +# Build a test livepatch using livepatch-build-tools.
> +
> +if [[ "$XEN_TARGET_ARCH" != "x86_64" ]]; then
> +    exit 1
> +fi
> +
> +BASE=xen/arch/x86/test/smoc-lp.c
> +ALT=xen/arch/x86/test/smoc-lp-alt.c
> +
> +[[ -f $BASE && -f $ALT ]]
> +
> +# git diff --no-index returns 0 if no differences, otherwise 1.
> +git diff --no-index --output=test.patch $BASE $ALT && exit 1
> +
> +BUILDID=$(readelf -Wn xen/xen-syms | sed -n -e 's/^.*Build ID: //p')
> +
> +git clone https://xenbits.xen.org/git-http/livepatch-build-tools.git
> +cd livepatch-build-tools
> +make
> +./livepatch-build -s ../ -p ../test.patch -o out -c ../xen/.config \
> +    --depends $BUILDID --xen-depends $BUILDID
> +cp out/test.livepatch ../binaries/test.livepatch
> diff --git a/automation/scripts/qemu-alpine-x86_64-livepatch.sh b/automation/scripts/qemu-alpine-x86_64-livepatch.sh
> new file mode 100755
> index 000000000000..da478cac4376
> --- /dev/null
> +++ b/automation/scripts/qemu-alpine-x86_64-livepatch.sh
> @@ -0,0 +1,68 @@
> +#!/bin/bash
> +
> +set -ex
> +
> +cd binaries
> +# initrd.tar.gz is Dom0 rootfs
> +mkdir -p rootfs
> +cd rootfs
> +tar xvzf ../initrd.tar.gz
> +mkdir proc
> +mkdir run
> +mkdir srv
> +mkdir sys
> +rm var/run
> +cp -ar ../dist/install/* .
> +cp ../test.livepatch ./root/
> +cat << "EOF" >> etc/local.d/xen-lp.start
> +#!/bin/bash
> +
> +set -ex
> +
> +trap poweroff EXIT
> +
> +export LD_LIBRARY_PATH=/usr/local/lib
> +
> +xen-livepatch test && exit 1 || true
> +
> +xen-livepatch load /root/test.livepatch
> +
> +# Cannot fail now
> +xen-livepatch test
> +
> +xen-livepatch revert test
> +xen-livepatch unload test
> +
> +xen-livepatch test && exit 1 || true
> +
> +echo "SUCCESS"
> +EOF
> +chmod +x etc/local.d/xen-lp.start
> +echo "rc_verbose=yes" >> etc/rc.conf
> +# rebuild Dom0 rootfs
> +find . |cpio -H newc -o|gzip > ../xen-rootfs.cpio.gz
> +cd ../..
> +
> +cat >> binaries/pxelinux.0 << EOF
> +#!ipxe
> +
> +kernel xen console=com1 console_timestamps=boot
> +module bzImage console=hvc0
> +module xen-rootfs.cpio.gz
> +boot
> +EOF
> +
> +# Run the test
> +rm -f smoke.serial
> +timeout -k 1 360 \
> +qemu-system-x86_64 \
> +    -cpu qemu64,+svm \
> +    -m 2G -smp 2 \
> +    -monitor none -serial stdio \
> +    -nographic \
> +    -device virtio-net-pci,netdev=n0 \
> +    -netdev user,id=n0,tftp=binaries,bootfile=/pxelinux.0 |& \
> +        tee smoke.serial | sed 's/\r//'
> +
> +grep -q "SUCCESS" smoke.serial
> +exit 0
> -- 
> 2.43.0
>