linux-kernel - Re: [PATCH 4/6] arm64/signal: Consistently invalidate the in register FP state in restore

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: <Z086mm/qzNCRB2jH@e133380.arm.com>
Date: Tue, 3 Dec 2024 17:06:34 +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
Subject: Re: [PATCH 4/6] arm64/signal: Consistently invalidate the in
 register FP state in restore

Hi,

On Tue, Dec 03, 2024 at 04:53:11PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2024 at 03:34:01PM +0000, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 12:45:56PM +0000, Mark Brown wrote:
> > > When restoring the SVE and SME specific floating point register states we
> > > flush the task floating point state, marking the hardware state as stale so
> > > that preemption does not result in us saving register state from the signal
> 
> Now I think about it again this should probably be dropped from the
> series, or at least ordered after the reenablement.
> 
> > > +	 * thread floating point state with preemption enabled, so
> > > +	 * protection is needed to prevent a racing context switch
> > > +	 * from writing stale registers back over the new data. Mark
> > > +	 * the register floating point state as invalid and unbind the
> > > +	 * task from the CPU to force a reload before we return to
> > > +	 * userspace. fpsimd_flush_task_state() has a check for FP
> > > +	 * support.
> > > +	 */
> 
> > Maybe add a comment in fpsimd_flush_task_state() about why the
> > system_supports_fpsimd() check is important?  It's not obvious there
> > why we should ever be calling that function on non-FPSIMD systems.
> 
> There already is a comment in there about it?

There's a comment, but it's not clear that calling that function is
considered correct / useful if there is no FPSIMD.

Not a big deal, anyhow.

> > But would it be a good idea to stick a
> > WARN_ON(!test_thread_flag(TIF_FOREIGN_FPSTATE)) at the start of the
> > functions that rely on this?
> 
> As I mentioned in reply to one of your other messages I want to just gut
> the whole API here and replace it with get/put functions for the state
> which would include the get/put functions making sure they're paired
> with each other.

No argument from me on that, but it would be good to have a way to
check that functions that expect to be called with the FP context held,
actually are (similar to lockdep_assert_held() etc.)

If the number of affected functions is low, I guess comments may be
enough, though.

> 
> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.

Ack, but opinions can differ about what context is unneeded.

I'll try to keep the noise down on these threads.

Cheers
---Dave