Date:   Fri, 15 May 2020 19:53:01 -0700
From:   Yu-cheng Yu <yu-cheng.yu@...el.com>
To:     Dave Hansen <dave.hansen@...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>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Vedvyas Shanbhogue <vedvyas.shanbhogue@...el.com>,
        Dave Martin <Dave.Martin@....com>,
        Weijiang Yang <weijiang.yang@...el.com>
Subject: Re: [PATCH v10 01/26] Documentation/x86: Add CET description

On Fri, 2020-05-15 at 16:56 -0700, Dave Hansen wrote:
> On 5/15/20 4:29 PM, Yu-cheng Yu wrote:
> > On Fri, 2020-05-15 at 15:43 -0700, Dave Hansen wrote:
> > > Basically, if there ends up being a bug in an app that violates the
> > > shadow stack rules, the app is broken, period.  The only recourse is to
> > > have the kernel disable CET and reboot.
> > > 
> > > Is that right?
> > 
> > You must be talking about init or any of the system daemons, right?
> > Assuming we let the app itself start CET with an arch_prctl(), why would that be
> > different from the current approach?
> 
> You're getting ahead of me a bit here.
> 
> I'm actually not asking directly about the prctls() or advocating for a
> different approach.  The MPX approach of _requiring the app to make a
> prctl() was actually pretty nasty because sometimes threads got created
> before the prctl() could get called.  Apps ended up inadvertently
> half-MPX-enabled.  Not fun.
> 
> Let's say we have an app doing silly things like retpolines.  (Lots of
> app do lots of silly things).  It gets compiled in a distro but never
> runs on a system with CET.  The app gets run for the first time on a
> system with CET.  App goes boom.  Not init, just some random app, say
> /usr/bin/ldapsearch.
> 
> What's my recourse as an end user?  I want to run my app and turn off
> CET for that app.  How can I do that?

GLIBC_TUNABLES=glibc.tune.hwcaps=-SHSTK,-IBT

> > > > > Can a binary compiled without CET run CET-enabled code?
> > > > 
> > > > Partially yes, but in reality somewhat difficult.
> > > ...
> > > > - If a not-CET application does fork(), and the child wants to turn on CET, it
> > > > would be difficult to manage the stack frames, unless the child knows what is is
> > > > doing.  
> > > 
> > > It might be hard to do, but it is possible with the patches you posted?
> > 
> > It is possible to add an arch_prctl() to turn on CET.  That is simple from the
> > kernel's perspective, but difficult for the application.  Once the app enables
> > shadow stack, it has to take care not to return beyond the function call layers
> > before that point.  It can no longer do longjmp or ucontext swaps to anything
> > before that point.  It will also be complicated if the app enables shadow stack
> > in a signal handler.
> 
> Yu-cheng, I'm having a very hard time getting direct answers to my
> questions.  Could you endeavor to give succinct, direct answers?  If you
> want to give a longer, conditioned answer, that's great.  But, I'd
> appreciate if you could please focus first on clearly answering the
> questions that I'm asking.
> 
> Let me try again:
> 
> 	Is it possible with the patches in this series to run a single-
> 	threaded binary which was has GNU_PROPERTY_X86_FEATURE_1_SHSTK
> 	unset to run with shadow stack protection?
> 
> I think the answer is an unambiguous: "No".  But I'd like to hear it
> from you.

No!

> > >  I think you're saying that the CET-enabled binary would do
> > > arch_setup_elf_property() when it was first exec()'d.  Later, it could
> > > use the new prctl(ARCH_X86_CET_DISABLE) to disable its shadow stack,
> > > then fork() and the child would not be using CET.  Right?
> > > 
> > > What is ARCH_X86_CET_DISABLE used for, anyway?
> > 
> > Both the parent and the child can do ARCH_X86_CET_DISABLE, if CET is
> > not locked.
> 
> Could you please describe a real-world example of why
> ARCH_X86_CET_DISABLE exists?  What kinds of apps will use it, or *are*
> using it?  Why was it created in the first place?

Currently, ld-linux turns off CET if the binary being loaded does not support
CET.

