Message-ID: <e83cf0ff1834bcfc7436d9370ae2807e5a026f32.camel@intel.com>
Date:   Fri, 4 Feb 2022 20:23:38 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "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>,
        "Eranian, Stephane" <eranian@...gle.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.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>,
        "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>,
        "Yang, Weijiang" <weijiang.yang@...el.com>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "pavel@....cz" <pavel@....cz>, "arnd@...db.de" <arnd@...db.de>,
        "Moreira, Joao" <joao.moreira@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "Dave.Martin@....com" <Dave.Martin@....com>,
        "john.allen@....com" <john.allen@....com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.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>
Subject: Re: [PATCH 00/35] Shadow stacks for userspace

On Thu, 2022-02-03 at 21:20 -0800, Andy Lutomirski wrote:
> On Thu, Feb 3, 2022, at 5:08 PM, Edgecombe, Rick P wrote:
> > Hi Thomas,
> > > >         Signals
> > > >         -------
> > > >         Originally signals placed the location of the shadow
> > > > stack
> > > > restore 
> > > >         token inside the saved state on the stack. This was
> > > > problematic from a 
> > > >         past ABI promises perspective.
> 
> What was the actual problem?

The meat of the discussion that I saw was this thread:

https://lore.kernel.org/lkml/CALCETrVTeYfzO-XWh+VwTuKCyPyp-oOMGH=QR_msG9tPQ4xPmA@mail.gmail.com/

The problem was that the saved shadow stack pointer was placed in a
location unexpected by userspace (at the end of the floating point
state), which apparently can be relocated by userspace that would not
be aware of the shadow stack state at the end. I think an earlier
version was not extendable either.

It does not seem to be a fully dead end, but I have to admit I didn’t
fully internalize the limits imposed by the userspace expectations of
the sigframe because the current solution seemed good. I’ll have to dig
into it a little more because alt stack, CRIU and these expectations
all seem intertwined.

Here is a question for you guys though – is a general ucontext
extension solution a nice-to-have on its own?

I was thinking that it would be better to keep CET stuff out of the
sigframe related structures if it can be avoided. One reason is that it
keeps security related register values out of writable locations. I’m
not thinking of any specific problem, but just a general idea of not
opening that stuff up if it’s not needed. An example is in the IBT
series, the wait-for-endbranch state was saved in a ucontext flag. Some
usages may want to keep it from being cleared in a signal and the
endbranch check skipped. So for shadow stack, just the general notion
that this is not ideal state to open up.

On where to keep the wait-for-endbranch state, PeterZ had suggested the
possibility of having a per-mm hashmap of (userspace stack addresses)-> 
(kernel side saved state), limited to some sensible limit of items.
This could be extendable to other state besides CET stuff. I was
thinking to look in that direction if it’s needed for the alt shadow
stack.

But, is it swimming against the place where saved state is "supposed"
to be? There is some optimum of compatibility (more apps able to opt-
in) and security. I guess it's probably not good to have the kernel
bend over backwards trying to get both.

> > > > So the restore location was
> > > > instead just 
> > > >         assumed from the shadow stack pointer. This works
> > > > because in
> > > > normal 
> > > >         allowed cases of calling sigreturn, the shadow stack
> > > > pointer
> > > > should be 
> > > >         right at the restore token at that time. There is no
> > > > alternate shadow 
> > > >         stack support. If an alt shadow stack is added later we
> > > > would
> > > >         need to
> > > 
> > > So how is that going to work? altstack is not an esoteric corner
> > > case.
> > 
> > My understanding is that the main usages for the signal stack were
> > handling stack overflows and corruption. Since the shadow stack
> > only
> > contains return addresses rather than large stack allocations, and
> > is
> > not generally writable or pivotable, I thought there was a good
> > possibility an alt shadow stack would not end up being especially
> > useful. Does it seem like reasonable guesswork?
> 
> It's also used for things like DOSEMU that execute in a weird context
> and then trap back out to the outer program using a signal handler
> and an altstack.  Also, imagine someone writing a SIGSEGV handler
> specifically intended to handle shadow stack overflow.

Interesting, thanks. I had been thinking that an alt shadow stack would
require a new interface that would mostly just sit dormant taking up
space. But probably an (obvious) better way would be to just have the
signalstack() syscall automatically create a new shadow stack, like the
rest of the series does automatically for new threads. I’ll think I’ll
see how that would look.

> 
> The shadow stack can be pivoted using RSTORSSP.

Yes, I just meant that the ability to pivot or modify is restricted (in
RSTORSSP's case by restore token checks) and so with less ability to
interact with it, it could be less likely for there to be corruptions.
This if of course just speculation.

Powered by blists - more mailing lists