diff mbox series

[v2] arm64/signal: Avoid corruption of SME state when entering signal handler

Message ID 20241030-arm64-fp-sme-sigentry-v2-1-43ce805d1b20@kernel.org (mailing list archive)
State New
Headers show
Series [v2] arm64/signal: Avoid corruption of SME state when entering signal handler | expand

Commit Message

Mark Brown Oct. 30, 2024, 7:58 p.m. UTC 
We intend that signal handlers are entered with PSTATE.{SM,ZA}={0,0}.
The logic for this in setup_return() manipulates the saved state and
live CPU state in an unsafe manner, and consequently, when a task enters
a signal handler:

 * The task entering the signal handler might not have its PSTATE.{SM,ZA}
   bits cleared, and other register state that is affected by changes to
   PSTATE.{SM,ZA} might not be zeroed as expected.

 * An unrelated task might have its PSTATE.{SM,ZA} bits cleared
   unexpectedly, potentially zeroing other register state that is
   affected by changes to PSTATE.{SM,ZA}.

   Tasks which do not set PSTATE.{SM,ZA} (i.e. those only using plain
   FPSIMD or non-streaming SVE) are not affected, as there is no
   resulting change to PSTATE.{SM,ZA}.

Consider for example two tasks on one CPU:

 A: Begins signal entry in kernel mode, is preempted prior to SMSTOP.
 B: Using SM and/or ZA in userspace with register state current on the
    CPU, is preempted.
 A: Scheduled in, no register state changes made as in kernel mode.
 A: Executes SMSTOP, modifying live register state.
 A: Scheduled out.
 B: Scheduled in, fpsimd_thread_switch() sees the register state on the
    CPU is tracked as being that for task B so the state is not reloaded
    prior to returning to userspace.

Task B is now running with SM and ZA incorrectly cleared.

Fix this by:

 * Checking TIF_FOREIGN_FPSTATE, and only updating the saved or live
   state as appropriate.

 * Using {get,put}_cpu_fpsimd_context() to ensure mutual exclusion
   against other code which manipulates this state. To allow their use,
   the logic is moved into a new fpsimd_enter_sighandler() helper in
   fpsimd.c.

This race has been observed intermittently with fp-stress, especially
with preempt disabled, commonly but not exclusively reporting "Bad SVCR: 0".

Fixes: 40a8e87bb3285 ("arm64/sme: Disable ZA and streaming mode when handling signals")
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
Changes in v2:
- Commit message tweaks.
- Flush the task state when updating in memory to ensure we reload.
- Link to v1: https://lore.kernel.org/r/20241023-arm64-fp-sme-sigentry-v1-1-249ff7ec3ad0@kernel.org
---
 arch/arm64/include/asm/fpsimd.h |  1 +
 arch/arm64/kernel/fpsimd.c      | 33 +++++++++++++++++++++++++++++++++
 arch/arm64/kernel/signal.c      | 19 +------------------
 3 files changed, 35 insertions(+), 18 deletions(-)


---
base-commit: 8e929cb546ee42c9a61d24fae60605e9e3192354
change-id: 20241023-arm64-fp-sme-sigentry-a2bd7187e71b

Best regards,

Comments

Mark Rutland Nov. 5, 2024, 3:18 p.m. UTC | #1 
On Wed, Oct 30, 2024 at 07:58:36PM +0000, Mark Brown wrote:
> We intend that signal handlers are entered with PSTATE.{SM,ZA}={0,0}.
> The logic for this in setup_return() manipulates the saved state and
> live CPU state in an unsafe manner, and consequently, when a task enters
> a signal handler:

Looking at this, I think there's a bigger question as to what we
actually intend here; explanation below, with two possible answers at
the end.

[...] 

