linux-kernel - Re: [PATCH v2] arm64/signal: Avoid corruption of SME state when entering signal handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zyo3QU8aBGmtbTRo@J2N7QTR9R3.cambridge.arm.com>
Date: Tue, 5 Nov 2024 15:18:33 +0000
From: Mark Rutland <mark.rutland@....com>
To: Mark Brown <broonie@...nel.org>
Cc: Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] arm64/signal: Avoid corruption of SME state when
 entering signal handler

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.