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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 06/70] x86: Introduce support for CET-IBT
Date: Tue, 15 Feb 2022 15:01:42 +0100	[thread overview]
Message-ID: <26bc666b-e9c3-7f12-6d8d-256311846233@suse.com> (raw)
In-Reply-To: <20220214125127.17985-7-andrew.cooper3@citrix.com>

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