Re: [PATCH v2 06/70] x86: Introduce support for CET-IBT
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 Jan
Re: [PATCH v2 06/70] x86: Introduce support for CET-IBT
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
Re: [PATCH v2 06/70] x86: Introduce support for CET-IBT
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
