| Message ID | 20231128100352.35430-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/x86: add testing for self modifying code and livepatch | expand |
On 28.11.2023 11:03, 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> > --- > Changes since v1: > - New interface & test. > --- > tools/misc/xen-livepatch.c | 29 +++++++++++++++++++++++++++++ > xen/arch/x86/Makefile | 2 ++ > xen/arch/x86/include/asm/test-smc.h | 2 ++ > xen/arch/x86/setup.c | 2 +- > xen/arch/x86/test-smc-lp-alt.c | 23 +++++++++++++++++++++++ > xen/arch/x86/test-smc-lp.c | 23 +++++++++++++++++++++++ > xen/arch/x86/test-smc.c | 11 ++++++++++- > xen/include/public/sysctl.h | 6 +++++- > 8 files changed, 95 insertions(+), 3 deletions(-) > create mode 100644 xen/arch/x86/test-smc-lp-alt.c > create mode 100644 xen/arch/x86/test-smc-lp.c Can these (and perhaps also the one file introduced earlier in the series) perhaps become xen/arch/x86/test/smc*.c? > --- /dev/null > +++ b/xen/arch/x86/test-smc-lp-alt.c > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include <asm/test-smc.h> > + > +/* > + * Interesting case because `return false` can be encoded as an xor > + * instruction, which is shorter than `return true` which is a mov instruction, > + * and also shorter than a jmp instruction. > + */ I'm a little wary of this comment: "mov $1, %al" is two bytes only, just like "xor %eax, %eax" is. > +bool cf_check test_lp_insn_replacement(void) What's the purpose of the cf_check here? Jan
On Tue, Dec 05, 2023 at 12:49:38PM +0100, Jan Beulich wrote: > On 28.11.2023 11:03, 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> > > --- > > Changes since v1: > > - New interface & test. > > --- > > tools/misc/xen-livepatch.c | 29 +++++++++++++++++++++++++++++ > > xen/arch/x86/Makefile | 2 ++ > > xen/arch/x86/include/asm/test-smc.h | 2 ++ > > xen/arch/x86/setup.c | 2 +- > > xen/arch/x86/test-smc-lp-alt.c | 23 +++++++++++++++++++++++ > > xen/arch/x86/test-smc-lp.c | 23 +++++++++++++++++++++++ > > xen/arch/x86/test-smc.c | 11 ++++++++++- > > xen/include/public/sysctl.h | 6 +++++- > > 8 files changed, 95 insertions(+), 3 deletions(-) > > create mode 100644 xen/arch/x86/test-smc-lp-alt.c > > create mode 100644 xen/arch/x86/test-smc-lp.c > > Can these (and perhaps also the one file introduced earlier in the series) > perhaps become xen/arch/x86/test/smc*.c? Yes, sure, I don't see why not. > > --- /dev/null > > +++ b/xen/arch/x86/test-smc-lp-alt.c > > @@ -0,0 +1,23 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > + > > +#include <asm/test-smc.h> > > + > > +/* > > + * Interesting case because `return false` can be encoded as an xor > > + * instruction, which is shorter than `return true` which is a mov instruction, > > + * and also shorter than a jmp instruction. > > + */ > > I'm a little wary of this comment: "mov $1, %al" is two bytes only, just like Don't we need to zero the high part of the register also? Or since the return type is a bool the compiler could assume it's truncated by the caller? > "xor %eax, %eax" is. GCC 13.2 (from godbolt) generates at -O2: mov $0x1,%eax ret Which is 5 bytes long mov insn. The return false case is: xor %eax,%eax ret I can adjust to mention this specific behavior. > > +bool cf_check test_lp_insn_replacement(void) > > What's the purpose of the cf_check here? Because it's added to the array of test functions to call in test_smc(). Doesn't it need cf_check in that case? Thanks, Roger.
On 05.12.2023 14:08, Roger Pau Monné wrote: > On Tue, Dec 05, 2023 at 12:49:38PM +0100, Jan Beulich wrote: >> On 28.11.2023 11:03, Roger Pau Monne wrote: >>> --- /dev/null >>> +++ b/xen/arch/x86/test-smc-lp-alt.c >>> @@ -0,0 +1,23 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> + >>> +#include <asm/test-smc.h> >>> + >>> +/* >>> + * Interesting case because `return false` can be encoded as an xor >>> + * instruction, which is shorter than `return true` which is a mov instruction, >>> + * and also shorter than a jmp instruction. >>> + */ >> >> I'm a little wary of this comment: "mov $1, %al" is two bytes only, just like > > Don't we need to zero the high part of the register also? Or since > the return type is a bool the compiler could assume it's truncated by > the caller? I think so, yes. I.e. ... >> "xor %eax, %eax" is. > > GCC 13.2 (from godbolt) generates at -O2: > > mov $0x1,%eax > ret > > Which is 5 bytes long mov insn. ... at -Os I'd kind of expect the compiler to use the shorter (albeit slower to execute) "mov $1,%al" (unless of course I'm overlooking a specific rule in psABI). > The return false case is: > > xor %eax,%eax > ret > > I can adjust to mention this specific behavior. > >>> +bool cf_check test_lp_insn_replacement(void) >> >> What's the purpose of the cf_check here? > > Because it's added to the array of test functions to call in > test_smc(). Doesn't it need cf_check in that case? Oh, of course it does. I managed to overlook that use (misguided by one of the two files being built without being actually used). Jan
diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c index 5bf9d9a32b65..fb396f46aaac 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_smc(xch, XEN_SYSCTL_TEST_SMC_LP, &results); + if ( rc ) + { + fprintf(stderr, "test operation failed: %s\n", strerror(errno)); + return -1; + } + if ( (results & XEN_SYSCTL_TEST_SMC_LP) != XEN_SYSCTL_TEST_SMC_LP ) + { + fprintf(stderr, "some tests failed: %#x (expected %#x)\n", + results, XEN_SYSCTL_TEST_SMC_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/Makefile b/xen/arch/x86/Makefile index bdd2183a2fd7..71cb22e080b8 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -66,6 +66,8 @@ obj-y += spec_ctrl.o obj-y += srat.o obj-y += string.o obj-y += test-smc.o +obj-$(CONFIG_LIVEPATCH) += test-smc-lp.o # for livepatch testing +extra-$(CONFIG_LIVEPATCH) += test-smc-lp-alt.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 index 18b23dbdbf2d..6013e4daf7f8 100644 --- a/xen/arch/x86/include/asm/test-smc.h +++ b/xen/arch/x86/include/asm/test-smc.h @@ -5,6 +5,8 @@ int test_smc(uint32_t selection, uint32_t *results); +bool cf_check test_lp_insn_replacement(void); + #endif /* _ASM_X86_TEST_SMC_H_ */ /* diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 1f90d30204fe..8bfb394909b4 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1953,7 +1953,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) alternative_branches(); - test_smc(XEN_SYSCTL_TEST_SMC_ALL, NULL); + test_smc(XEN_SYSCTL_TEST_SMC_ALL & ~XEN_SYSCTL_TEST_SMC_LP, NULL); /* * NB: when running as a PV shim VCPUOP_up/down is wired to the shim diff --git a/xen/arch/x86/test-smc-lp-alt.c b/xen/arch/x86/test-smc-lp-alt.c new file mode 100644 index 000000000000..7bde547a950d --- /dev/null +++ b/xen/arch/x86/test-smc-lp-alt.c @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <asm/test-smc.h> + +/* + * Interesting case because `return false` can be encoded as an xor + * instruction, which is shorter than `return true` which is a mov instruction, + * and also shorter than a 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-smc-lp.c b/xen/arch/x86/test-smc-lp.c new file mode 100644 index 000000000000..0ae776053a42 --- /dev/null +++ b/xen/arch/x86/test-smc-lp.c @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <asm/test-smc.h> + +/* + * Interesting case because `return false` can be encoded as an xor + * instruction, which is shorter than `return true` which is a mov instruction, + * and also shorter than a 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-smc.c b/xen/arch/x86/test-smc.c index 8916c185d60a..1967016a229f 100644 --- a/xen/arch/x86/test-smc.c +++ b/xen/arch/x86/test-smc.c @@ -27,6 +27,10 @@ int test_smc(uint32_t selection, uint32_t *results) } static const tests[] = { { XEN_SYSCTL_TEST_SMC_INSN_REPL, &test_insn_replacement, "alternative instruction replacement" }, +#ifdef CONFIG_LIVEPATCH + { XEN_SYSCTL_TEST_SMC_LP_INSN, &test_lp_insn_replacement, + "livepatch instruction replacement" }, +#endif }; unsigned int i; @@ -50,7 +54,12 @@ int test_smc(uint32_t selection, uint32_t *results) continue; } - add_taint(TAINT_ERROR_SMC); + /* + * livepatch related tests don't taint the hypervisor because we also + * want to check the failing case. + */ + if ( !(tests[i].mask & XEN_SYSCTL_TEST_SMC_LP) ) + add_taint(TAINT_ERROR_SMC); 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 94287009387c..c87878e72a42 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -1204,7 +1204,11 @@ struct xen_sysctl_dt_overlay { 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) +#define XEN_SYSCTL_TEST_SMC_LP_INSN (1U << 1) +#define XEN_SYSCTL_TEST_SMC_ALL (XEN_SYSCTL_TEST_SMC_INSN_REPL | \ + XEN_SYSCTL_TEST_SMC_LP_INSN) +#define XEN_SYSCTL_TEST_SMC_LP (XEN_SYSCTL_TEST_SMC_LP_INSN) + uint32_t results; /* OUT: test result: 1 -> success, 0 -> failure. */ };
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 v1: - New interface & test. --- tools/misc/xen-livepatch.c | 29 +++++++++++++++++++++++++++++ xen/arch/x86/Makefile | 2 ++ xen/arch/x86/include/asm/test-smc.h | 2 ++ xen/arch/x86/setup.c | 2 +- xen/arch/x86/test-smc-lp-alt.c | 23 +++++++++++++++++++++++ xen/arch/x86/test-smc-lp.c | 23 +++++++++++++++++++++++ xen/arch/x86/test-smc.c | 11 ++++++++++- xen/include/public/sysctl.h | 6 +++++- 8 files changed, 95 insertions(+), 3 deletions(-) create mode 100644 xen/arch/x86/test-smc-lp-alt.c create mode 100644 xen/arch/x86/test-smc-lp.c