Re: [PATCH v2 06/70] x86: Introduce support for CET-IBT

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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