linux-kernel - Re: [PATCH RFC RFT 2/5] 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2ec0be71ade109873445a95f3f3c107711bb0943.camel@intel.com>
Date:   Thu, 26 Oct 2023 17:10:47 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "broonie@...nel.org" <broonie@...nel.org>
CC:     "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>,
        "debug@...osinc.com" <debug@...osinc.com>,
        "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 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 don't understand why doing size-only is awkward. Just because it
doesn't match the regular stack clone3 semantics?

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

> 
> > > +               /*
> > > +                * 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.