Re: [PATCH RFT v3 2/5] fork: Add shadow stack support to clone3()

From: Mark Brown <broonie@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Cc: "Rick P. Edgecombe" <rick.p.edgecombe@intel.com>,
	Deepak Gupta <debug@rivosinc.com>,
	Szabolcs Nagy <Szabolcs.Nagy@arm.com>,
	"H.J. Lu" <hjl.tools@gmail.com>,
	Florian Weimer <fweimer@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Valentin Schneider <vschneid@redhat.com>,
	Shuah Khan <shuah@kernel.org>,
	linux-kernel@vger.kernel.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Kees Cook <keescook@chromium.org>,
	jannh@google.com, linux-kselftest@vger.kernel.org,
	linux-api@vger.kernel.org
Subject: Re: [PATCH RFT v3 2/5] fork: Add shadow stack support to clone3()
Date: Thu, 23 Nov 2023 12:17:19 +0000	[thread overview]
Message-ID: <ZV9Cz00vAKd7EwKD@finisterre.sirena.org.uk> (raw)
In-Reply-To: <20231123-derivate-freikarte-6de8984caf85@brauner>

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

On Thu, Nov 23, 2023 at 11:28:47AM +0100, Christian Brauner wrote:
> On Mon, Nov 20, 2023 at 11:54:30PM +0000, Mark Brown wrote:

> Any reasonably maximum that should be assumed here? IOW, what happens if
> userspace starts specifying 4G shadow_stack_size with each clone3() call
> for lolz?

I guess we could impose RLIMIT_STACK?

> > +	} else {
> > +		/*
> > +		 * For CLONE_VFORK the child will share the parents
> > +		 * shadow stack.  Make sure to clear the internal
> > +		 * tracking of the thread shadow stack so the freeing
> > +		 * logic run for child knows to leave it alone.
> > +		 */
> > +		if (clone_flags & CLONE_VFORK) {
> > +			shstk->base = 0;
> > +			shstk->size = 0;
> > +			return 0;
> > +		}

> Why is the CLONE_VFORK handling only necessary if shadow_stack_size is
> unset? In general, a comment or explanation on the interaction between
> CLONE_VFORK and shadow_stack_size would be helpful.

This is the existing implicit behaviour that clone() has, it's current
ABI for x86.  The intent is that if the user has explicitly configured a
shadow stack then we just do whatever they asked us to do, if they
didn't we try to guess something sensible.  The comment at the top of
this block when where we check if shadow_stack_size is set is intended
to capture this requirement:

	/*
	 * If the user specified a shadow stack then do some basic
	 * validation and use it, otherwise fall back to a default
	 * shadow stack size if the clone_flags don't indicate an
	 * allocation is unneeded.
	 */
	if (args->shadow_stack_size) {
		size = args->shadow_stack_size;

		if (size < 8)
			return (unsigned long)ERR_PTR(-EINVAL);
	} else {

I'm not immediately seeing somewhere else to add something?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]