linux-kernel - Re: [PATCH v11 25/39] arm64/signal: Expose GCS state in signal frames

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] [day] [month] [year] [list]

Message-ID: <4de41478-3bc8-4cb4-a557-3e9402afe858@sirena.org.uk>
Date: Wed, 28 Aug 2024 18:32:48 +0100
From: Mark Brown <broonie@...nel.org>
To: Catalin Marinas <catalin.marinas@....com>
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>,
	Yury Khrustalev <yury.khrustalev@....com>,
	Wilco Dijkstra <wilco.dijkstra@....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 v11 25/39] arm64/signal: Expose GCS state in signal frames

On Mon, Aug 26, 2024 at 01:00:09PM +0300, Catalin Marinas wrote:
> On Fri, Aug 23, 2024 at 11:01:13PM +0100, Mark Brown wrote:
> > On Fri, Aug 23, 2024 at 04:59:11PM +0100, Catalin Marinas wrote:

> gcs_preserve_current_state() only a context switch thing. Would it work
> if we don't touch the thread structure at all in the signal code? We
> wouldn't deliver a signal in the middle of the switch_to() code. So any
> value we write in thread struct would be overridden at the next switch.

I think so, yes.

> If GCS is disabled for a guest, we save the GCSPR_EL0 with the cap size

s/guest/task/ I guess?

> subtracted but there's no cap written. In restore_gcs_context() it
> doesn't look like we add the cap size back when writing GCSPR_EL0. If
> GCS is enabled, we do consume the cap and add 8 but otherwise it looks
> that we keep decreasing GCSPR_EL0. I think we should always subtract the
> cap size if GCS is enabled. This could could do with some refactoring as
> I find it hard to follow (not sure exactly how, maybe just comments will
> do).

I've changed this so we instead only add the frame for the token if GCS
is enabled and updated the comment, that way we don't modify GCSPR_EL0
in cases where GCS is not enabled.

> I'd also keep a single write to GCSPR_EL0 on the return path but I'm ok
> with two if we need to cope with GCS being disabled but the GCSPR_EL0
> still being saved/restored.

I think the handling for the various options in the second case mean
that it's clearer and simpler to write once when we restore the frame
and once when we consume the token.

> Another aspect for gcs_restore_signal(), I think it makes more sense for
> the cap to be consumed _after_ restoring the sigcontext since this has
> the actual gcspr_el0 where we stored the cap and represents the original
> stack. If we'll get an alternative shadow stack, current GCSPR_EL0 on
> sigreturn points to that alternative shadow stack rather than the
> original one. That's what confused me when reviewing the patch and I
> thought the cap goes to the top of the signal stack.

I've moved gcs_restore_signal() before the altstack restore which I
think is what you're looking for here?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)