[RFC PATCH] arm64/fpsimd: Implement strict mode for kernel mode SIMD

[RFC PATCH] arm64/fpsimd: Implement strict mode for kernel mode SIMD

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

The arch-specific code on arm64 that uses the FP/SIMD register file
takes great care to use FP/SIMD enabled C code only from isolated
compilation units. This is needed because that code is compiled
with the -mgeneral-regs-only compiler flag omitted, and this permits the
compiler to use FP/SIMD registers anywhere, including for things like
spilling. This could result in unrelated FP/SIMD state getting
corrupted, as that may only be manipulated from within a
kernel_neon_begin/end block.

The generic kernel mode FPU API, as used by the amdgpu driver, also
omits the -mgeneral-regs-only flag when building FP/SIMD code, but
without any guardrails, potentially resulting in user data corruption if
any of that code is reachable from outside of a kernel_fpu_begin/end
pair.

On arm64, this issue is more severe than on other architectures, as
access to the FP/SIMD registers is never disabled, and so such
corruption could happen silently.

So implement a strict FP/SIMD mode that does disable access to the
FP/SIMD registers, so that inadvertent accesses trap and cause an
exception.

Link: https://lore.kernel.org/all/20251002210044.1726731-2-ardb+git@google.com/
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/Kconfig               | 11 ++++++++
 arch/arm64/include/asm/fpsimd.h  | 29 +++++++++++++++++++
 arch/arm64/kernel/entry-common.c |  3 ++
 arch/arm64/kernel/fpsimd.c       | 48 +++++++++++++++++++++++---------
 4 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 84c7a455d16c..53162e13c0d4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -384,6 +384,17 @@ config SMP
 config KERNEL_MODE_NEON
 	def_bool y
 
+config STRICT_KERNEL_FPSIMD
+	bool "Enable strict checking of kernel mode FP/SIMD register accesses"
+	default DRM_AMDGPU
+	help
+	  Disable access to the FP/SIMD register file when entering the kernel,
+	  and only enable it temporarily when using it for kernel mode FP/SIMD,
+	  or for context switching the contents. This ensures that inadvertent
+	  accesses from code that fails to use the kernel NEON begin/end API
+	  properly will trigger an exception, instead of silently corrupting
+	  user data.
+
 config FIX_EARLYCON_MEM
 	def_bool y
 
diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index b8cf0ea43cc0..0681c081db79 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -57,6 +57,35 @@ static inline void cpacr_restore(unsigned long cpacr)
 	isb();
 }
 
