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: <23a8838adda28b03b3db77e135934e2da0599d0f.camel@intel.com>
Date: Fri, 16 Aug 2024 14:52:28 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "catalin.marinas@....com" <catalin.marinas@....com>
CC: "dietmar.eggemann@....com" <dietmar.eggemann@....com>,
	"juri.lelli@...hat.com" <juri.lelli@...hat.com>, "linux-api@...r.kernel.org"
	<linux-api@...r.kernel.org>, "shuah@...nel.org" <shuah@...nel.org>,
	"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>, "Szabolcs.Nagy@....com"
	<Szabolcs.Nagy@....com>, "fweimer@...hat.com" <fweimer@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"mingo@...hat.com" <mingo@...hat.com>, "hjl.tools@...il.com"
	<hjl.tools@...il.com>, "rostedt@...dmis.org" <rostedt@...dmis.org>,
	"vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "vschneid@...hat.com"
	<vschneid@...hat.com>, "kees@...nel.org" <kees@...nel.org>, "will@...nel.org"
	<will@...nel.org>, "hpa@...or.com" <hpa@...or.com>, "peterz@...radead.org"
	<peterz@...radead.org>, "jannh@...gle.com" <jannh@...gle.com>, "bp@...en8.de"
	<bp@...en8.de>, "bsegall@...gle.com" <bsegall@...gle.com>,
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
	"broonie@...nel.org" <broonie@...nel.org>, "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH RFT v8 4/9] fork: Add shadow stack support to clone3()

On Fri, 2024-08-16 at 09:44 +0100, Catalin Marinas wrote:
> > After a token is consumed normally, it doesn't set it to zero. Instead it
> > sets
> > it to a "previous-ssp token". I don't think we actually want to do that here
> > though because it involves the old SSP, which doesn't really apply in this
> > case.
> > I don't see any problem with zero, but was there any special thinking behind
> > it?
> 
> BTW, since it's the parent setting up the shadow stack in its own
> address space before forking, I think at least the read can avoid
> access_remote_vm() and we could do it earlier, even before the new
> process is created.

Hmm. Makes sense. It's a bit racy since the parent could consume that token from
another thread, but it would be a race in any case.

> 
> > > +       if (access_remote_vm(mm, addr, &val, sizeof(val),
> > > +                            FOLL_FORCE | FOLL_WRITE) != sizeof(val))
> > > +               goto out;
> > 
> > The GUPs still seem a bit unfortunate for a couple reasons:
> >   - We could do a CMPXCHG version and are just not (I see ARM has identical
> > code
> > in gcs_consume_token()). It's not the only race like this though FWIW.
> >   - I *think* this is the only unprivileged FOLL_FORCE that can write to the
> > current process in the kernel. As is, it could be used on normal RO
> > mappings, at
> > least in a limited way. Maybe another point for the VMA check. We'd want to
> > check that it is normal shadow stack?
> >   - Lingering doubts about the wisdom of doing GUPs during task creation.
> 
> I don't like the access_remote_vm() either. In the common (practically
> only) case with CLONE_VM, the mm is actually current->mm, so no need for
> a GUP.

On the x86 side, we don't have a shadow stack access CMPXCHG. We will have to
GUP and do a normal CMPXCHG off of the direct map to handle it fully properly in
any case (CLONE_VM or not).

> 
> We could, in theory, consume this token in the parent before the child
> mm is created. The downside is that if a parent forks multiple
> processes using the same shadow stack, it will have to set the token
> each time. I'd be fine with this, that's really only for the mostly
> theoretical case where one doesn't use CLONE_VM and still want a
> separate stack and shadow stack.
> 
> > I don't think they are show stoppers, but the VMA check would be nice to
> > have in
> > the first upstream support.
> 
> Good point.