Message-ID: <aaf918de8585204fb0785ac1fc0f686a8fd88bb0.camel@intel.com>
Date:   Thu, 9 Mar 2023 19:39:41 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "debug@...osinc.com" <debug@...osinc.com>,
        "szabolcs.nagy@....com" <szabolcs.nagy@....com>
CC:     "david@...hat.com" <david@...hat.com>,
        "bsingharora@...il.com" <bsingharora@...il.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "Syromiatnikov, Eugene" <esyr@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Eranian, Stephane" <eranian@...gle.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "nadav.amit@...il.com" <nadav.amit@...il.com>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "dethoma@...rosoft.com" <dethoma@...rosoft.com>,
        "kcc@...gle.com" <kcc@...gle.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "bp@...en8.de" <bp@...en8.de>, "oleg@...hat.com" <oleg@...hat.com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "pavel@....cz" <pavel@....cz>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Schimpe, Christina" <christina.schimpe@...el.com>,
        "mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "Yang, Weijiang" <weijiang.yang@...el.com>,
        "al.grant@....com" <al.grant@....com>,
        "jamorris@...ux.microsoft.com" <jamorris@...ux.microsoft.com>,
        "john.allen@....com" <john.allen@....com>,
        "rppt@...nel.org" <rppt@...nel.org>, "nd@....com" <nd@....com>,
        "andrew.cooper3@...rix.com" <andrew.cooper3@...rix.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "gorcunov@...il.com" <gorcunov@...il.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH v7 33/41] x86/shstk: Introduce map_shadow_stack syscall

On Thu, 2023-03-09 at 10:55 -0800, Deepak Gupta wrote:
> On Thu, Mar 02, 2023 at 05:22:07PM +0000, Szabolcs Nagy wrote:
> > The 02/27/2023 14:29, Rick Edgecombe wrote:
> > > Previously, a new PROT_SHADOW_STACK was attempted,
> > 
> > ...
> > > So rather than repurpose two existing syscalls (mmap, madvise)
> > > that don't
> > > quite fit, just implement a new map_shadow_stack syscall to allow
> > > userspace to map and setup new shadow stacks in one step. While
> > > ucontext
> > > is the primary motivator, userspace may have other unforeseen
> > > reasons to
> > > setup it's own shadow stacks using the WRSS instruction. Towards
> > > this
> > > provide a flag so that stacks can be optionally setup securely
> > > for the
> > > common case of ucontext without enabling WRSS. Or potentially
> > > have the
> > > kernel set up the shadow stack in some new way.
> > 
> > ...
> > > The following example demonstrates how to create a new shadow
> > > stack with
> > > map_shadow_stack:
> > > void *shstk = map_shadow_stack(addr, stack_size,
> > > SHADOW_STACK_SET_TOKEN);
> > 
> > i think
> > 
> > mmap(addr, size, PROT_READ, MAP_ANON|MAP_SHADOW_STACK, -1, 0);
> > 
> > could do the same with less disruption to users (new syscalls
> > are harder to deal with than new flags). it would do the
> > guard page and initial token setup too (there is no flag for
> > it but could be squeezed in).
> 
> Discussion on this topic in v6
> 
https://lore.kernel.org/all/20230223000340.GB945966@debug.ba.rivosinc.com/
> 
> Again I know earlier CET patches had protection flag and somehow due
> to pushback
> on mailing list,
>  it was adopted to go for special syscall because no one else
> had shadow stack.
> 
> Seeing a response from Szabolcs, I am assuming arm4 would also want
> to follow
> using mmap to manufacture shadow stack. For reference RFC patches for
> risc-v shadow stack,
> use a new protection flag = PROT_SHADOWSTACK.
> 
https://lore.kernel.org/lkml/20230213045351.3945824-1-debug@rivosinc.com/
> 
> I know earlier discussion had been that we let this go and do a re-
> factor later as other
> arch support trickle in. But as I thought more on this and I think it
> may just be
> messy from user mode point of view as well to have cognition of two
> different ways of
> creating shadow stack. One would be special syscall (in current libc)
> and another `mmap`
> (whenever future re-factor happens)
> 
> If it's not too late, it would be more wise to take `mmap`
> approach rather than special `syscall` approach.

There is sort of two things intermixed here when we talk about a
PROT_SHADOW_STACK.

One is: what is the interface for specifying how the shadow stack
should be provisioned with data? Right now there are two ways
supported, all zero or with an X86 shadow stack restore token at the
end. Then there was already some conversation about a third type. In
which case the question would be is using mmap MAP_ flags the right
place for this? How many types of initialization will be needed in the
end and what is the overlap between the architectures?

The other thing is: should shadow stack memory creation be tightly
controlled? For example in x86 we limit this to anonymous memory, etc.
Some reasons for this are x86 specific, but some are not. So if we
disallow most of the options why allow the interface to take them? And
then you are in the position of carefully maintaining a list of not-
allowed options instead letting a list of allowed options sit there.

The only benefit I've heard is that it saves creating a new syscall,
but it also saves several MAP_ flags. That, and that the RFC for riscv
did a PROT_SHADOW_STACK to start. So, yes, two people asked the same
question, but I'm still not seeing any benefits. Can you give the pros
and cons please?

BTW, in glibc map_shadow_stack is called from arch code. So I think
userspace wise, for this to affect other architectures there would need
to be some code that could do things generically, with somehow the
shadow stack pivot abstracted but the shadow stack allocation not.

Powered by blists - more mailing lists