Re: [PATCH v24 04/30] x86/cpufeatures: Introduce X86_FEATURE_CET and setup functions

From: Borislav Petkov <bp@alien8.de>
To: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-mm@kvack.org, linux-arch@vger.kernel.org,
	linux-api@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
	Andy Lutomirski <luto@kernel.org>,
	Balbir Singh <bsingharora@gmail.com>,
	Cyrill Gorcunov <gorcunov@gmail.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Eugene Syromiatnikov <esyr@redhat.com>,
	Florian Weimer <fweimer@redhat.com>,
	"H.J. Lu" <hjl.tools@gmail.com>, Jann Horn <jannh@google.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Kees Cook <keescook@chromium.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Nadav Amit <nadav.amit@gmail.com>,
	Oleg Nesterov <oleg@redhat.com>, Pavel Machek <pavel@ucw.cz>,
	Peter Zijlstra <peterz@infradead.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	Vedvyas Shanbhogue <vedvyas.shanbhogue@intel.com>,
	Dave Martin <Dave.Martin@arm.com>,
	Weijiang Yang <weijiang.yang@intel.com>,
	Pengfei Xu <pengfei.xu@intel.com>,
	Haitao Huang <haitao.huang@intel.com>
Subject: Re: [PATCH v24 04/30] x86/cpufeatures: Introduce X86_FEATURE_CET and setup functions
Date: Fri, 9 Apr 2021 12:12:14 +0200	[thread overview]
Message-ID: <20210409101214.GC15567@zn.tnic> (raw)
In-Reply-To: <20210401221104.31584-5-yu-cheng.yu@intel.com>

On Thu, Apr 01, 2021 at 03:10:38PM -0700, Yu-cheng Yu wrote:
> Introduce a software-defined X86_FEATURE_CET, which indicates either Shadow
> Stack or Indirect Branch Tracking (or both) is present.  Also introduce
> related cpu init/setup functions.
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> ---
> v24:
> - Update #ifdef placement to reflect Kconfig changes of splitting shadow stack and ibt.
> 
>  arch/x86/include/asm/cpufeatures.h          |  2 +-
>  arch/x86/include/asm/disabled-features.h    |  9 ++++++++-
>  arch/x86/include/uapi/asm/processor-flags.h |  2 ++
>  arch/x86/kernel/cpu/common.c                | 14 ++++++++++++++
>  arch/x86/kernel/cpu/intel.c                 |  3 +++
>  5 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index bf861fc89fef..d771e62677de 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -108,7 +108,7 @@
>  #define X86_FEATURE_EXTD_APICID		( 3*32+26) /* Extended APICID (8 bits) */
>  #define X86_FEATURE_AMD_DCM		( 3*32+27) /* AMD multi-node processor */
>  #define X86_FEATURE_APERFMPERF		( 3*32+28) /* P-State hardware coordination feedback capability (APERF/MPERF MSRs) */
> -/* free					( 3*32+29) */
> +#define X86_FEATURE_CET			( 3*32+29) /* Control-flow enforcement */

Right, I know we talked about having this synthetic flag but now that we
are moving to CONFIG_X86_SHADOW_STACK and separate SHSTK and IBT feature
bits, that synthetic flag is not needed anymore.

For the cases where you wanna test whether any of the two are present,
we're probably better off adding a x86_cet_enabled() helper which tests
SHSTK and IBT bits.

I haven't gone through the whole thing yet but depending on the context
and the fact that AMD doesn't support IBT, that helper might need some
tweaking too. I'll see.

>  #define X86_FEATURE_NONSTOP_TSC_S3	( 3*32+30) /* TSC doesn't stop in S3 state */
>  #define X86_FEATURE_TSC_KNOWN_FREQ	( 3*32+31) /* TSC has known frequency */
>  
> diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h
> index e5c6ed9373e8..018cd7acd3e9 100644
> --- a/arch/x86/include/asm/disabled-features.h
> +++ b/arch/x86/include/asm/disabled-features.h
> @@ -74,13 +74,20 @@
>  #define DISABLE_SHSTK	(1 << (X86_FEATURE_SHSTK & 31))
>  #endif
>  
> +#ifdef CONFIG_X86_CET

And you don't need that config item either - AFAICT, you can use
CONFIG_X86_SHADOW_STACK everywhere.

Which would simplify that config space.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette