Message-ID: <64837d2af3ae39bafd025b3141a04f04f4323205.camel@intel.com>
Date:   Wed, 14 Jun 2023 16:57:53 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "broonie@...nel.org" <broonie@...nel.org>,
        "szabolcs.nagy@....com" <szabolcs.nagy@....com>
CC:     "Xu, Pengfei" <pengfei.xu@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "kcc@...gle.com" <kcc@...gle.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "nadav.amit@...il.com" <nadav.amit@...il.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "david@...hat.com" <david@...hat.com>,
        "Schimpe, Christina" <christina.schimpe@...el.com>,
        "Yang, Weijiang" <weijiang.yang@...el.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dethoma@...rosoft.com" <dethoma@...rosoft.com>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
        "pavel@....cz" <pavel@....cz>, "bp@...en8.de" <bp@...en8.de>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "rppt@...nel.org" <rppt@...nel.org>,
        "jamorris@...ux.microsoft.com" <jamorris@...ux.microsoft.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "john.allen@....com" <john.allen@....com>,
        "bsingharora@...il.com" <bsingharora@...il.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "oleg@...hat.com" <oleg@...hat.com>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "andrew.cooper3@...rix.com" <andrew.cooper3@...rix.com>,
        "gorcunov@...il.com" <gorcunov@...il.com>,
        "Yu, Yu-cheng" <yu-cheng.yu@...el.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "debug@...osinc.com" <debug@...osinc.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "Syromiatnikov, Eugene" <esyr@...hat.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "Torvalds, Linus" <torvalds@...ux-foundation.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "Eranian, Stephane" <eranian@...gle.com>
Subject: Re: [PATCH v9 23/42] Documentation/x86: Add CET shadow stack
 description

On Wed, 2023-06-14 at 11:43 +0100, szabolcs.nagy@....com wrote:
> The 06/13/2023 19:57, Edgecombe, Rick P wrote:
> > On Tue, 2023-06-13 at 18:57 +0100, Mark Brown wrote:
> > > > > > For my part, the thing I would really like to see unified
> > > > > > as
> > > > > > much > > as
> > > > > > possible is at the app developer's interface (glibc/gcc).
> > > > > > The
> > > > > > idea
> > > > > > would be to make it easy for app developers to know if
> > > > > > their
> > > > > > app
> > > > > > supports shadow stack. There will probably be some
> > > > > > differences,
> > > > > > but > > it
> > > > > > would be great if there was mostly the same behavior and a
> > > > > > small > > list
> > > > > > of differences. I'm thinking about the behavior of
> > > > > > longjmp(),
> > > > > > swapcontext(), etc.
> > > > 
> > > > Yes, very much so.  sigaltcontext() too.
> > 
> > For alt shadow stack's, this is what I came up with:
> > https://lore.kernel.org/lkml/20220929222936.14584-40-rick.p.edgecombe@intel.com/
> > 
> > Unfortunately it can't work automatically with sigaltstack(). Since
> > it
> > has to be a new thing anyway, it's been left for the future. I
> > guess
> > that might have a better chance of being cross arch.
> 
> i dont think you can add sigaltshstk later.
> 
> libgcc already has unwinder code for shstk and that cannot handle
> discontinous shadow stack.

Are you referring to the existing C++ exception unwinding code that
expects a different signal frame format? Yea this is a problem, but I
don't see how it's a problem with any solutions now that will be harder
later. I mentioned it when I brought up all the app compatibility
problems.[0]

The problem is that gcc expects a fixed 8 byte sized shadow stack
signal frame. The format in these patches is such that it can be
expanded for the sake of supporting alt shadow stack later, but it
happens to be a fixed 8 bytes for now, so it will work seamlessly with
these old gcc's. HJ has some patches to fix GCC to jump over a
dynamically sized shadow stack signal frame, but this of course won't
stop old gcc's from generating binaries that won't work with an
expanded frame.

I was waffling on whether it would be better to pad the shadow stack
[1] signal frame to start, this would break compatibility with any
binaries that use this -fnon-call-exceptions feature (if there are
any), but would set us up better for the future if we got away with it.
On one hand we are already juggling some compatibility issues so maybe
it's not too much worse, but on the other hand the kernel is trying its
best to be as compatible as it can given the situation. It doesn't
*need* to break this compatibility at this point.

In the end I thought it was better to deal with it later.

>  (may affect longjmp too depending on
> how it is implemented)

glibc's longjmp ignores anything everything it skips over and just does
INCSSP until it gets back to the setjmp point. So it is not affected by
the shadow stack signal frame format. I don't think we can support
longjmping off an alt shadow stack unless we enable WRSS or get the
kernel's help. So this was to be declared as unsupported.

> 
> we can change the unwinder now to know how to switch shstk when
> it unwinds the signal frame and backport that to systems that
> want to support shstk. or we can introduce a new elf marking
> scheme just for sigaltshstk when it is added so incompatibility
> can be detected. or we simply not support unwinding with
> sigaltshstk which would make it pretty much useless in practice.

Yea, I was thinking along the same lines. Someday we could easily need
some new marker. Maybe because we want to add something, or maybe
because of the pre-existing userspace. In that case, this
implementation will get the ball rolling and we can learn more about
how shadow stack will be used. So if we need to break compatibility
with any apps, we would not really be in a different situation than we
are already in (if we are going to take proper care to not break
userspace). So if/when that happens all the learning's can go into the
clean break.

But if it's not clear, unwinder's that properly use the format in these
patches should work from an alt shadow stack implemented like that RFC
linked earlier in the thread. At least it will be able to read back the
shadow stack starting from the alt shadow stack, it can't actually
resume control flow from where it unwound to. For that we need WRSS or
some kernel help.

[0]
https://lore.kernel.org/lkml/7d8133c7e0186bdaeb3893c1c808148dc0d11945.camel@intel.com/

Powered by blists - more mailing lists