+static inline void cpacr_enable_fpsimd(void)
+{
+	sysreg_clear_set(cpacr_el1, 0, CPACR_EL1_FPEN);
+	isb();
+}
+
+static inline void cpacr_disable_fpsimd(void)
+{
+	sysreg_clear_set(cpacr_el1, CPACR_EL1_FPEN, 0);
+	isb();
+}
+
+static inline bool fpsimd_kmode_strict(void)
+{
+	return IS_ENABLED(CONFIG_STRICT_KERNEL_FPSIMD);
+}
+
+static inline void fpsimd_kmode_enable_access(void)
+{
+	if (fpsimd_kmode_strict())
+		cpacr_enable_fpsimd();
+}
+
+static inline void fpsimd_kmode_disable_access(void)
+{
+	if (fpsimd_kmode_strict())
+		cpacr_disable_fpsimd();
+}
+
 /*
  * When we defined the maximum SVE vector length we defined the ABI so
  * that the maximum vector length included all the reserved for future
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index f546a914f041..3b83989c768c 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -21,6 +21,7 @@
 #include <asm/daifflags.h>
 #include <asm/esr.h>
 #include <asm/exception.h>
+#include <asm/fpsimd.h>
 #include <asm/irq_regs.h>
 #include <asm/kprobes.h>
 #include <asm/mmu.h>
@@ -88,6 +89,7 @@ static __always_inline void __enter_from_user_mode(struct pt_regs *regs)
 
 static __always_inline void arm64_enter_from_user_mode(struct pt_regs *regs)
 {
+	fpsimd_kmode_disable_access();
 	__enter_from_user_mode(regs);
 }
 
@@ -104,6 +106,7 @@ static __always_inline void arm64_exit_to_user_mode(struct pt_regs *regs)
 	local_daif_mask();
 	mte_check_tfsr_exit();
 	exit_to_user_mode();
+	fpsimd_kmode_enable_access();
 }
 
 asmlinkage void noinstr asm_exit_to_user_mode(struct pt_regs *regs)
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index e3f8f51748bc..c1c3a554b3e5 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -359,6 +359,9 @@ static void task_fpsimd_load(void)
 	WARN_ON(preemptible());
 	WARN_ON(test_thread_flag(TIF_KERNEL_FPSTATE));
 
+	/* Enable access so the FP/SIMD register contents can be loaded */
+	fpsimd_kmode_enable_access();
+
 	if (system_supports_sve() || system_supports_sme()) {
 		switch (current->thread.fp_type) {
 		case FP_STATE_FPSIMD:
@@ -432,7 +435,7 @@ static void task_fpsimd_load(void)
  * than via current, if we are saving KVM state then it will have
  * ensured that the type of registers to save is set in last->to_save.
  */
-static void fpsimd_save_user_state(void)
+static void fpsimd_save_user_state(bool leave_fpsimd_enabled)
 {
 	struct cpu_fp_state const *last =
 		this_cpu_ptr(&fpsimd_last_state);
@@ -447,6 +450,9 @@ static void fpsimd_save_user_state(void)
 	if (test_thread_flag(TIF_FOREIGN_FPSTATE))
 		return;
 
+	/* Enable access so the FP/SIMD register contents can be saved */
+	fpsimd_kmode_enable_access();
+
 	if (system_supports_fpmr())
 		*(last->fpmr) = read_sysreg_s(SYS_FPMR);
 
@@ -492,7 +498,7 @@ static void fpsimd_save_user_state(void)
 			 * There's no way to recover, so kill it:
 			 */
 			force_signal_inject(SIGKILL, SI_KERNEL, 0, 0);
-			return;
+			goto out;
 		}
 
 		sve_save_state((char *)last->sve_state +
@@ -503,6 +509,10 @@ static void fpsimd_save_user_state(void)
 		fpsimd_save_state(last->st);
 		*last->fp_type = FP_STATE_FPSIMD;
 	}
+
+out:
+	if (!leave_fpsimd_enabled)
+		fpsimd_kmode_disable_access();
 }
 
 /*
@@ -1114,10 +1124,10 @@ static void __init sve_efi_setup(void)
 
 void cpu_enable_sve(const struct arm64_cpu_capabilities *__always_unused p)
 {
-	write_sysreg(read_sysreg(CPACR_EL1) | CPACR_EL1_ZEN_EL1EN, CPACR_EL1);
-	isb();
+	unsigned long cpacr = cpacr_save_enable_kernel_sve();
 
 	write_sysreg_s(0, SYS_ZCR_EL1);
+	cpacr_restore(cpacr);
 }
 
 void __init sve_setup(void)
@@ -1543,10 +1553,11 @@ void fpsimd_thread_switch(struct task_struct *next)
 	if (test_thread_flag(TIF_KERNEL_FPSTATE))
 		fpsimd_save_kernel_state(current);
 	else
-		fpsimd_save_user_state();
+		fpsimd_save_user_state(true);
 
 	if (test_tsk_thread_flag(next, TIF_KERNEL_FPSTATE)) {
 		fpsimd_flush_cpu_state();
+		fpsimd_kmode_enable_access();
 		fpsimd_load_kernel_state(next);
 	} else {
 		/*
@@ -1561,6 +1572,7 @@ void fpsimd_thread_switch(struct task_struct *next)
 
 		update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE,
 				       wrong_task || wrong_cpu);
+		fpsimd_kmode_disable_access();
 	}
 }
 
@@ -1654,7 +1666,7 @@ void fpsimd_preserve_current_state(void)
 		return;
 
 	get_cpu_fpsimd_context();
-	fpsimd_save_user_state();
+	fpsimd_save_user_state(false);
 	put_cpu_fpsimd_context();
 }
 
@@ -1747,6 +1759,9 @@ void fpsimd_restore_current_state(void)
 		fpsimd_bind_task_to_cpu();
 	}
 
+	/* Disable access to the FP/SIMD registers until return to userland */
+	fpsimd_kmode_disable_access();
+
 	put_cpu_fpsimd_context();
 }
 
