Re: [PATCH RFC v2 04/29] mm: asi: Add infrastructure for boot-time enablement

[Borislav Petkov <bp@alien8.de>]

On Fri, Jan 10, 2025 at 06:40:30PM +0000, Brendan Jackman wrote:
> Add a boot time parameter to control the newly added X86_FEATURE_ASI.
> "asi=on" or "asi=off" can be used in the kernel command line to enable
> or disable ASI at boot time. If not specified, ASI enablement depends
> on CONFIG_ADDRESS_SPACE_ISOLATION_DEFAULT_ON, which is off by default.

I don't know yet why we need this default-on thing...

> asi_check_boottime_disable() is modeled after
> pti_check_boottime_disable().
> 
> The boot parameter is currently ignored until ASI is fully functional.
> 
> Once we have a set of ASI features checked in that we have actually
> tested, we will stop ignoring the flag. But for now let's just add the
> infrastructure so we can implement the usage code.
> 
> Ignoring checkpatch.pl CONFIG_DESCRIPTION because the _DEFAULT_ON
> Kconfig is trivial to explain.

Those last two paragraphs go...

> Checkpatch-args: --ignore CONFIG_DESCRIPTION
> Co-developed-by: Junaid Shahid <junaids@google.com>
> Signed-off-by: Junaid Shahid <junaids@google.com>
> Co-developed-by: Yosry Ahmed <yosryahmed@google.com>
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---

... here as that's text not really pertaining to the contents of the patch.

>  arch/x86/Kconfig                         |  9 +++++
>  arch/x86/include/asm/asi.h               | 19 ++++++++--
>  arch/x86/include/asm/cpufeatures.h       |  1 +
>  arch/x86/include/asm/disabled-features.h |  8 ++++-
>  arch/x86/mm/asi.c                        | 61 +++++++++++++++++++++++++++-----
>  arch/x86/mm/init.c                       |  4 ++-
>  include/asm-generic/asi.h                |  4 +++
>  7 files changed, 92 insertions(+), 14 deletions(-)

...

>   * the N ASI classes.
>   */
>  
> +#define static_asi_enabled() cpu_feature_enabled(X86_FEATURE_ASI)

Yeah, as already mentioned somewhere else, whack that thing pls.

> +
>  /*
>   * ASI uses a per-CPU tainting model to track what mitigation actions are
>   * required on domain transitions. Taints exist along two dimensions:
> @@ -131,6 +134,8 @@ struct asi {
>  
>  DECLARE_PER_CPU_ALIGNED(struct asi *, curr_asi);
>  
> +void asi_check_boottime_disable(void);
> +
>  void asi_init_mm_state(struct mm_struct *mm);
>  
>  int asi_init_class(enum asi_class_id class_id, struct asi_taint_policy *taint_policy);
> @@ -155,7 +160,9 @@ void asi_exit(void);
>  /* The target is the domain we'll enter when returning to process context. */
>  static __always_inline struct asi *asi_get_target(struct task_struct *p)
>  {
> -	return p->thread.asi_state.target;
> +	return static_asi_enabled()
> +	       ? p->thread.asi_state.target
> +	       : NULL;

Waaay too fancy for old people:

	if ()
		return...
	else
		return NULL;

:-)

The others too pls.

