Received-SPF: pass (google.com: domain of linux-kernel+bounces-165584-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 1 May 2024 09:33:14 -0700
From: Nathan Chancellor <nathan@kernel.org>
To: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Sami Tolvanen <samitolvanen@google.com>,
	Josh Poimboeuf <jpoimboe@kernel.org>,
	Juergen Gross <jgross@suse.com>, Breno Leitao <leitao@debian.org>,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] x86/alternatives: Make FineIBT mode Kconfig selectable
Message-ID: <20240501163314.GA2472577@dev-arch.thelio-3990X>
References: <20240501000218.work.998-kees@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240501000218.work.998-kees@kernel.org>

On Tue, Apr 30, 2024 at 05:02:22PM -0700, Kees Cook wrote:
> Since FineIBT performs checking at the destination, it is weaker against
> attacks that can construct arbitrary executable memory contents. As such,
> some system builders want to run with FineIBT disabled by default. Allow
> the "cfi=kcfi" boot param mode to be selectable through Kconfig via the
> newly introduced CONFIG_CFI_AUTO_DEFAULT.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

I verified that flipping the configuration does indeed change the
default and that 'cfi=' could still be used to override whatever choice
was made at compile time. This patch was a perfect excuse to put my new
CET enabled test machine to work.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>

CFI_DEFAULT_AUTO reads a little bit better to me personally but I am not
looking to get into painting today :)

> ---
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: x86@kernel.org
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
>  arch/x86/Kconfig              | 9 +++++++++
>  arch/x86/include/asm/cfi.h    | 2 +-
>  arch/x86/kernel/alternative.c | 8 ++++----
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 4fff6ed46e90..d5cf52d2f6a8 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2424,6 +2424,15 @@ config STRICT_SIGALTSTACK_SIZE
>  
>  	  Say 'N' unless you want to really enforce this check.
>  
> +config CFI_AUTO_DEFAULT
> +	bool "Attempt to use FineIBT by default at boot time"
> +	depends on FINEIBT
> +	default y
> +	help
> +	  Attempt to use FineIBT by default at boot time. If enabled,
> +	  this is the same as booting with "cfi=auto". If disabled,
> +	  this is the same as booting with "cfi=kcfi".
> +
>  source "kernel/livepatch/Kconfig"
>  
>  endmenu
> diff --git a/arch/x86/include/asm/cfi.h b/arch/x86/include/asm/cfi.h
> index 7cd752557905..31d19c815f99 100644
> --- a/arch/x86/include/asm/cfi.h
> +++ b/arch/x86/include/asm/cfi.h
> @@ -93,7 +93,7 @@
>   *
>   */
>  enum cfi_mode {
> -	CFI_DEFAULT,	/* FineIBT if hardware has IBT, otherwise kCFI */
> +	CFI_AUTO,	/* FineIBT if hardware has IBT, otherwise kCFI */
>  	CFI_OFF,	/* Taditional / IBT depending on .config */
>  	CFI_KCFI,	/* Optionally CALL_PADDING, IBT, RETPOLINE */
>  	CFI_FINEIBT,	/* see arch/x86/kernel/alternative.c */
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 45a280f2161c..e8d0892d89cf 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -902,8 +902,8 @@ void __init_or_module apply_seal_endbr(s32 *start, s32 *end) { }
>  
>  #endif /* CONFIG_X86_KERNEL_IBT */
>  
> -#ifdef CONFIG_FINEIBT
> -#define __CFI_DEFAULT	CFI_DEFAULT
> +#ifdef CONFIG_CFI_AUTO_DEFAULT
> +#define __CFI_DEFAULT	CFI_AUTO
>  #elif defined(CONFIG_CFI_CLANG)
>  #define __CFI_DEFAULT	CFI_KCFI
>  #else
> @@ -1011,7 +1011,7 @@ static __init int cfi_parse_cmdline(char *str)
>  		}
>  
>  		if (!strcmp(str, "auto")) {
> -			cfi_mode = CFI_DEFAULT;
> +			cfi_mode = CFI_AUTO;
>  		} else if (!strcmp(str, "off")) {
>  			cfi_mode = CFI_OFF;
>  			cfi_rand = false;
> @@ -1271,7 +1271,7 @@ static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
>  		      "FineIBT preamble wrong size: %ld", fineibt_preamble_size))
>  		return;
>  
> -	if (cfi_mode == CFI_DEFAULT) {
> +	if (cfi_mode == CFI_AUTO) {
>  		cfi_mode = CFI_KCFI;
>  		if (HAS_KERNEL_IBT && cpu_feature_enabled(X86_FEATURE_IBT))
>  			cfi_mode = CFI_FINEIBT;
> -- 
> 2.34.1
>