linux-kernel - Re: [PATCH RFT v8 4/9] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zrx7Lj09b99ozgAE@arm.com>
Date: Wed, 14 Aug 2024 10:38:54 +0100
From: Catalin Marinas <catalin.marinas@....com>
To: Mark Brown <broonie@...nel.org>
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,
	linux-kselftest@...r.kernel.org, linux-api@...r.kernel.org,
	Kees Cook <kees@...nel.org>
Subject: Re: [PATCH RFT v8 4/9] fork: Add shadow stack support to clone3()

On Tue, Aug 13, 2024 at 07:58:26PM +0100, Mark Brown wrote:
> On Tue, Aug 13, 2024 at 05:25:47PM +0100, Catalin Marinas wrote:
> > However, the x86 would be slightly inconsistent here between clone() and
> > clone3(). I guess it depends how you look at it. The classic clone()
> > syscall, if one doesn't pass CLONE_VM but does set new stack, there's no
> > new shadow stack allocated which I'd expect since it's a new stack.
> > Well, I doubt anyone cares about this scenario. Are there real cases of
> > !CLONE_VM but with a new stack?
> 
> ISTR the concerns were around someone being clever with vfork() but I
> don't remember anything super concrete.  In terms of the inconsistency
> here that was actually another thing that came up - if userspace
> specifies a stack for clone3() it'll just get used even with CLONE_VFORK
> so it seemed to make sense to do the same thing for the shadow stack.
> This was part of the thinking when we were looking at it, if you can
> specify a regular stack you should be able to specify a shadow stack.

Yes, I agree. But by this logic, I was wondering why the current clone()
behaviour does not allocate a shadow stack when a new stack is
requested with CLONE_VFORK. That's rather theoretical though and we may
not want to change the ABI.

> > > > I'm confused that we need to consume the token here. I could not find
> > > > the default shadow stack allocation doing this, only setting it via
> > > > create_rstor_token() (or I did not search enough). In the default case,
> 
> > > As discussed for a couple of previous versions if we don't have the
> > > token and userspace can specify any old shadow stack page as the shadow
> > > stack this allows clone3() to be used to overwrite the shadow stack of
> > > another thread, you can point to a shadow stack page which is currently
> 
> > IIUC, the kernel-allocated shadow stack will have the token always set
> > while the user-allocated one will be cleared. I was looking to
> 
> No, when the kernel allocates we don't bother with tokens at all.  We
> only look for and clear a token with the user specified shadow stack.

Ah, you are right, I misread the alloc_shstk() function. It takes a
set_res_tok parameter which is false for the normal allocation.

> > I guess I was rather questioning the current choices than the new
> > clone3() ABI. But even for the new clone3() ABI, does it make sense to
> > set up a shadow stack if the current stack isn't changed? We'll end up
> > with a lot of possible combinations that will never get tested but
> > potentially become obscure ABI. Limiting the options to the sane choices
> > only helps with validation and unsurprising changes later on.
> 
> OTOH if we add the restrictions it's more code (and more test code) to
> check, and thinking about if we've missed some important use case.  Not
> that it's a *huge* amount of code, like I say I'd not be too unhappy
> with adding a restriction on having a regular stack specified in order
> to specify a shadow stack.

I guess we just follow the normal stack behaviour for clone3(), at least
we'd be consistent with that.

Anyway, I understood this patch now and the ABI decisions. FWIW:

Reviewed-by: Catalin Marinas <catalin.marinas@....com>