Linux-api Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Hansen <dave.hansen@intel.com>
To: Yu-cheng Yu <yu-cheng.yu@intel.com>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-mm@kvack.org, linux-arch@vger.kernel.org,
	linux-api@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
	Andy Lutomirski <luto@kernel.org>,
	Balbir Singh <bsingharora@gmail.com>,
	Borislav Petkov <bp@alien8.de>,
	Cyrill Gorcunov <gorcunov@gmail.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Eugene Syromiatnikov <esyr@redhat.com>,
	Florian Weimer <fweimer@redhat.com>,
	"H.J. Lu" <hjl.tools@gmail.com>, Jann Horn <jannh@google.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Kees Cook <keescook@chromium.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Nadav Amit <nadav.amit@gmail.com>,
	Oleg Nesterov <oleg@redhat.com>, Pavel Machek <pavel@ucw.cz>,
	Peter Zijlstra <peterz@infradead.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	Vedvyas Shanbhogue <vedvyas.shanbhogue@intel.com>,
	Dave Martin <Dave.Martin@arm.com>,
	Weijiang Yang <weijiang.yang@intel.com>
Subject: Re: [PATCH v10 01/26] Documentation/x86: Add CET description
Date: Fri, 15 May 2020 16:56:38 -0700
Message-ID: <631f071c-c755-a818-6a97-b333eb1fe21c@intel.com> (raw)
In-Reply-To: <b09658f92eb66c1d1be509813939b9ed827f9cf0.camel@intel.com>

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?

>>>> 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.

>>  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?

>>> 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?

>>>> 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.

>>>> 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.

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.

>>> 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.

  reply index

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-29 22:07 [PATCH v10 00/26] Control-flow Enforcement: Shadow Stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 01/26] Documentation/x86: Add CET description Yu-cheng Yu
2020-04-29 22:53   ` Dave Hansen
2020-04-29 23:02     ` Yu-cheng Yu
2020-05-12 23:20       ` Yu-cheng Yu
2020-05-15 18:39         ` Dave Hansen
2020-05-15 21:33           ` Yu-cheng Yu
2020-05-15 22:43             ` Dave Hansen
2020-05-15 23:29               ` Yu-cheng Yu
2020-05-15 23:56                 ` Dave Hansen [this message]
2020-05-16  2:51                   ` H.J. Lu
2020-05-17 23:09                     ` Dave Hansen
2020-05-16  2:53                   ` Yu-cheng Yu
2020-05-18 13:41                     ` Dave Hansen
2020-05-18 14:01                       ` H.J. Lu
2020-05-18 14:26                         ` Dave Hansen
2020-05-18 14:21                       ` Yu-cheng Yu
2020-05-18 23:47                     ` Yu-cheng Yu
2020-05-19  0:38                       ` Dave Hansen
2020-05-19  1:35                         ` Andy Lutomirski
2020-05-20  1:04                           ` Andy Lutomirski
2020-05-29  2:08                             ` Yu-cheng Yu
2020-05-16  0:13               ` Andrew Cooper
2020-05-16  2:37                 ` H.J. Lu
2020-05-16 14:09                   ` Andrew Cooper
2020-05-22 16:49                     ` Peter Zijlstra
2020-05-22 17:48                       ` Andrew Cooper
2020-04-29 22:07 ` [PATCH v10 02/26] x86/cpufeatures: Add CET CPU feature flags for Control-flow Enforcement Technology (CET) Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 03/26] x86/fpu/xstate: Introduce CET MSR XSAVES supervisor states Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 04/26] x86/cet: Add control-protection fault handler Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 05/26] x86/cet/shstk: Add Kconfig option for user-mode Shadow Stack Yu-cheng Yu
2020-05-07 15:55   ` Dave Hansen
2020-05-07 16:59     ` Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 06/26] x86/mm: Change _PAGE_DIRTY to _PAGE_DIRTY_HW Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 07/26] x86/mm: Remove _PAGE_DIRTY_HW from kernel RO pages Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 08/26] x86/mm: Introduce _PAGE_COW Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 09/26] drm/i915/gvt: Change _PAGE_DIRTY to _PAGE_DIRTY_BITS Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 10/26] x86/mm: Update pte_modify for _PAGE_COW Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 11/26] x86/mm: Update ptep_set_wrprotect() and pmdp_set_wrprotect() for transition from _PAGE_DIRTY_HW to _PAGE_COW Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 12/26] mm: Introduce VM_SHSTK for shadow stack memory Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 13/26] x86/mm: Shadow Stack page fault error checking Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 14/26] x86/mm: Update maybe_mkwrite() for shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 15/26] mm: Fixup places that call pte_mkwrite() directly Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 16/26] mm: Add guard pages around a shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 17/26] mm/mmap: Add shadow stack pages to memory accounting Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 18/26] mm: Update can_follow_write_pte() for shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 19/26] x86/cet/shstk: User-mode shadow stack support Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 20/26] x86/cet/shstk: Handle signals for shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 21/26] ELF: UAPI and Kconfig additions for ELF program properties Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 22/26] ELF: Add ELF program property parsing support Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 23/26] ELF: Introduce arch_setup_elf_property() Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 24/26] x86/cet/shstk: ELF header parsing for shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 25/26] x86/cet/shstk: Handle thread " Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 26/26] x86/cet/shstk: Add arch_prctl functions for " Yu-cheng Yu
2020-05-21 22:42   ` Kees Cook
2020-05-22 17:17     ` Yu-cheng Yu
2020-05-22 17:29       ` Eugene Syromiatnikov
2020-05-22 18:13         ` Yu-cheng Yu
2020-05-21 15:15 ` [PATCH v10 00/26] Control-flow Enforcement: Shadow Stack Josh Poimboeuf
2020-05-21 15:57   ` Yu-cheng Yu
2020-05-21 18:50     ` Josh Poimboeuf
2020-05-21 19:08       ` Yu-cheng Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=631f071c-c755-a818-6a97-b333eb1fe21c@intel.com \
    --to=dave.hansen@intel.com \
    --cc=Dave.Martin@arm.com \
    --cc=arnd@arndb.de \
    --cc=bp@alien8.de \
    --cc=bsingharora@gmail.com \
    --cc=corbet@lwn.net \
    --cc=dave.hansen@linux.intel.com \
    --cc=esyr@redhat.com \
    --cc=fweimer@redhat.com \
    --cc=gorcunov@gmail.com \
    --cc=hjl.tools@gmail.com \
    --cc=hpa@zytor.com \
    --cc=jannh@google.com \
    --cc=keescook@chromium.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@kernel.org \
    --cc=mike.kravetz@oracle.com \
    --cc=mingo@redhat.com \
    --cc=nadav.amit@gmail.com \
    --cc=oleg@redhat.com \
    --cc=pavel@ucw.cz \
    --cc=peterz@infradead.org \
    --cc=ravi.v.shankar@intel.com \
    --cc=rdunlap@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=vedvyas.shanbhogue@intel.com \
    --cc=weijiang.yang@intel.com \
    --cc=x86@kernel.org \
    --cc=yu-cheng.yu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-api Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-api/0 linux-api/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-api linux-api/ https://lore.kernel.org/linux-api \
		linux-api@vger.kernel.org
	public-inbox-index linux-api

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-api


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git