@@ -1793,7 +1808,7 @@ void fpsimd_save_and_flush_current_state(void)
 		return;
 
 	get_cpu_fpsimd_context();
-	fpsimd_save_user_state();
+	fpsimd_save_user_state(false);
 	fpsimd_flush_task_state(current);
 	put_cpu_fpsimd_context();
 }
@@ -1810,7 +1825,7 @@ void fpsimd_save_and_flush_cpu_state(void)
 		return;
 	WARN_ON(preemptible());
 	local_irq_save(flags);
-	fpsimd_save_user_state();
+	fpsimd_save_user_state(false);
 	fpsimd_flush_cpu_state();
 	local_irq_restore(flags);
 }
@@ -1848,7 +1863,7 @@ void kernel_neon_begin(void)
 		BUG_ON(IS_ENABLED(CONFIG_PREEMPT_RT) || !in_serving_softirq());
 		fpsimd_save_kernel_state(current);
 	} else {
-		fpsimd_save_user_state();
+		fpsimd_save_user_state(true);
 
 		/*
 		 * Set the thread flag so that the kernel mode FPSIMD state
@@ -1869,6 +1884,9 @@ void kernel_neon_begin(void)
 		 */
 		if (IS_ENABLED(CONFIG_PREEMPT_RT) || !in_serving_softirq())
 			set_thread_flag(TIF_KERNEL_FPSTATE);
+
+		/* Allow use of FP/SIMD until kernel_neon_end() is called */
+		fpsimd_kmode_enable_access();
 	}
 
 	/* Invalidate any task state remaining in the fpsimd regs: */
@@ -1900,8 +1918,10 @@ void kernel_neon_end(void)
 	if (!IS_ENABLED(CONFIG_PREEMPT_RT) && in_serving_softirq() &&
 	    test_thread_flag(TIF_KERNEL_FPSTATE))
 		fpsimd_load_kernel_state(current);
-	else
+	else {
 		clear_thread_flag(TIF_KERNEL_FPSTATE);
+		fpsimd_kmode_disable_access();
+	}
 }
 EXPORT_SYMBOL_GPL(kernel_neon_end);
 
@@ -1939,6 +1959,8 @@ void __efi_fpsimd_begin(void)
 	if (may_use_simd()) {
 		kernel_neon_begin();
 	} else {
+		fpsimd_kmode_enable_access();
+
 		/*
 		 * If !efi_sve_state, SVE can't be in use yet and doesn't need
 		 * preserving:
@@ -2020,6 +2042,7 @@ void __efi_fpsimd_end(void)
 		}
 
 		efi_fpsimd_state_used = false;
+		fpsimd_kmode_disable_access();
 	}
 }
 
@@ -2076,9 +2099,8 @@ static inline void fpsimd_hotplug_init(void) { }
 
 void cpu_enable_fpsimd(const struct arm64_cpu_capabilities *__always_unused p)
 {
-	unsigned long enable = CPACR_EL1_FPEN_EL1EN | CPACR_EL1_FPEN_EL0EN;
-	write_sysreg(read_sysreg(CPACR_EL1) | enable, CPACR_EL1);
-	isb();
+	if (!fpsimd_kmode_strict())
+		cpacr_enable_fpsimd();
 }
 
 /*
-- 
2.51.0.618.g983fd99d29-goog

Re: [RFC PATCH] arm64/fpsimd: Implement strict mode for kernel mode SIMD

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On Fri, Oct 03, 2025 at 06:21:21PM +0200, Ard Biesheuvel wrote:

> So implement a strict FP/SIMD mode that does disable access to the
> FP/SIMD registers, so that inadvertent accesses trap and cause an
> exception.

I like this idea.  However, my first thought when I stated looking
through the code was that it lines up with the idea I had to move the
way we access to the FP state (kernel_neon_*(), disabling preemption to
access userspace state in registers and flushing to memory) to use a
get/put model.  The idea was to make the patterns we use for taking
ownership of the FP state and the lifetime of that ownership more
consistent, and support validation like this.  It's a bigger change than
what you've done here but with the updates to manage the traps it'll be
touching a lot of the same places and I think it'd be really useful from
a maintainability point of view even if we don't actually end up doing
anything with the traps.

I'll have a proper look at the change next week.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]