Re: [PATCH RFT v3 0/5] fork: Support shadow stacks in clone3()

From: Mark Brown <broonie@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Cc: Szabolcs Nagy <Szabolcs.Nagy@arm.com>,
	"Rick P. Edgecombe" <rick.p.edgecombe@intel.com>,
	Deepak Gupta <debug@rivosinc.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, David Hildenbrand <david@redhat.com>,
	nd@arm.com
Subject: Re: [PATCH RFT v3 0/5] fork: Support shadow stacks in clone3()
Date: Thu, 23 Nov 2023 11:37:54 +0000	[thread overview]
Message-ID: <ZV85khoUcFyKhQ+w@finisterre.sirena.org.uk> (raw)
In-Reply-To: <20231123-geflattert-mausklick-63d8ebcacffb@brauner>

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

On Thu, Nov 23, 2023 at 11:10:24AM +0100, Christian Brauner wrote:
> On Tue, Nov 21, 2023 at 04:09:40PM +0000, Mark Brown wrote:
> > On Tue, Nov 21, 2023 at 12:21:37PM +0000, Szabolcs Nagy wrote:
> > > The 11/21/2023 11:17, Christian Brauner wrote:

> > > > (2) With what other interfaces is implicit allocation and deallocation
> > > >     not consistent? I don't understand this argument. The kernel creates
> > > >     a shadow stack as a security measure to store return addresses. It
> > > >     seems to me exactly that the kernel should implicitly allocate and
> > > >     deallocate the shadow stack and not have userspace muck around with
> > > >     its size?

...

> > The inconsistency here is with the management of the standard stack -
> > with the standard stack userspace passes an already allocated address
> > range to the kernel.  A constant tension during review of the shadow
> > stack interfaces has been that shadow stack memory is userspace memory
> > but the security constraints mean that we've come down on the side of
> > having a custom allocation syscall for it instead of using flags on
> > mmap() and friends like people often expect, and now having it allocated
> > as part of clone3().  The aim is to highlight that this difference is

> So you have two interfaces for allocating a shadow stack. The first one
> is to explicitly alloc a shadow stack via the map_shadow_stack(). The
> second one is an implicit allocation during clone3() and you want to
> allow explicitly influencing that.

Yes.  Shadow stacks are also allocated when the inital call to enable
shadow stacks is done, the clone()/clone3() behaviour is to implicitly
allocate when a thread is created by a thread that itself has shadow
stacks enabled (so we avoid gaps in coverage).

> > feature (hence the RFT in the subject line) - Rick Edgecombe did the x86
> > work.  The arm64 code is still in review, the userspace interface is
> > very similar to that for x86 and there doesn't seem to be any
> > controversy there which makes me expect that a change is likely.  Unlike
> > x86 we only have a spec and virtual implementations at present, there's
> > no immintent hardware, so we're taking our time with making sure that
> > everything is working well.  Deepak Gupta (in CC) has been reviewing the
> > series from the point of view of RISC-V.  I think we're all fairly well
> > aligned on the requirements here.

> I'm still not enthusiastic that we only have one implementation for this
> in the kernel. What's the harm in waiting until the arm patches are
> merged? This shouldn't result in chicken and egg: if the implementations
> are sufficiently similar then we can do an appropriate clone3()
> extension.

The main thing would be that it would mean that people doing userspace
enablement based on the merged x86 support can't use the stack size
control.  It's not the end of the world if that has to wait a bit, it's
a bit of a detail thing, but it would make life easier, I guess the
userspace people can let us know if it's starting to be a real hassle
and we can reevaulate if that happens.

It's also currently a dependency for the arm64 code so it'd be good to
at least get ageement that assuming nothing comes up in testing the
patches can go in along with the arm64 series, removing the dependency
and then adding it as an incremental thing would be a hassle.  It's
likely that the arm64 series will be held out of tree for a while to as
more complete userspace support is built up and validated so things
might be sitting for a while - we don't have hardware right now so we
can be cautious with the testing.

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