Message-ID: <3e3570b8-f138-4373-94b8-b471419bbf11@sirena.org.uk>
Date: Thu, 15 Aug 2024 15:24:12 +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>,
	"jannh@...gle.com" <jannh@...gle.com>,
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
	"kees@...nel.org" <kees@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"catalin.marinas@....com" <catalin.marinas@....com>,
	"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
	"will@...nel.org" <will@...nel.org>
Subject: Re: [PATCH RFT v8 4/9] fork: Add shadow stack support to clone3()

On Thu, Aug 15, 2024 at 12:18:23AM +0000, Edgecombe, Rick P wrote:
> On Thu, 2024-08-08 at 09:15 +0100, Mark Brown wrote:

> > +       ssp = args->shadow_stack + args->shadow_stack_size;
> > +       addr = ssp - SS_FRAME_SIZE;
> > +       expected = ssp | BIT(0);

> > +       mm = get_task_mm(t);
> > +       if (!mm)
> > +               return -EFAULT;

> We could check that the VMA is shadow stack here. I'm not sure what could go
> wrong though. If you point it to RW memory it could start the thread with that
> as a shadow stack and just blow up at the first call. It might be nicer to fail
> earlier though.

Sure, I wasn't doing anything since like you say the new thread will
fail anyway but we can do the check.  As you point out below it'll close
down the possibility of writing to memory.

> > +       /* This should really be an atomic cmpxchg.  It is not. */
> > +       if (access_remote_vm(mm, addr, &val, sizeof(val),
> > +                            FOLL_FORCE) != sizeof(val))
> > +               goto out;
> > +
> > +       if (val != expected)
> > +               goto out;
> > +       val = 0;

> 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?

I wasn't aware of the x86 behaviour for pivots here, 0 was just a
default thing to choose for an invalid value.  arm64 will also leave
a value on the outgoing stack as a product of the two step pivots we
have but it's not really something you'd look for.

> > +       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 think they are show stoppers, but the VMA check would be nice to have in
> the first upstream support.

The check you suggest for shadow stack memory should avoid abuse of the
FOLL_FORCE at least.  It'd be a bit narrow, you'd only be able to
overwrite a value where we managed to read a valid token, but it's
there.

> > +static void shstk_post_fork(struct task_struct *p,
> > +                           struct kernel_clone_args *args)
> > +{
> > +       if (!IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK))
> > +               return;
> > +
> > +       if (!args->shadow_stack)
> > +               return;
> > +
> > +       if (arch_shstk_post_fork(p, args) != 0)
> > +               force_sig_fault_to_task(SIGSEGV, SEGV_CPERR, NULL, p);
> > +}

> Hmm, is this forcing the signal on the new task, which is set up on a user
> provided shadow stack that failed the token check? It would handle the signal
> with an arbitrary SSP then I think. We should probably fail the clone call in
> the parent instead, which can be done by doing the work in copy_process(). Do

One thing I was thinking when writing this was that I wanted to make it
possible to implement the check in the vDSO if there's any architectures
that could do so, avoiding any need to GUP, but I can't see that that's
actually been possible.

> you see a problem with doing it at the end of copy_process()? I don't know if
> there could be ordering constraints.

I was concerned when I was writing the code about ordring constraints,
but I did revise what the code was doing several times and as I was
saying in reply to Catalin I'm no longer sure those apply.

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

Powered by blists - more mailing lists