> +/*
> + * Called by the signal handling code when preparing current to enter
> + * a signal handler. Currently this only needs to take care of exiting
> + * streaming mode and clearing ZA on SME systems.
> + */
> +void fpsimd_enter_sighandler(void)
> +{
> +	if (!system_supports_sme())
> +		return;
> +
> +	get_cpu_fpsimd_context();
> +
> +	if (test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> +		/* Exiting streaming mode zeros the FPSIMD state */
> +		if (current->thread.svcr & SVCR_SM_MASK) {
> +			memset(&current->thread.uw.fpsimd_state, 0,
> +			       sizeof(current->thread.uw.fpsimd_state));
> +			current->thread.fp_type = FP_STATE_FPSIMD;
> +		}
> +
> +		current->thread.svcr &= ~(SVCR_ZA_MASK |
> +					  SVCR_SM_MASK);
> +
> +		/* Ensure any copies on other CPUs aren't reused */
> +		fpsimd_flush_task_state(current);
> +	} else {
> +		/* The register state is current, just update it. */
> +		sme_smstop();
> +	}

I don't think that the foreign / non-foreign cases are equivalent. In
the foreign case we clear the entire fpsimd_state structure, i.e. all
of:

	struct user_fpsimd_state {
		__uint128_t     vregs[32];
		__u32           fpsr;
		__u32           fpcr;
		__u32           __reserved[2];
	};

Looking at the latest ARM ARM (ARM DDI 0487K.a):

  https://developer.arm.com/documentation/ddi0487/ka/

... the descriptions for FPSR and FPCR say nothing about exiting
streaming mode, and rule RKFRQZ says:

| When the Effective value of PSTATE.SM is changed by any method from 1
| to 0, an exit from Streaming SVE mode is performed, and in the
| newly-entered mode, all implemented bits of the SVE scalable vector
| registers, SVE predicate registers, and FFR, are set to zero.	

... which doesn't say anything about FPSR or FPCR, so from the ARM ARM
it doesn't look like SMSTOP will clobber those.

Looking at the latest "Arm A-profile Architecture Registers" document
(ARM DDI 061 2024-09):

  https://developer.arm.com/documentation/ddi0601/2024-09/

... the description of FPCR says nothing about exiting streaming mode,
so it appears to be preserved.

... the description of FPMR (which is not in the latest ARM ARM) says:

| On entry to or exit from Streaming SVE mode, FPMR is set to 0.

... so we'd need code to clobber that.

... and the description of FPSR says:

| On entry to or exit from Streaming SVE mode, FPSR.{IOC, DZC, OFC, UFC,
| IXC, IDC, QC} are set to 1 and the remaining bits are set to 0.

... so we'd need something more elaborate.

AFAICT either:

(a) Our intended ABI is that signal handlers are entered as-if an SMSTOP
    is executed to exit streaming mode and disable ZA storage.

    In this case we'll need a more elaborate sequence here to simulate
    that effect.

(b) Our intended ABI is that signal handlers are entered with
    PSTATE.{SM,ZA} cleared, FPSR cleared, FPCR cleared, and FPMR
    preserved.

    In this case we cannot use SMSTOP in the non-foreign case, and it
    would be simplest to always save the value back to memory and
    manipulate it there.

Our documentation in Documentation/arch/arm64/sme.rst says:

| Signal handlers are invoked with streaming mode and ZA disabled.

... and doesn't mention FPCR/FPMR/FPSR, so we could go either way,
though I suspect we intended case (a) ?

Mark.
Mark Brown Nov. 5, 2024, 3:55 p.m. UTC | #2 
On Tue, Nov 05, 2024 at 03:18:33PM +0000, Mark Rutland wrote:

> I don't think that the foreign / non-foreign cases are equivalent. In
> the foreign case we clear the entire fpsimd_state structure, i.e. all
> of:

You're right, they're not - thanks for spotting this.

> AFAICT either:

> (a) Our intended ABI is that signal handlers are entered as-if an SMSTOP
>     is executed to exit streaming mode and disable ZA storage.
> 
>     In this case we'll need a more elaborate sequence here to simulate
>     that effect.

That's the intention, so we do need to just clear the vregs instead of
the whole user_fpsimd_state and add clearing of FPMR.

> ... the description of FPMR (which is not in the latest ARM ARM) says:

> | On entry to or exit from Streaming SVE mode, FPMR is set to 0.

> ... so we'd need code to clobber that.

Right, that was missed with the addition of FPMR support.  We'll have
the same thing in ptrace streaming mode enter/exits, FPCR and FPSR
should be better there as in most cases register state is provided when
changing mode.

> Our documentation in Documentation/arch/arm64/sme.rst says:

> | Signal handlers are invoked with streaming mode and ZA disabled.

> ... and doesn't mention FPCR/FPMR/FPSR, so we could go either way,
> though I suspect we intended case (a) ?

Yes.  The the intended goal is literally just that, but if we accomplish
it by issuing a SMSTOP in the live registers case (which is the only
reasonable implementation) then we should obviously behave the same in
the live memory case.  I'll add a patch which makes this explicit in the
documentation.
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index f2a84efc361858d4deda99faf1967cc7cac386c1..09af7cfd9f6c2cec26332caa4c254976e117b1bf 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -76,6 +76,7 @@  extern void fpsimd_load_state(struct user_fpsimd_state *state);
 extern void fpsimd_thread_switch(struct task_struct *next);
 extern void fpsimd_flush_thread(void);
 
+extern void fpsimd_enter_sighandler(void);
 extern void fpsimd_signal_preserve_current_state(void);
 extern void fpsimd_preserve_current_state(void);
 extern void fpsimd_restore_current_state(void);
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 77006df20a75aee7c991cf116b6d06bfe953d1a4..c4149f474ce889af42bc2ce9402e7d032818c2e4 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1693,6 +1693,39 @@  void fpsimd_signal_preserve_current_state(void)
 		sve_to_fpsimd(current);
 }
 
