linux-kernel - Re: [PATCH RFT v17 4/8] fork: Add shadow stack support to clone3()

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: <97702623-0a1e-4231-9550-79aaa9d41fac@sirena.org.uk>
Date: Fri, 27 Jun 2025 22:31:05 +0100
From: Mark Brown <broonie@...nel.org>
To: Catalin Marinas <catalin.marinas@....com>
Cc: "Rick P. Edgecombe" <rick.p.edgecombe@...el.com>,
	Deepak Gupta <debug@...osinc.com>,
	Szabolcs Nagy <Szabolcs.Nagy@....com>,
	"H.J. Lu" <hjl.tools@...il.com>,
	Florian Weimer <fweimer@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Valentin Schneider <vschneid@...hat.com>,
	Christian Brauner <brauner@...nel.org>,
	Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
	Will Deacon <will@...nel.org>, jannh@...gle.com,
	Yury Khrustalev <yury.khrustalev@....com>,
	Wilco Dijkstra <wilco.dijkstra@....com>,
	linux-kselftest@...r.kernel.org, linux-api@...r.kernel.org,
	Kees Cook <kees@...nel.org>
Subject: Re: [PATCH RFT v17 4/8] fork: Add shadow stack support to clone3()

On Fri, Jun 27, 2025 at 06:19:33PM +0100, Catalin Marinas wrote:
> On Mon, Jun 09, 2025 at 01:54:05PM +0100, Mark Brown wrote:

> > +	/* Ensure that a token written as a result of a pivot is visible */
> > +	gcsb_dsync();
> > +	gcspr_el0 = args->shadow_stack_token;
> > +	if (!gcs_consume_token(vma, page, gcspr_el0))
> > +		return -EINVAL;
> > +
> > +	tsk->thread.gcspr_el0 = gcspr_el0 + sizeof(u64);
> > +
> > +	/* Ensure that our token consumption visible */
> > +	gcsb_dsync();

> What are the scenarios where we need the barriers? We have one via
> map_shadow_stack() that would cover the first one. IIUC, GCSSS2 also
> generates a GCSB event (or maybe I got it all wrong; I need to read the
> spec).

I think now that gcs_consume_token() does a cmpxchg they're redundant,
your analysis covers the first one (anything that puts a valid token
in memory should have a barrier) and now gcs_consume_token() does a
cmpxchg the second one should also be redundant thanks to R_FZRGP.  It
would be good if someone double checked though.

Originally gcs_consume_token() was using regular accesses as for the
example in DDI0487 L.a K3.3 and was tried on two addresses, I missed
dropping the barriers when changing to a cmpxchg.

> > +static int shstk_validate_clone(struct task_struct *p,
> > +				struct kernel_clone_args *args)
> > +{

> > +	mmap_read_lock(mm);

> > +	addr = untagged_addr_remote(mm, args->shadow_stack_token);

> I think down the line, get_user_page_vma_remote() already does an
> untagged_addr_remote(). But it does it after the vma look-up, so we
> still need the untagging early.

> That said, would we ever allowed a tagged pointer for the shadow stack?

For arm64 you can architecturally use tags as per G_HMJHM.  I_WBHHX says
that GCS accesses are tag unchecked, but tags are used on GCSSS1 as per
I_MGLTC and I_MBHFS.  We'll need new ABI to allow userspace to get a
PROT_MTE GCS though, I'd planned on extending map_shadow_stack() for
that, and adding handling in the token validation here.

There's also the fact that the untagging should be very cheap in the
context of what we're doing so it seems sensible to just have it,
especially generic code which applies to all arches.

> > +static inline bool clone3_shadow_stack_valid(struct kernel_clone_args *kargs)
> > +{
> > +	if (!kargs->shadow_stack_token)
> > +		return true;
> > +
> > +	if (!IS_ALIGNED(kargs->shadow_stack_token, sizeof(void *)))
> > +		return false;
> > +
> > +	/*
> > +	 * The architecture must check support on the specific
> > +	 * machine.
> > +	 */
> > +	return IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK);

> I don't understand the comment here. It implies some kind of fallback
> for further arch checks but it's just a return.

Here we're just doing initial triage that a shadow stack could possibly
be valid, the check here is there to fail if there's one specified but
there is no support in the kernel (eg, for architectures that don't have
the feature at all like arm32).  The comment is trying to say that we're
not attempting to validate that we can actually use shadow stacks on the
current system, just that the support exists in the kernel.  I'll reword
the comment, it's not clear.

> BTW, clone3_stack_valid() has an access_ok() check as well. Shall we add
> it here? That's where the size would have come in handy but IIUC the
> decision was to drop it (fine by me, just validate that the token is
> accessible).

AIUI the main reason for doing that for the normal stack is to report an
error before we actually start the thread and have it fault trying to
access an invalid stack since we don't otherwise look at the memory,
like you say with shadow stacks we'll consume the token before we start
the new thread so we get the equivalent error reporting as part of that.
I don't think the extra check would buy us much.

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