| Message ID | 20220214125127.17985-7-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Support for CET Indirect Branch Tracking | expand |
On 14.02.2022 13:50, Andrew Cooper wrote: > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -39,6 +39,11 @@ config HAS_AS_CET_SS > # binutils >= 2.29 or LLVM >= 6 > def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy) > > +config HAS_CC_CET_IBT > + # GCC >= 9 and binutils >= 2.29 > + # Retpoline check to work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654 > + def_bool $(cc-option,-fcf-protection=branch -mmanual-endbr -mindirect-branch=thunk-extern) && $(as-instr,endbr64) At the top of asm-defns.h we have a number of similarly operand-less instructions expressed via .macro expanding to .byte. I don't see why we couldn't do so here as well, eliminating the need for the $(as-instr ...). In fact ... > --- a/xen/arch/x86/include/asm/asm-defns.h > +++ b/xen/arch/x86/include/asm/asm-defns.h > @@ -57,6 +57,12 @@ > INDIRECT_BRANCH jmp \arg > .endm > > +#ifdef CONFIG_XEN_IBT > +# define ENDBR64 endbr64 > +#else > +# define ENDBR64 > +#endif ... it could also be this macro which ends up conditionally empty, but would then want expressing as an assembler macro. Albeit no, the lower case form would probably still be needed to deal with compiler emitted insns, as the compiler doesn't appear to make recognition of the command line option dependent on the underlying assembler's capabilities. > --- a/xen/arch/x86/include/asm/cpufeatures.h > +++ b/xen/arch/x86/include/asm/cpufeatures.h > @@ -39,6 +39,7 @@ XEN_CPUFEATURE(SC_VERW_PV, X86_SYNTH(23)) /* VERW used by Xen for PV */ > XEN_CPUFEATURE(SC_VERW_HVM, X86_SYNTH(24)) /* VERW used by Xen for HVM */ > XEN_CPUFEATURE(SC_VERW_IDLE, X86_SYNTH(25)) /* VERW used by Xen for idle */ > XEN_CPUFEATURE(XEN_SHSTK, X86_SYNTH(26)) /* Xen uses CET Shadow Stacks */ > +XEN_CPUFEATURE(XEN_IBT, X86_SYNTH(27)) /* Xen uses CET Indirect Branch Tracking */ Is a feature flag actually warranted here, rather than a single global boolean? You don't key any alternatives patching to this bit, unlike was the case for XEN_SHSTK. And the only consumer is cpu_has_xen_ibt, expanding to the boot CPU's instance of the bit. Jan
On 15/02/2022 14:01, Jan Beulich wrote: > On 14.02.2022 13:50, Andrew Cooper wrote: >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -39,6 +39,11 @@ config HAS_AS_CET_SS >> # binutils >= 2.29 or LLVM >= 6 >> def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy) >> >> +config HAS_CC_CET_IBT >> + # GCC >= 9 and binutils >= 2.29 >> + # Retpoline check to work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654 >> + def_bool $(cc-option,-fcf-protection=branch -mmanual-endbr -mindirect-branch=thunk-extern) && $(as-instr,endbr64) > At the top of asm-defns.h we have a number of similarly operand-less > instructions expressed via .macro expanding to .byte. I don't see why > we couldn't do so here as well, eliminating the need for the > $(as-instr ...). In fact ... > >> --- a/xen/arch/x86/include/asm/asm-defns.h >> +++ b/xen/arch/x86/include/asm/asm-defns.h >> @@ -57,6 +57,12 @@ >> INDIRECT_BRANCH jmp \arg >> .endm >> >> +#ifdef CONFIG_XEN_IBT >> +# define ENDBR64 endbr64 >> +#else >> +# define ENDBR64 >> +#endif > ... it could also be this macro which ends up conditionally empty, > but would then want expressing as an assembler macro. Albeit no, the > lower case form would probably still be needed to deal with compiler > emitted insns, as the compiler doesn't appear to make recognition of > the command line option dependent on the underlying assembler's > capabilities. $(as-instr) isn't only for endbr64. It also for the notrack prefix, which GCC does emit for any function pointer call laundered through void * even when everything was otherwise cf_check. It's another area where treating the cf_check-ness as type-checking falls down, and created some very weird build failures until I figured out that Juergen's "Don't use the hypercall table for calling compat hypercalls" really did need to be a prerequisite. CET-IBT toolchain support is 3 years old already, and I don't think there is any value attempting to support a developer mixing a new GCC and ancient binutils. >> --- a/xen/arch/x86/include/asm/cpufeatures.h >> +++ b/xen/arch/x86/include/asm/cpufeatures.h >> @@ -39,6 +39,7 @@ XEN_CPUFEATURE(SC_VERW_PV, X86_SYNTH(23)) /* VERW used by Xen for PV */ >> XEN_CPUFEATURE(SC_VERW_HVM, X86_SYNTH(24)) /* VERW used by Xen for HVM */ >> XEN_CPUFEATURE(SC_VERW_IDLE, X86_SYNTH(25)) /* VERW used by Xen for idle */ >> XEN_CPUFEATURE(XEN_SHSTK, X86_SYNTH(26)) /* Xen uses CET Shadow Stacks */ >> +XEN_CPUFEATURE(XEN_IBT, X86_SYNTH(27)) /* Xen uses CET Indirect Branch Tracking */ > Is a feature flag actually warranted here, rather than a single > global boolean? You don't key any alternatives patching to this > bit, unlike was the case for XEN_SHSTK. And the only consumer is > cpu_has_xen_ibt, expanding to the boot CPU's instance of the bit. These are just bits. They long predate alternatives finding a convenient use for the form, and are 8 times more compact than a global boolean, with better locality of reference too. ~Andrew
On 16.02.2022 22:54, Andrew Cooper wrote: > On 15/02/2022 14:01, Jan Beulich wrote: >> On 14.02.2022 13:50, Andrew Cooper wrote: >>> --- a/xen/arch/x86/Kconfig >>> +++ b/xen/arch/x86/Kconfig >>> @@ -39,6 +39,11 @@ config HAS_AS_CET_SS >>> # binutils >= 2.29 or LLVM >= 6 >>> def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy) >>> >>> +config HAS_CC_CET_IBT >>> + # GCC >= 9 and binutils >= 2.29 >>> + # Retpoline check to work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654 >>> + def_bool $(cc-option,-fcf-protection=branch -mmanual-endbr -mindirect-branch=thunk-extern) && $(as-instr,endbr64) >> At the top of asm-defns.h we have a number of similarly operand-less >> instructions expressed via .macro expanding to .byte. I don't see why >> we couldn't do so here as well, eliminating the need for the >> $(as-instr ...). In fact ... >> >>> --- a/xen/arch/x86/include/asm/asm-defns.h >>> +++ b/xen/arch/x86/include/asm/asm-defns.h >>> @@ -57,6 +57,12 @@ >>> INDIRECT_BRANCH jmp \arg >>> .endm >>> >>> +#ifdef CONFIG_XEN_IBT >>> +# define ENDBR64 endbr64 >>> +#else >>> +# define ENDBR64 >>> +#endif >> ... it could also be this macro which ends up conditionally empty, >> but would then want expressing as an assembler macro. Albeit no, the >> lower case form would probably still be needed to deal with compiler >> emitted insns, as the compiler doesn't appear to make recognition of >> the command line option dependent on the underlying assembler's >> capabilities. > > $(as-instr) isn't only for endbr64. It also for the notrack prefix, > which GCC does emit for any function pointer call laundered through void > * even when everything was otherwise cf_check. > > It's another area where treating the cf_check-ness as type-checking > falls down, and created some very weird build failures until I figured > out that Juergen's "Don't use the hypercall table for calling compat > hypercalls" really did need to be a prerequisite. Oh, I see. I can certainly accept this as a reason, but half a sentence mentioning this would be nice in the description. >>> --- a/xen/arch/x86/include/asm/cpufeatures.h >>> +++ b/xen/arch/x86/include/asm/cpufeatures.h >>> @@ -39,6 +39,7 @@ XEN_CPUFEATURE(SC_VERW_PV, X86_SYNTH(23)) /* VERW used by Xen for PV */ >>> XEN_CPUFEATURE(SC_VERW_HVM, X86_SYNTH(24)) /* VERW used by Xen for HVM */ >>> XEN_CPUFEATURE(SC_VERW_IDLE, X86_SYNTH(25)) /* VERW used by Xen for idle */ >>> XEN_CPUFEATURE(XEN_SHSTK, X86_SYNTH(26)) /* Xen uses CET Shadow Stacks */ >>> +XEN_CPUFEATURE(XEN_IBT, X86_SYNTH(27)) /* Xen uses CET Indirect Branch Tracking */ >> Is a feature flag actually warranted here, rather than a single >> global boolean? You don't key any alternatives patching to this >> bit, unlike was the case for XEN_SHSTK. And the only consumer is >> cpu_has_xen_ibt, expanding to the boot CPU's instance of the bit. > > These are just bits. They long predate alternatives finding a > convenient use for the form, and are 8 times more compact than a global > boolean, with better locality of reference too. Well, I disagree (and we were here before, so I think you could have predicted such a comment coming back). We should never have cloned this directly from Linux. It's only bits, but with enough CPUs it sums up. We shouldn't duplicate data when we need only a single instance (and when no other infrastructure, like alternative patching, depends on it). Last time you put me in a situation like this one, I told myself to not ack such changes anymore, but here I am again - in the interest of not being blamed for blocking this series: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/Config.mk b/Config.mk index 95c053212ec3..f56f7dc33468 100644 --- a/Config.mk +++ b/Config.mk @@ -190,7 +190,6 @@ APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i)) EMBEDDED_EXTRA_CFLAGS := -nopie -fno-stack-protector -fno-stack-protector-all EMBEDDED_EXTRA_CFLAGS += -fno-exceptions -fno-asynchronous-unwind-tables -EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles # All the files at that location were downloaded from elsewhere on diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile index 345037b93b7f..53ed4f161edb 100644 --- a/tools/firmware/Makefile +++ b/tools/firmware/Makefile @@ -6,6 +6,8 @@ TARGET := hvmloader/hvmloader INST_DIR := $(DESTDIR)$(XENFIRMWAREDIR) DEBG_DIR := $(DESTDIR)$(DEBUG_DIR)$(XENFIRMWAREDIR) +EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none + SUBDIRS-y := SUBDIRS-$(CONFIG_OVMF) += ovmf-dir SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir diff --git a/tools/libs/guest/xg_dom_decompress_unsafe.h b/tools/libs/guest/xg_dom_decompress_unsafe.h index 4e0bf23aa587..3bce0cfefb88 100644 --- a/tools/libs/guest/xg_dom_decompress_unsafe.h +++ b/tools/libs/guest/xg_dom_decompress_unsafe.h @@ -8,6 +8,8 @@ typedef int decompress_fn(unsigned char *inbuf, unsigned int len, void (*error)(const char *x)); #endif +#define cf_check + int xc_dom_decompress_unsafe( decompress_fn fn, struct xc_dom_image *dom, void **blob, size_t *size) __attribute__((visibility("internal"))); diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h index 7f60ef9e89ba..c6819a417d05 100644 --- a/tools/tests/x86_emulator/x86-emulate.h +++ b/tools/tests/x86_emulator/x86-emulate.h @@ -54,6 +54,8 @@ #define likely(x) __builtin_expect(!!(x), true) #define unlikely(x) __builtin_expect(!!(x), false) +#define cf_check + #define container_of(ptr, type, member) ({ \ typeof(((type *)0)->member) *mptr__ = (ptr); \ (type *)((char *)mptr__ - offsetof(type, member)); \ diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index b4abfca46f6a..8b7ad0145b29 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -39,6 +39,11 @@ config HAS_AS_CET_SS # binutils >= 2.29 or LLVM >= 6 def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy) +config HAS_CC_CET_IBT + # GCC >= 9 and binutils >= 2.29 + # Retpoline check to work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654 + def_bool $(cc-option,-fcf-protection=branch -mmanual-endbr -mindirect-branch=thunk-extern) && $(as-instr,endbr64) + menu "Architecture Features" source "arch/Kconfig" @@ -124,6 +129,18 @@ config XEN_SHSTK When CET-SS is active, 32bit PV guests cannot be used. Backwards compatiblity can be provided via the PV Shim mechanism. +config XEN_IBT + bool "Supervisor Indirect Branch Tracking" + depends on HAS_CC_CET_IBT + default y + help + Control-flow Enforcement Technology (CET) is a set of features in + hardware designed to combat Return-oriented Programming (ROP, also + call/jump COP/JOP) attacks. Indirect Branch Tracking is one CET + feature designed to provide function pointer protection. + + This option arranges for Xen to use CET-IBT for its own protection. + config SHADOW_PAGING bool "Shadow Paging" default !PV_SHIM_EXCLUSIVE diff --git a/xen/arch/x86/arch.mk b/xen/arch/x86/arch.mk index fa7cf3844362..8b88f0240e85 100644 --- a/xen/arch/x86/arch.mk +++ b/xen/arch/x86/arch.mk @@ -47,6 +47,12 @@ CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch=thunk-extern CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch-register CFLAGS-$(CONFIG_INDIRECT_THUNK) += -fno-jump-tables +ifdef CONFIG_XEN_IBT +CFLAGS += -fcf-protection=branch -mmanual-endbr +else +$(call cc-option-add,CFLAGS,CC,-fcf-protection=none) +endif + # If supported by the compiler, reduce stack alignment to 8 bytes. But allow # this to be overridden elsewhere. $(call cc-option-add,CFLAGS_stack_boundary,CC,-mpreferred-stack-boundary=3) diff --git a/xen/arch/x86/include/asm/asm-defns.h b/xen/arch/x86/include/asm/asm-defns.h index 505f39ad5f76..8bd9007731d5 100644 --- a/xen/arch/x86/include/asm/asm-defns.h +++ b/xen/arch/x86/include/asm/asm-defns.h @@ -57,6 +57,12 @@ INDIRECT_BRANCH jmp \arg .endm +#ifdef CONFIG_XEN_IBT +# define ENDBR64 endbr64 +#else +# define ENDBR64 +#endif + .macro guest_access_mask_ptr ptr:req, scratch1:req, scratch2:req #if defined(CONFIG_SPECULATIVE_HARDEN_GUEST_ACCESS) /* diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h index a0ab6d7d78ea..f2c6f255ace9 100644 --- a/xen/arch/x86/include/asm/cpufeature.h +++ b/xen/arch/x86/include/asm/cpufeature.h @@ -152,6 +152,7 @@ #define cpu_has_nscb boot_cpu_has(X86_FEATURE_NSCB) #define cpu_has_xen_lbr boot_cpu_has(X86_FEATURE_XEN_LBR) #define cpu_has_xen_shstk boot_cpu_has(X86_FEATURE_XEN_SHSTK) +#define cpu_has_xen_ibt boot_cpu_has(X86_FEATURE_XEN_IBT) #define cpu_has_msr_tsc_aux (cpu_has_rdtscp || cpu_has_rdpid) diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h index b10154fc44bb..7413febd7ad8 100644 --- a/xen/arch/x86/include/asm/cpufeatures.h +++ b/xen/arch/x86/include/asm/cpufeatures.h @@ -39,6 +39,7 @@ XEN_CPUFEATURE(SC_VERW_PV, X86_SYNTH(23)) /* VERW used by Xen for PV */ XEN_CPUFEATURE(SC_VERW_HVM, X86_SYNTH(24)) /* VERW used by Xen for HVM */ XEN_CPUFEATURE(SC_VERW_IDLE, X86_SYNTH(25)) /* VERW used by Xen for idle */ XEN_CPUFEATURE(XEN_SHSTK, X86_SYNTH(26)) /* Xen uses CET Shadow Stacks */ +XEN_CPUFEATURE(XEN_IBT, X86_SYNTH(27)) /* Xen uses CET Indirect Branch Tracking */ /* Bug words follow the synthetic words. */ #define X86_NR_BUG 1 diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index 696c7eb89e4c..933aec09a92d 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -37,6 +37,12 @@ # define nocall #endif +#ifdef CONFIG_XEN_IBT +# define cf_check __attribute__((__cf_check__)) +#else +# define cf_check +#endif + #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5)) #define unreachable() do {} while (1) #else
CET Indirect Branch Tracking is a hardware feature designed to provide forward-edge control flow integrity, protecting against jump/call oriented programming. IBT requires the placement of ENDBR{32,64} instructions at the target of every indirect call/jmp, and every entrypoint. However, the default -fcf-protection=branch places an ENDBR on every function which far more than necessary, and reduces the quantity of protection afforded. Therefore, we use manual placement using the cf_check attribute. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> Clang/LLVM support for -mmanual-endbr is in progress: https://reviews.llvm.org/D118355 v2: * Correct CONFIG_HAS_CC_CET_IBT to CONFIG_XEN_IBT in some places * Move cf_check compatibility into tools/tests/x86_emulator/x86-emulate.h --- Config.mk | 1 - tools/firmware/Makefile | 2 ++ tools/libs/guest/xg_dom_decompress_unsafe.h | 2 ++ tools/tests/x86_emulator/x86-emulate.h | 2 ++ xen/arch/x86/Kconfig | 17 +++++++++++++++++ xen/arch/x86/arch.mk | 6 ++++++ xen/arch/x86/include/asm/asm-defns.h | 6 ++++++ xen/arch/x86/include/asm/cpufeature.h | 1 + xen/arch/x86/include/asm/cpufeatures.h | 1 + xen/include/xen/compiler.h | 6 ++++++ 10 files changed, 43 insertions(+), 1 deletion(-)