linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Xing, Cedric" <cedric.xing@intel.com>
To: Andy Lutomirski <luto@kernel.org>
Cc: "Christopherson, Sean J" <sean.j.christopherson@intel.com>,
	"Jarkko Sakkinen" <jarkko.sakkinen@linux.intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"nhorman@redhat.com" <nhorman@redhat.com>,
	"npmccallum@redhat.com" <npmccallum@redhat.com>,
	"Ayoun, Serge" <serge.ayoun@intel.com>,
	"Katz-zamir, Shay" <shay.katz-zamir@intel.com>,
	"Huang, Haitao" <haitao.huang@intel.com>,
	"andriy.shevchenko@linux.intel.com" 
	<andriy.shevchenko@linux.intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"Svahn, Kai" <kai.svahn@intel.com>, "bp@alien8.de" <bp@alien8.de>,
	"josh@joshtriplett.org" <josh@joshtriplett.org>,
	"Huang, Kai" <kai.huang@intel.com>,
	"rientjes@google.com" <rientjes@google.com>,
	"Dave Hansen" <dave.hansen@linux.intel.com>,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Jethro Beekman <jethro@fortanix.com>,
	"Dr . Greg Wettstein" <greg@enjellic.com>
Subject: RE: [PATCH v19,RESEND 24/27] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
Date: Sun, 24 Mar 2019 08:59:48 +0000	[thread overview]
Message-ID: <960B34DE67B9E140824F1DCDEC400C0F4E85E989@ORSMSX116.amr.corp.intel.com> (raw)
In-Reply-To: <CALCETrV589POrG3RaPHBWOFuytkWjeJgJgLHk4OKbizOgJVKZQ@mail.gmail.com>

Hi Andy,

Thank you for your valuable feedbacks!

Per what you have been saying, your feedbacks come from different angles - i.e. functionality vs. security, but they are mixed up somehow. As an effort to make the discussion more constructive going forward, I'd like you to acknowledge that, in terms of functionality, my proposal is a superset of the current patch, as I have proven by implementing Sean's API using mine as a subroutine. That said, if you are satisfied with his, you should be satisfied with mine as well, from functional perspective. And because of that, I'll try to interpret/address your concerns from security perspective unless otherwise noted. I'm aware that there's still subtle difference between Sean's API and mine - e.g. my proposal consumes 24 bytes more stack space (for the same functionality, i.e. exit callback is null) than his, due to the additional parameters. But I don't believe that would become a "make it or break it" situation in practice.

> I’m going to put my vDSO maintainer hat on for a minute.  Cedric, your
> proposal has the following issues related specifically to the vDSO:
> 
> It inherently contains indirect branches.  This means that, on retpoline
> configurations, it probably needs to use retpolines.  This is doable,
> but it’s nasty, and you need to worry about register clobbers.

Only the weakest link matters in security. With dynamic linking in use, this additional indirect CALL can't make things worse. But I'm open to, and in fact also willing to, apply whatever mitigation that you think is satisfactory (or that has been applied to other indirect branches, such as in PLT), such as retpoline. Btw, don't worry about register clobbers because we have at least %rax at our disposal.

> 
> It uses effectively unbounded stack space. The vDSO timing functions are
> already a problem for Go, and this is worse.

If targeting the same functionality (i.e. no exit callback), my API uses exactly 24 bytes more than Sean's. Is it really the case that those 24 bytes will break Go?

> 
> And with my vDSO hat back off, I find it disappointing that SGX SDKs
> seem willing to couple the SGX enclaves so tightly to their host ABIs.
> An *unmodified* SGX enclave should be able to run, without excessive
> annoyance, in a Windows process, a Linux process, a C process, a Java
> process, a Go process, and pretty much any other process.  Saying “I’ll
> just recompile it” is a bad solution — for enclaves that use MRENCLAVE,
> you can’t, and for enclaves that use MRSIGNER, you need to deal with the
> fact the protecting the signing key is a big deal.
> Someone should be able to port the entire host program to a different
> language without losing secrets and without access to a signing key.