+/*
+ * Called by the signal handling code when preparing current to enter
+ * a signal handler. Currently this only needs to take care of exiting
+ * streaming mode and clearing ZA on SME systems.
+ */
+void fpsimd_enter_sighandler(void)
+{
+	if (!system_supports_sme())
+		return;
+
+	get_cpu_fpsimd_context();
+
+	if (test_thread_flag(TIF_FOREIGN_FPSTATE)) {
+		/* Exiting streaming mode zeros the FPSIMD state */
+		if (current->thread.svcr & SVCR_SM_MASK) {
+			memset(&current->thread.uw.fpsimd_state, 0,
+			       sizeof(current->thread.uw.fpsimd_state));
+			current->thread.fp_type = FP_STATE_FPSIMD;
+		}
+
+		current->thread.svcr &= ~(SVCR_ZA_MASK |
+					  SVCR_SM_MASK);
+
+		/* Ensure any copies on other CPUs aren't reused */
+		fpsimd_flush_task_state(current);
+	} else {
+		/* The register state is current, just update it. */
+		sme_smstop();
+	}
+
+	put_cpu_fpsimd_context();
+}
+
 /*
  * Called by KVM when entering the guest.
  */
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 5619869475304776fc005fe24a385bf86bfdd253..fe07d0bd9f7978d73973f07ce38b7bdd7914abb2 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -1218,24 +1218,7 @@  static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
 	/* TCO (Tag Check Override) always cleared for signal handlers */
 	regs->pstate &= ~PSR_TCO_BIT;
 
-	/* Signal handlers are invoked with ZA and streaming mode disabled */
-	if (system_supports_sme()) {
-		/*
-		 * If we were in streaming mode the saved register
-		 * state was SVE but we will exit SM and use the
-		 * FPSIMD register state - flush the saved FPSIMD
-		 * register state in case it gets loaded.
-		 */
-		if (current->thread.svcr & SVCR_SM_MASK) {
-			memset(&current->thread.uw.fpsimd_state, 0,
-			       sizeof(current->thread.uw.fpsimd_state));
-			current->thread.fp_type = FP_STATE_FPSIMD;
-		}
-
-		current->thread.svcr &= ~(SVCR_ZA_MASK |
-					  SVCR_SM_MASK);
-		sme_smstop();
-	}
+	fpsimd_enter_sighandler();
 
 	if (system_supports_poe())
 		write_sysreg_s(POR_EL0_INIT, SYS_POR_EL0);