>  static __always_inline void asi_set_target(struct task_struct *p,
> @@ -166,7 +173,9 @@ static __always_inline void asi_set_target(struct task_struct *p,
>  
>  static __always_inline struct asi *asi_get_current(void)
>  {
> -	return this_cpu_read(curr_asi);
> +	return static_asi_enabled()
> +	       ? this_cpu_read(curr_asi)
> +	       : NULL;
>  }
>  
>  /* Are we currently in a restricted address space? */
> @@ -175,7 +184,11 @@ static __always_inline bool asi_is_restricted(void)
>  	return (bool)asi_get_current();
>  }
>  
> -/* If we exit/have exited, can we stay that way until the next asi_enter? */
> +/*
> + * If we exit/have exited, can we stay that way until the next asi_enter?

What is that supposed to mean here?

> + *
> + * When ASI is disabled, this returns true.
> + */
>  static __always_inline bool asi_is_relaxed(void)
>  {
>  	return !asi_get_target(current);
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 913fd3a7bac6506141de65f33b9ee61c615c7d7d..d6a808d10c3b8900d190ea01c66fc248863f05e2 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -474,6 +474,7 @@
>  #define X86_FEATURE_CLEAR_BHB_HW	(21*32+ 3) /* BHI_DIS_S HW control enabled */
>  #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* Clear branch history at vmexit using SW loop */
>  #define X86_FEATURE_FAST_CPPC		(21*32 + 5) /* AMD Fast CPPC */
> +#define X86_FEATURE_ASI			(21*32+6) /* Kernel Address Space Isolation */
>  
>  /*
>   * BUG word(s)
> diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h
> index c492bdc97b0595ec77f89dc9b0cefe5e3e64be41..c7964ed4fef8b9441e1c0453da587787d8008d9d 100644
> --- a/arch/x86/include/asm/disabled-features.h
> +++ b/arch/x86/include/asm/disabled-features.h
> @@ -50,6 +50,12 @@
>  # define DISABLE_PTI		(1 << (X86_FEATURE_PTI & 31))
>  #endif
>  
> +#ifdef CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION
> +# define DISABLE_ASI		0
> +#else
> +# define DISABLE_ASI		(1 << (X86_FEATURE_ASI & 31))
> +#endif
> +
>  #ifdef CONFIG_MITIGATION_RETPOLINE
>  # define DISABLE_RETPOLINE	0
>  #else
> @@ -154,7 +160,7 @@
>  #define DISABLED_MASK17	0
>  #define DISABLED_MASK18	(DISABLE_IBT)
>  #define DISABLED_MASK19	(DISABLE_SEV_SNP)
> -#define DISABLED_MASK20	0
> +#define DISABLED_MASK20	(DISABLE_ASI)
>  #define DISABLED_MASK21	0
>  #define DISABLED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 22)
>  

Right, that hunk is done this way now:

diff --git a/arch/x86/Kconfig.cpufeatures b/arch/x86/Kconfig.cpufeatures
index e12d5b7e39a2..f219eaf664fb 100644
--- a/arch/x86/Kconfig.cpufeatures
+++ b/arch/x86/Kconfig.cpufeatures
@@ -199,3 +199,7 @@ config X86_DISABLED_FEATURE_SEV_SNP
 config X86_DISABLED_FEATURE_INVLPGB
 	def_bool y
 	depends on !BROADCAST_TLB_FLUSH
+
+config X86_DISABLED_FEATURE_ASI
+	def_bool y
+	depends on !MITIGATION_ADDRESS_SPACE_ISOLATION


> diff --git a/arch/x86/mm/asi.c b/arch/x86/mm/asi.c
> index 105cd8b43eaf5c20acc80d4916b761559fb95d74..5baf563a078f5b3a6cd4b9f5e92baaf81b0774c4 100644
> --- a/arch/x86/mm/asi.c
> +++ b/arch/x86/mm/asi.c
> @@ -4,6 +4,7 @@
>  #include <linux/percpu.h>
>  #include <linux/spinlock.h>
>  
> +#include <linux/init.h>
>  #include <asm/asi.h>
>  #include <asm/cmdline.h>
>  #include <asm/cpufeature.h>
> @@ -29,6 +30,9 @@ static inline bool asi_class_id_valid(enum asi_class_id class_id)
>  
>  static inline bool asi_class_initialized(enum asi_class_id class_id)
>  {
> +	if (!boot_cpu_has(X86_FEATURE_ASI))

check_for_deprecated_apis: WARNING: arch/x86/mm/asi.c:33: Do not use boot_cpu_has() - use cpu_feature_enabled() instead.

Check your whole set pls.

> +		return 0;
> +
>  	if (WARN_ON(!asi_class_id_valid(class_id)))
>  		return false;
>  
> @@ -51,6 +55,9 @@ EXPORT_SYMBOL_GPL(asi_init_class);
>  
>  void asi_uninit_class(enum asi_class_id class_id)
>  {
> +	if (!boot_cpu_has(X86_FEATURE_ASI))
> +		return;
> +
>  	if (!asi_class_initialized(class_id))
>  		return;
>  
> @@ -66,10 +73,36 @@ const char *asi_class_name(enum asi_class_id class_id)
>  	return asi_class_names[class_id];
>  }
>  
> +void __init asi_check_boottime_disable(void)
> +{
> +	bool enabled = IS_ENABLED(CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION_DEFAULT_ON);
> +	char arg[4];
> +	int ret;
> +
> +	ret = cmdline_find_option(boot_command_line, "asi", arg, sizeof(arg));
> +	if (ret == 3 && !strncmp(arg, "off", 3)) {
> +		enabled = false;
> +		pr_info("ASI disabled through kernel command line.\n");
> +	} else if (ret == 2 && !strncmp(arg, "on", 2)) {
> +		enabled = true;
> +		pr_info("Ignoring asi=on param while ASI implementation is incomplete.\n");
> +	} else {
> +		pr_info("ASI %s by default.\n",
> +			enabled ? "enabled" : "disabled");
> +	}
> +
> +	if (enabled)
> +		pr_info("ASI enablement ignored due to incomplete implementation.\n");

Incomplete how?

> +}
> +
>  static void __asi_destroy(struct asi *asi)
>  {
> -	lockdep_assert_held(&asi->mm->asi_init_lock);
> +	WARN_ON_ONCE(asi->ref_count <= 0);
> +	if (--(asi->ref_count) > 0)

Switch that to

include/linux/kref.h

It gives you a sanity-checking functionality too so you don't need the WARN...

> +		return;
>  
> +	free_pages((ulong)asi->pgd, PGD_ALLOCATION_ORDER);
> +	memset(asi, 0, sizeof(struct asi));

And then you can do:

	if (kref_put())
		free_pages...

and so on.

Thx.

-- 
Regards/Gruss,
    Boris.

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