I'm not sure which SGX SDKs you are referring to. But for Intel SGX SDK, we defined our own ABI that is consistent between Windows and Linux - i.e. there's no technical problem to load on Windows an enclave built on Linux or vice versa. In terms of what programming languages they can work with, I have to say it was designed exclusively for C/C++. Fortunately, there's usually a "native" interface (e.g. JNI, cgo, etc.) supported by a language runtime so it hasn't been a roadblock so far. Alternatively, the enclave vendor could ship an enclave along with an "interface" shared object that encapsulates all of the marshaling specifics, then the combination of that enclave and its "interface" shared object may be able to work "universally", which should be close to what you want.

The idea we had, when Intel SGX SDK was designed, was that different SDKs would be developed for different languages to take advantage of specific language features. That is similar to different programming languages were invented to target different usages. As we all know, every programming language has both advantages and disadvantages, hence no single language dominates. And that same idea applies to SGX SDKs. If there existed an SDK that worked with everything, probably it wouldn't work well with anything.

> 
> Cedric, your proposal allows an enclave to muck with RSP, but not in a
> way that’s particularly pleasant.

From security perspective, it is SGX ISA, but NOT any particular ABI, that allows enclaves "to muck with RSP". 

> Since the ISA is set in stone, we
> can’t do anything about the enclave’s access to its caller’s registers.
> I would love to see a straightforward way to run an enclave such that it
> does not access the main untrusted stack at all — uRSP and uRBP should
> be arbitrary values passed in the untrusted code, and the values the
> enclave sets should be relayed back to the caller but otherwise not have
> any effect.  Sadly I see no way to do this short of using GSBASE to
> store the real untrusted stack pointer.

I understand your sadness. You are "hoping" SGX to be a sandbox technology (i.e. to prevent enclave from reaching out into the host) but that wasn't the security objective when SGX was defined.

Anyway, SGX is what it is. A restrictive ABI only takes away flexibilities from "good" enclaves but can NEVER restrict malicious ones, so Sean's ABI cannot offer what you want.

> Other than the segment bases, there appear to be literally zero
> untrusted registers that are reliably preserved across an enclave entry
> and exit.  I suppose we should use a syscall to help.

The good news is with CET, there are viable solutions to implement bi-directional protection as you would hope. You are more than welcome to ask me offline for more details.

> 
> Since the above tricks seem unlikely to make it into the kernel, I think
> we’re doing everyone a favor if the Linux APIs strongly encourage SDK
> authors to build enclaves in a way that they don’t make problematic
> assumptions about the untrusted world. I would really like to see
> enclaves generated by the Linux SDK work on Windows and vice versa.

As said in my previous email, this vDSO API isn't even compliant to x86_64 ABI and is absolutely NOT for average developers. Instead, host/enclave communications are expected to be handled by SDKs and those developers will be very aware of the limitations of their targeted environments, and will need the freedom to deploy optimal solutions. 

I understand your intention to advocate the programming model that you believe is "right". But there are 7 billion people on this planet and the "right" thing for you could be "wrong" for others, especially in future usages/situations that can't be foreseen today. Software is stacked, with the lower layers being more generic and higher layers being more specific. This vDSO API is sitting at the bottom of the stack, therefore shall be as generic as possible. A better approach to advocate your idea is to wrap it (i.e. to implement it using the more generic vDSO API as a subroutine) in a library for the public to choose (and you can imagine others bearing different ideas will do the same). Then good ideas will stand out!