> > > > The JIT examples I mentioned previously run with CET enabled from the
> > > > beginning.  Do you have a reason to do this?  In other words, if the JIT code
> > > > needs CET, the app could have started with CET in the first place.
> > > 
> > > Let's say I have a JIT'd sandbox.  I want the sandbox to be
> > > CET-protected, but the JIT engine itself not to be.
> > 
> > I do not have any objections to this use case, but it needs some cautions as
> > stated above.  It will be much easier and cleaner if the sandbox is in a
> > separate exec'ed task with CET on.
> 
> OK, great suggestion!  Could you do some research and look at the
> various sandboxing techniques?  Is imposing this requirement for a
> separate exec'd task reasonable?  Does it fit nicely with their existing
> models?  How about the Chrome browser and Firefox sandboxs?

I will check.

> > > > > Does this *code* work?  Could you please indicate which JITs have been
> > > > > enabled to use the code in this series?  How much of the new ABI is in use?
> > > > 
> > > > JIT does not necessarily use all of the ABI.  The JIT changes mainly fix stack
> > > > frames and insert ENDBRs.  I do not work on JIT.  What I found is LLVM JIT fixes
> > > > are tested and in the master branch.  Sljit fixes are in the release.
> > > 
> > > Huh, so who is using the new prctl() ABIs?
> > 
> > Any code can use the ABI, but JIT code CET-enabling part mostly do not use these
> > new prctl()'s, except, probably to get CET status.
> 
> Which applications specifically are going to use the new prctl()s which
> this series adds?  How are they going to use them?
> 
> "Any code can use them" is not a specific enough answer.

We have four arch_ptctl() calls.  ARCH_X86_CET_DISABLE and ARCH_X86_CET_LOCK are
used by ld-linux.  ARCH_X86_CET_STATUS are used in many places to determine if
CET is on.  ARCH_X86_CET_ALLOC_SHSTK is used in ucontext related handling, but
it can be use by any application to switch shadow stacks.

> > > > > Where are the selftests/ for this new ABI?  Were you planning on
> > > > > submitting any with this series?
> > > > 
> > > > The ABI is more related to the application side, and therefore most suitable for
> > > > GLIBC unit tests.
> > > 
> > > I was mostly concerned with the kernel selftests.  The things in
> > > tools/testing/selftests/x86 in the kernel tree.
> > 
> > I have run them with CET enabled.  All of them pass, except for the following:
> > Sigreturn from 64-bit to 32-bit fails, because shadow stack is at a 64-bit
> > address.  This is understandable.
> 
> That is not what I meant.  I'm going to be as explicit:
> 
> I expect you to create a test case which you will submit with these
> patches and the test case will go into the tools/testing/selftests/x86
> directory in the kernel tree.  This test case will exercise the kernel
> functionality added in this series, especially the new prctl()s.

I will submit the test case as a separate patch in response to this discussion,
and combine with the series when the discussion concludes.

> One a separate topic: You ran the selftests and one failed.  This is a
> *MASSIVE* warning sign.  It should minimally be described in your cover
> letter, and accompanied by a fix to the test case.  It is absolutely
> unacceptable to introduce a kernel feature that causes a test to fail.
> You must either fix your kernel feature or you fix the test.
> 
> This code can not be accepted until this selftests issue is rectified.

Sure, I will do that.

> 
> > > > The more complicated areas such as pthreads, signals, ucontext,
> > > > fork() are all included there.  I have been constantly running these 
> > > > tests without any problems.  I can provide more details if testing is
> > > > the concern.
> > > 
> > > For something this complicated, with new kernel ABIs, we need an
> > > in-kernel sefltest.
> > > 
> > > MPX was not that much different from this feature.  It required a
> > > boatload of compiler and linker changes to function.  Yet, there was a
> > > simple in-kernel test for it that didn't require *any* of that big pile
> > > of toolchain bits.
> > > 
> > > Is there a reason we don't have one of those for CET?
> > 
> > I have a quick test that checks shadow stack and ibt in both main program and in
> > signals.  Currently it is public on Github.  If that is desired, I can submit it
> > to the mailing list.
> 
> Yes, that is desired.  It must accompany this submission.  It must also
> exercise all of the new ABIs.

Ok.

Yu-cheng

Powered by blists - more mailing lists