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

From: Borislav Petkov
Date: Wed Mar 19 2025 - 13:31:57 EST

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@xxxxxxxxxx>
> Signed-off-by: Junaid Shahid <junaids@xxxxxxxxxx>
> Co-developed-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
> ---

... 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