-Cedric

  reply	other threads:[~2019-03-24  8:59 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-20 16:20 [PATCH v19,RESEND 00/27] Intel SGX1 support Jarkko Sakkinen
2019-03-20 16:20 ` [PATCH v19,RESEND 01/27] x86/cpufeatures: Add Intel-defined SGX feature bit Jarkko Sakkinen
2019-03-20 19:41   ` Neil Horman
2019-03-21 14:16     ` Jarkko Sakkinen
2019-03-20 16:20 ` [PATCH v19,RESEND 02/27] x86/cpufeatures: Add SGX sub-features (as Linux-defined bits) Jarkko Sakkinen
2019-03-20 16:20 ` [PATCH v19,RESEND 03/27] x86/msr: Add IA32_FEATURE_CONTROL.SGX_ENABLE definition Jarkko Sakkinen
2019-03-20 16:20 ` [PATCH v19,RESEND 04/27] x86/cpufeatures: Add Intel-defined SGX_LC feature bit Jarkko Sakkinen
2019-03-20 16:20 ` [PATCH v19,RESEND 05/27] x86/msr: Add SGX Launch Control MSR definitions Jarkko Sakkinen
2019-03-20 16:20 ` [PATCH v19,RESEND 06/27] x86/mm: x86/sgx: Add new 'PF_SGX' page fault error code bit Jarkko Sakkinen
2019-03-20 16:20 ` [PATCH v19,RESEND 07/27] x86/mm: x86/sgx: Signal SIGSEGV for userspace #PFs w/ PF_SGX Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 08/27] x86/cpu/intel: Detect SGX support and update caps appropriately Jarkko Sakkinen
2019-03-26 12:17   ` Huang, Kai
2019-03-26 14:27     ` Sean Christopherson
2019-03-26 21:25       ` Huang, Kai
2019-03-26 21:57         ` Sean Christopherson
2019-03-26 23:19           ` Huang, Kai
2019-03-20 16:21 ` [PATCH v19,RESEND 09/27] x86/sgx: Add ENCLS architectural error codes Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 10/27] x86/sgx: Add SGX1 and SGX2 architectural data structures Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 11/27] x86/sgx: Add definitions for SGX's CPUID leaf and variable sub-leafs Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 12/27] x86/sgx: Enumerate and track EPC sections Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 13/27] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 14/27] x86/sgx: Add functions to allocate and free EPC pages Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 15/27] x86/sgx: Add sgx_einit() for initializing enclaves Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 16/27] x86/sgx: Add the Linux SGX Enclave Driver Jarkko Sakkinen
2019-03-26 12:01   ` Huang, Kai
2019-03-26 12:40     ` Thomas Gleixner
2019-03-26 14:54       ` Sean Christopherson
2019-03-26 21:11         ` Huang, Kai
2019-03-27  5:02     ` Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 17/27] x86/sgx: Add provisioning Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 18/27] x86/sgx: Add swapping code to the core and SGX driver Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 19/27] x86/sgx: ptrace() support for the " Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 20/27] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 21/27] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 22/27] x86/fault: Attempt to fixup unhandled #PF in vDSO before signaling Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 23/27] x86/traps: Attempt to fixup exceptions " Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 24/27] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions Jarkko Sakkinen
2019-03-20 18:30   ` Xing, Cedric
2019-03-20 18:52     ` Andy Lutomirski
2019-03-20 19:57       ` Xing, Cedric
2019-03-20 21:03         ` Sean Christopherson
2019-03-21  0:17           ` Xing, Cedric
2019-03-22 21:20             ` Sean Christopherson
2019-03-21 17:17         ` Andy Lutomirski
2019-03-22 20:31           ` Xing, Cedric
2019-03-20 19:02     ` Jethro Beekman
2019-03-20 20:10       ` Xing, Cedric
2019-03-20 19:13     ` Sean Christopherson
2019-03-20 20:38       ` Xing, Cedric
2019-03-22 21:59         ` Sean Christopherson
2019-03-23 17:36           ` Xing, Cedric
2019-03-23 21:38             ` Andy Lutomirski
2019-03-24  8:59               ` Xing, Cedric [this message]
2019-03-25 18:03                 ` Sean Christopherson
2019-03-25 23:59                   ` Andy Lutomirski
2019-03-26  4:53                     ` Xing, Cedric
2019-03-26 17:08                       ` Andy Lutomirski
2019-03-28  4:23                         ` Xing, Cedric
2019-03-28 19:18                           ` Andy Lutomirski
2019-03-28 23:19                             ` Xing, Cedric
2019-03-29  9:48                               ` Jarkko Sakkinen
2019-03-31  8:43                                 ` Dr. Greg
2019-04-03 23:03                             ` Sean Christopherson
2019-03-25 23:54                 ` Andy Lutomirski
2019-03-26  4:16                   ` Xing, Cedric
2019-03-20 16:21 ` [PATCH v19,RESEND 25/27] x86/sgx: SGX documentation Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 26/27] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2019-03-20 16:21 ` [PATCH v19,RESEND 27/27] x86/sgx: Update MAINTAINERS Jarkko Sakkinen

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=960B34DE67B9E140824F1DCDEC400C0F4E85E989@ORSMSX116.amr.corp.intel.com \
    --to=cedric.xing@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=greg@enjellic.com \
    --cc=haitao.huang@intel.com \
    --cc=haitao.huang@linux.intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=josh@joshtriplett.org \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=rientjes@google.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=serge.ayoun@intel.com \
    --cc=shay.katz-zamir@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).