Date:   Thu, 16 Mar 2023 12:30:40 -0700
From:   Deepak Gupta <debug@...osinc.com>
To:     Mike Rapoport <rppt@...nel.org>
Cc:     Szabolcs Nagy <szabolcs.nagy@....com>,
        Rick Edgecombe <rick.p.edgecombe@...el.com>, x86@...nel.org,
        "H . Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-mm@...ck.org,
        linux-arch@...r.kernel.org, linux-api@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Andy Lutomirski <luto@...nel.org>,
        Balbir Singh <bsingharora@...il.com>,
        Borislav Petkov <bp@...en8.de>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Eugene Syromiatnikov <esyr@...hat.com>,
        Florian Weimer <fweimer@...hat.com>,
        "H . J . Lu" <hjl.tools@...il.com>, Jann Horn <jannh@...gle.com>,
        Jonathan Corbet <corbet@....net>,
        Kees Cook <keescook@...omium.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Oleg Nesterov <oleg@...hat.com>, Pavel Machek <pavel@....cz>,
        Peter Zijlstra <peterz@...radead.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Weijiang Yang <weijiang.yang@...el.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        John Allen <john.allen@....com>, kcc@...gle.com,
        eranian@...gle.com, jamorris@...ux.microsoft.com,
        dethoma@...rosoft.com, akpm@...ux-foundation.org,
        Andrew.Cooper3@...rix.com, christina.schimpe@...el.com,
        david@...hat.com, nd@....com, al.grant@....com
Subject: Re: [PATCH v7 33/41] x86/shstk: Introduce map_shadow_stack syscall

On Tue, Mar 14, 2023 at 12:19 AM Mike Rapoport <rppt@...nel.org> wrote:
>
> Hi,
>
> On Thu, Mar 09, 2023 at 10:55:11AM -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.
>
> I disagree.
>
> Having shadow stack flags for mmap() adds unnecessary complexity to the
> core-mm, while having a dedicated syscall hides all the details in the
> architecture specific code.

Again reiterating it would've made sense if only x86 had a shadow stack.
aarch64 announced support for guarded stack. risc-v spec is in
development to support shadow stack.

So there will be shadow stack related flow in these arches.

>
> Another reason to use a dedicated system call allows for better
> extensibility if/when we'd need to update the way shadow stack VMA is
> created.

I see two valid points here
    - Shadow stack doesn't need conversion into different memory types
(which is usually the case for address ranges created by mmap)
      So there is a static page permissions on shadow stack which is
not mutable.

    - Future feature addition (if there is one needed) at the time of
shadow stack creation
      It would avoid future tax on mmap

I'll think more about this.

>
> As for the userspace convenience, it is anyway required to add special
> code for creating the shadow stack and it wouldn't matter if that code
> would use mmap(NEW_FLAG) or map_shadow_stack().

Yes *strictly* from userspace convenience, it doesn't matter which option.

>
> > > most of the mmap features need not be available (EINVAL) when
> > > MAP_SHADOW_STACK is specified.
> > >
> > > the main drawback is running out of mmap flags so extension
> > > is limited. (but the new syscall has limitations too).
>
> --
> Sincerely yours,
> Mike.

Powered by blists - more mailing lists