Re: [PATCH 5/6] arm64/signal: Avoid corruption of SME state when entering signal handler

[Dave Martin]

On Tue, Dec 03, 2024 at 12:45:57PM +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:
> 
>  * 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".
> 
> While we're at it also fix a discrepancy between in register and in memory
> entries. When operating on the register state we issue a SMSTOP, exiting
> streaming mode if we were in it. This clears the V/Z and P register and
> FPMR but nothing else. The in memory version clears all the user FPSIMD
> state including FPCR and FPSR but does not clear FPMR. Add the clear of
> FPMR and limit the existing memset() to only cover the vregs, preserving
> the state of FPCR and FPSR like SMSTOP does.
> 
> Fixes: 40a8e87bb3285 ("arm64/sme: Disable ZA and streaming mode when handling signals")
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  arch/arm64/include/asm/fpsimd.h |  1 +
>  arch/arm64/kernel/fpsimd.c      | 39 +++++++++++++++++++++++++++++++++++++++
>  arch/arm64/kernel/signal.c      | 19 +------------------
>  3 files changed, 41 insertions(+), 18 deletions(-)
> 
> 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 f02762762dbcf954e9add6dfd3575ae7055b6b0e..c5465c8ec467cb1ab8bd211dc5370f91aa2bcf35 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1696,6 +1696,45 @@ 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 V/Z and P
> +		 * registers and FPMR.  Zero FPMR and the V registers,
> +		 * marking the state as FPSIMD only to force a clear
> +		 * of the remaining bits during reload if needed.
> +		 */
> +		if (current->thread.svcr & SVCR_SM_MASK) {
> +			memset(&current->thread.uw.fpsimd_state.vregs, 0,
> +			       sizeof(current->thread.uw.fpsimd_state.vregs));

Do we need to hold the CPU fpsimd context across this memset?

IIRC, TIF_FOREIGN_FPSTATE can be spontaneously cleared along with
dumping of the regs into thread_struct (from current's PoV), but never
spontaneously set again.  So ... -> [*]

> +			current->thread.uw.fpmr = 0;
> +			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);

(This is very similar to fpsimd_flush_thread(); can they be unified?)

> +	} else {
> +		/* The register state is current, just update it. */
> +		sme_smstop();

... [*] the critical thing seems to be that the CPU fpsimd context is
held from the test on TIF_FOREIGN_FPSTATE, across this else clause.

(Whether or not this is a worthwhile optimisation is another matter.
But if the behaviour of TIF_FOREIGN_FPSTATE is still the same, then it
may be a good idea to avoid sending mixed messages about this in the
code.)

(A similar argument applies in fpsimd_flush_thread().)

[...]

Cheers
---Dave