Date:   Thu, 26 Oct 2023 13:40:35 -0700
From:   Deepak Gupta <debug@...osinc.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
        "dietmar.eggemann@....com" <dietmar.eggemann@....com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "Szabolcs.Nagy@....com" <Szabolcs.Nagy@....com>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "shuah@...nel.org" <shuah@...nel.org>,
        "mgorman@...e.de" <mgorman@...e.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "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>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "juri.lelli@...hat.com" <juri.lelli@...hat.com>
Subject: Re: [PATCH RFC RFT 2/5] fork: Add shadow stack support to clone3()

On Thu, Oct 26, 2023 at 06:53:37PM +0100, Mark Brown wrote:
>On Thu, Oct 26, 2023 at 05:10:47PM +0000, Edgecombe, Rick P wrote:
>> On Mon, 2023-10-23 at 19:32 +0100, Mark Brown wrote:
>
>> > Right.  We're already adding the cost of the extra map_shadow_stack()
>> > so
>> > it doesn't seem that out of scope.  We could also allow clone3() to
>> > be
>> > used for allocation, potentially removing the ability to specify the
>> > address entirely and only specifying the size.  I did consider that
>> > option but it felt awkward in the API, though equally the whole
>> > shadow
>> > stack allocation thing is a bit that way.  That would avoid concerns
>> > about placing and validating tokens entirely but gives less control
>> > to
>> > userspace.
>
>> There is also cost in the form of extra complexity. Not to throw FUD,
>> but GUP has been the source of thorny problems. And here we would be
>> doing it around security races. We're probably helped that shadow stack
>> is only private/anonymous memory, so maybe it's enough of a normal case
>> to not worry about it.
>
>> Still, there is some extra complexity, and I'm not sure if we really
>> need it. The justification seems to mostly be that it's not as flexible
>> as normal stacks with clone3.
>
>I definitely agree on the complexity, trying to valdiate a token is
>going to be more code doing fiddly things and there's always the risk
>that something will change around it and invalidate assumptions the code
>makes.  Particularly given my inability to test x86 I'm certainly way
>more happy pushing this series forward implementing size only than I am
>doing token validation.
>

FWIW, from arch specific perspective, RISC-V shadow stack extension has
`ssamoswap` to perform this token exchange. But I understand x86 has this
limitation (not sure about arm GCS).

 From security perspective:--
Someone having ability to execute clone3 with control on input, probably
already achieved some level of control flow bending because they need to
corrupt memory and then carefully control registers input to clone3.
Although if it is purely a data oriented gadget, I think it is possible.

Since this RFC is mostly concerned about `size` of shadow stack. I think
we should limit it to size only.

>> I don't understand why doing size-only is awkward. Just because it
>> doesn't match the regular stack clone3 semantics?
>
>Basically, yes - we don't allocate userpace pages in clone3() for the
>normal stack and we do offer userspace control over where to place
>things.  There was some grumbling about this in the current ABI from the
>arm64 side, though the limited control of the size is more of the issue
>really.
>
>I'm not sure placement control is essential but the other bit of it is
>the freeing of the shadow stack, especially if userspace is doing stack
>switches the current behaviour where we free the stack when the thread
>is exiting doesn't feel great exactly.  It's mainly an issue for
>programs that pivot stacks which isn't the common case but it is a
>general sharp edge.

In general, I am assuming such placement requirements emanate because
regular stack holds data (local args, etc) as well and thus software may
make assumptions about how stack frame is prepared and may worry about
layout and such. In case of shadow stack, it can only hold return
addresses and tokens (created by user mode itself). Both of them endup
there as result of call or user sw own way of setting up tokens.

So I don't see a need for software to specify address.

>
>> > This also doesn't do anything to stop anyone trying to allocate sub
>> > page
>> > shadow stacks if they're trying to save memory with all the lack of
>> > overrun protection that implies, though that seems to me to be much
>> > more
>> > of a deliberate decision that people might make, a token would
>> > prevent
>> > that too unless write access to the shadow stack is enabled.
>
>> Sorry, I'm not following. Sub-page shadow stacks?
>
>If someone decides to allocate a page of shadow stack then point thread
>A at the first half of the page and thread B at the second half of the
>page nothing would stop them.  There are obvious issues with this but I
>can see someone trying to do it in a system that creates lots of
>threads and has memory constraints.
>
>> > > > +               /*
>> > > > +                * This doesn't validate that the addresses are
>> > > > mapped
>> > > > +                * VM_SHADOW_STACK, just that they're mapped at
>> > > > all.
>> > > > +                */
>
>> > > It just checks the range, right?
>
>> > Yes, same check as for the normal stack.
>
>> What looked wrong is that the comment says that it checks if the
>> addresses are mapped, but the code just does access_ok(). It's a minor
>> thing in any case.
>
>Oh, I see, yes.

Powered by blists - more mailing lists