linux-kernel - Re: [PATCH v10 19/40] arm64/gcs: Context switch GCS state for EL0

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: <ZsTNUYCMKgkrdRiS@arm.com>
Date: Tue, 20 Aug 2024 18:07:29 +0100
From: Catalin Marinas <catalin.marinas@....com>
To: Mark Brown <broonie@...nel.org>
Cc: Will Deacon <will@...nel.org>, Jonathan Corbet <corbet@....net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Marc Zyngier <maz@...nel.org>,
	Oliver Upton <oliver.upton@...ux.dev>,
	James Morse <james.morse@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Arnd Bergmann <arnd@...db.de>, Oleg Nesterov <oleg@...hat.com>,
	Eric Biederman <ebiederm@...ssion.com>,
	Shuah Khan <shuah@...nel.org>,
	"Rick P. Edgecombe" <rick.p.edgecombe@...el.com>,
	Deepak Gupta <debug@...osinc.com>, Ard Biesheuvel <ardb@...nel.org>,
	Szabolcs Nagy <Szabolcs.Nagy@....com>, Kees Cook <kees@...nel.org>,
	"H.J. Lu" <hjl.tools@...il.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>,
	Florian Weimer <fweimer@...hat.com>,
	Christian Brauner <brauner@...nel.org>,
	Thiago Jung Bauermann <thiago.bauermann@...aro.org>,
	Ross Burton <ross.burton@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org,
	kvmarm@...ts.linux.dev, linux-fsdevel@...r.kernel.org,
	linux-arch@...r.kernel.org, linux-mm@...ck.org,
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v10 19/40] arm64/gcs: Context switch GCS state for EL0

On Mon, Aug 19, 2024 at 04:44:42PM +0100, Mark Brown wrote:
> On Mon, Aug 19, 2024 at 12:46:13PM +0100, Catalin Marinas wrote:
> > On Thu, Aug 01, 2024 at 01:06:46PM +0100, Mark Brown wrote:
> > > +	/*
> > > +	 * Ensure that GCS changes are observable by/from other PEs in
> > > +	 * case of migration.
> > > +	 */
> > > +	if (task_gcs_el0_enabled(current) || task_gcs_el0_enabled(next))
> > > +		gcsb_dsync();
[...]
> > What's the GCSB DSYNC supposed to do here? The Arm ARM talks about
> > ordering between GCS memory effects and other memory effects. I haven't
> > looked at the memory model in detail yet (D11.9.1) but AFAICT it has
> > nothing to do with the system registers. We'll need this barrier when
> > ordering is needed between explicit or implicit (e.g. BL) GCS accesses
> > and the explicit classic memory accesses. Paging comes to mind, so maybe
> > flush_dcache_page() would need this barrier. ptrace() is another case if
> > the memory accessed is a GCS page. I can see you added it in other
> > places, I'll have a look as I go through the rest. But I don't think one
> > is needed here.
> 
> It's not particuarly for the system registers, is there's so that
> anything else that looks at the task's GCS sees the current state.

Ah, so that's the to ensure that any writes on the CPU to the GCS stack
would be observable if the task appears on a different CPU (together
with the additional classic ordering/spinlocks used for the run queues).
Maybe update the comment to say "GCS memory effects" instead of "GCS
changes". I read the latter as GCS sysreg changes. Something like below
would make it clearer:

	/*
	 * Ensure that GCS memory effects of the 'prev' thread are
	 * ordered before other memory accesses with release semantics
	 * (or preceded by a DMB) on the current PE. In addition, any
	 * memory accesses with acquire semantics (or succeeded by a
	 * DMB) are ordered before GCS memory effects of the 'next'
	 * thread. This will ensure that the GCS memory effects are
	 * visible to other PEs in case of migration.
	 */

Feel free to rephrase as you see fit.

> I'm pretty confident this excessive, the goal was to err on the side
> of correctness and then relax later.

I think we are missing some. Paging should be ok as we have a pte change
and TLBI and IIRC the same rules as for standard memory accesses apply.
ptrace() memory accesses may need something though I'm fine with
considering this a best effort (we can't guarantee anyway if any
accesses are on different CPUs). I haven't got to the signal handling
patch yet.

-- 
Catalin