Re: [PATCH v10 01/26] Documentation/x86: Add CET description

Yu-cheng Yu
2020-05-16T02:53:01Z

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