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 <juna...@google.com>
> Signed-off-by: Junaid Shahid <juna...@google.com>
> Co-developed-by: Yosry Ahmed <yosryah...@google.com>
> Signed-off-by: Yosry Ahmed <yosryah...@google.com>
> Signed-off-by: Brendan Jackman <jackm...@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
