Message-ID: <d05d23d56bd2c7de30e7732e6bd3d313d8385c47.camel@intel.com>
Date:   Thu, 16 Nov 2023 00:52:09 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "broonie@...nel.org" <broonie@...nel.org>,
        "Szabolcs.Nagy@....com" <Szabolcs.Nagy@....com>
CC:     "dietmar.eggemann@....com" <dietmar.eggemann@....com>,
        "keescook@...omium.org" <keescook@...omium.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>,
        "vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
        "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>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "vschneid@...hat.com" <vschneid@...hat.com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "bristot@...hat.com" <bristot@...hat.com>,
        "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>,
        "Pandey, Sunil K" <sunil.k.pandey@...el.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "juri.lelli@...hat.com" <juri.lelli@...hat.com>
Subject: Re: [PATCH RFC RFT v2 2/5] fork: Add shadow stack support to clone3()

On Wed, 2023-11-15 at 18:43 +0000, Mark Brown wrote:
> > end marker token (0) needs it i guess.
> 
> x86 doesn't currently have end markers.  Actually, that's a point -
> should we add a flag for specifying the use of end markers here?
> There's code in my map_shadow_stack() implementation for arm64 which
> does that.

Hmm, I guess there isn't a way to pass a flag for the initial exec
stack? So probably it should just mirror that behavior. Unless you
think a lot of people would like to skip the default behavior.

And of course we don't have a marker on x86 (TODO with alt shadow
stacks). We could still check for size < 8 if we want it to be a
universal thing.

> 
> > otherwise 0 size would be fine: the child may not execute
> > a call instruction at all.

It seems like a special case. Where should the SSP be for the new
thread?

> 
> Well, a size of specifically zero will result in a fallback to
> implicit
> allocation/sizing of the stack as things stand so this is
> specifically
> the case where a size has been specified but is smaller than a single
> entry.
> 
> > > > I think for CLONE_VM we should not require a non-zero size.
> > > > Speaking of
> > > > CLONE_VM we should probably be clear on what the expected
> > > > behavior is
> > > > for situations when a new shadow stack is not usually
> > > > allocated.
> > > > !CLONE_VM || CLONE_VFORK will use the existing shadow stack.
> > > > Should we
> > > > require shadow_stack_size be zero in this case, or just ignore
> > > > it? I'd
> > > > lean towards requiring it to be zero so userspace doesn't pass
> > > > garbage
> > > > in that we have to accommodate later. What we could possibly
> > > > need to do
> > > > around that though, I'm not sure. What do you think?
> 
> > > Yes, requiring it to be zero in that case makes sense I think.
> 
> > i think the condition is "no specified separate stack for
> > the child (stack==0 || stack==sp)".
> 
> > CLONE_VFORK does not imply that the existing stack will be
> > used (a stack for the child can be specified, i think both
> > glibc and musl do this in posix_spawn).
> 
> That also works as a check I think, though it requires the arch to
> check
> for the stack==sp case - I hadn't been aware of the posix_spawn()
> usage,
> the above checks Rick suggested just follow the handling for implicit
> allocation we have currently.

I didn't realize it was passing its own stack either. I guess the
reason is to avoid stack overflows. But none of the specific reasons
listed in the comments seem to applicable to shadow stacks.

What is the case for stack=sp bit of the logic?


I need to look into this more. My first thought is, passing in a stack
doesn't absolutely mean they want a new shadow stack allocated either.
We are changing one heuristic, for another.

The other thought is, the new behavior in the !CLONE_VM case doesn't
seem optimal. fork has ->stack==0. Then we would be allocating a stack
in only the child's MM and changing the SSP there, and for what reason?
So I don't think we should fully move away from taking hints from the
CLONE flags.

Maybe an alternate could just be to lose the CLONE_VFORK specific stack
sharing logic. CLONE_VM always gets a new shadow stack. I don't think
it would disturb userspace functionally, but just involves more
mapping/unmapping.

Powered by blists - more mailing lists