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

Message-ID: <cc2e7d86-c890-4cb1-8cad-1cfaa9f53dc8@sirena.org.uk>
Date: Wed, 21 Aug 2024 00:34:39 +0100
From: Mark Brown <broonie@...nel.org>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc: "dietmar.eggemann@....com" <dietmar.eggemann@....com>,
	"Szabolcs.Nagy@....com" <Szabolcs.Nagy@....com>,
	"brauner@...nel.org" <brauner@...nel.org>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"debug@...osinc.com" <debug@...osinc.com>,
	"mgorman@...e.de" <mgorman@...e.de>,
	"vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
	"fweimer@...hat.com" <fweimer@...hat.com>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"rostedt@...dmis.org" <rostedt@...dmis.org>,
	"hjl.tools@...il.com" <hjl.tools@...il.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"vschneid@...hat.com" <vschneid@...hat.com>,
	"shuah@...nel.org" <shuah@...nel.org>,
	"hpa@...or.com" <hpa@...or.com>,
	"peterz@...radead.org" <peterz@...radead.org>,
	"bp@...en8.de" <bp@...en8.de>,
	"bsegall@...gle.com" <bsegall@...gle.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"juri.lelli@...hat.com" <juri.lelli@...hat.com>,
	"yury.khrustalev@....com" <yury.khrustalev@....com>,
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
	"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
	"jannh@...gle.com" <jannh@...gle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"catalin.marinas@....com" <catalin.marinas@....com>,
	"will@...nel.org" <will@...nel.org>,
	"wilco.dijkstra@....com" <wilco.dijkstra@....com>,
	"kees@...nel.org" <kees@...nel.org>
Subject: Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()

On Tue, Aug 20, 2024 at 09:36:46PM +0000, Edgecombe, Rick P wrote:
> On Mon, 2024-08-19 at 20:24 +0100, Mark Brown wrote:

> > +       /* This should really be an atomic cmpxchg.  It is not. */
> > +       copy_from_user_page(vma, page, addr, &val, maddr + offset,
> > +                           sizeof(val));

> Were so close to the real cmpxchg at this point. I took a shot at it with the
> diff at the end. I'm not sure if it might need some of the instrumentation
> calls.

Great - I hadn't been sure if there was any fun with access from kernel
mode on x86.  I can't get that patch to apply cleanly FWIW:

patching file arch/x86/kernel/shstk.c
Hunk #1 FAILED at 204.
patch: **** malformed patch at line 24: offset),

I think I got everything integrated correctly, I should have a version
with that folded in out tomorrow.

> > +
> > +       if (val != expected)
> > +               return false;

> Return false for an int will be 0 (i.e. success). I think it might be covering
> up a bug. The gup happens to args->shadow_stack + args->shadow_stack_size - 1
> (the size inclusive). But the copy happens at the size exclusive.

Ah, yeah, thanks for noticing - that's cut'n'paste from the arm64 code
where the token check is in a separate function.

> > +       if (!(vma->vm_flags & VM_SHADOW_STACK)) {

> Can we check VM_WRITE here too? At least on x86, shadow stacks can be
> mprotect()ed as read-only. The reason for this before I think fell out of the
> implementation details, but all the same it would be nice be consistent. Then it
> should behave identically to a real shadow stack access.

Seems reasonable.

> > +               if (kargs->shadow_stack_size < SHADOW_STACK_SIZE_MIN)
> > +                       return false;

> > +               if (kargs->shadow_stack_size > rlimit(RLIMIT_STACK))
> > +                       return false;

> At the risk of asking a stupid question or one that I should have asked a long
> time ago...
> 
> Why do we need both shadow_stack and shadow_stack_size? We are basically asking
> it to consume a token at a pointer and have userspace manage the shadow stack
> itself. So why does the kernel care what size it is? Couldn't we just have
> 'shadow_stack' have that mean consume a token here.

I was doing things this way for symmetry with how we specify the normal
stack.  That's a bit different since the kernel will actually use the
size for the normal stack but it felt nicer to keep things looking
consistent, it saves users wondering why they work differently.  It's
also a bit of a help with portability given that arm64 expects to have a
top of stack marker above the token by default while x86 doesn't support
that.

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