linux-kernel - Re: [PATCH 5/6] 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: <Z087f9lkTBPFyOzA@e133380.arm.com>
Date: Tue, 3 Dec 2024 17:10:23 +0000
From: Dave Martin <Dave.Martin@....com>
To: Mark Brown <broonie@...nel.org>
Cc: Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH 5/6] arm64/signal: Avoid corruption of SME state when
 entering signal handler

Hi,

On Tue, Dec 03, 2024 at 04:12:33PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2024 at 03:33:18PM +0000, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 12:45:57PM +0000, Mark Brown wrote:
> 
> > > +	get_cpu_fpsimd_context();
> 
> > > +		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 ... -> [*]
> 
> Yes, we could drop the lock here.  OTOH this is very simple and easy to
> understand.

Ack; it works either way.

Since this is a Fixes: patch, it may be better to keep it simple.

> 
> > > +		/* 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?)
> 
> I have a half finished series to replace the whole setup around
> accessing the state with get/put operations for working on the state
> which should remove all these functions.  The pile of similarly and
> confusingly named operations we have for working on the state is one of
> the major sources of issues with this code, even when actively working
> on the code it's hard to remember exactly which operation does what
> never mind the rules for which is needed.

Sure, something like that would definitely help.

Cheers
---Dave