Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Nathaniel McCallum <npmccallum@redhat.com>
Cc: Jethro Beekman <jethro@fortanix.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	linux-kernel@vger.kernel.org, x86@kernel.org,
	linux-sgx@vger.kernel.org, akpm@linux-foundation.org,
	dave.hansen@intel.com, Neil Horman <nhorman@redhat.com>,
	"Huang, Haitao" <haitao.huang@intel.com>,
	andriy.shevchenko@linux.intel.com, tglx@linutronix.de, "Svahn,
	Kai" <kai.svahn@intel.com>,
	bp@alien8.de, Josh Triplett <josh@joshtriplett.org>,
	luto@kernel.org, kai.huang@intel.com,
	David Rientjes <rientjes@google.com>,
	cedric.xing@intel.com, Patrick Uiterwijk <puiterwijk@redhat.com>,
	Andy Lutomirski <luto@amacapital.net>,
	Connor Kuehl <ckuehl@redhat.com>,
	Harald Hoyer <harald@redhat.com>,
	Lily Sturmann <lsturman@redhat.com>
Subject: Re: [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX enclave call
Date: Mon, 16 Mar 2020 10:17:20 -0700
Message-ID: <20200316171720.GG24267@linux.intel.com> (raw)
In-Reply-To: <CAOASepNZZ8rGmJNvSdFHbtYpJkfkHp4vAdDFOmR4BcuOcCRgNQ@mail.gmail.com>

On Mon, Mar 16, 2020 at 10:03:31AM -0400, Nathaniel McCallum wrote:
> On Mon, Mar 16, 2020 at 9:59 AM Jethro Beekman <jethro@fortanix.com> wrote:
> >
> > On 2020-03-16 14:57, Nathaniel McCallum wrote:
> > > On Mon, Mar 16, 2020 at 9:32 AM Jethro Beekman <jethro@fortanix.com> wrote:
> > >>
> > >> On 2020-03-15 18:53, Nathaniel McCallum wrote:
> > >>> On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen
> > >>> <jarkko.sakkinen@linux.intel.com> wrote:
> > >>>>
> > >>>> On Wed, Mar 11, 2020 at 01:30:07PM -0400, Nathaniel McCallum wrote:
> > >>>>> Currently, the selftest has a wrapper around
> > >>>>> __vdso_sgx_enter_enclave() which preserves all x86-64 ABI callee-saved
> > >>>>> registers (CSRs), though it uses none of them. Then it calls this
> > >>>>> function which uses %rbx but preserves none of the CSRs. Then it jumps
> > >>>>> into an enclave which zeroes all these registers before returning.
> > >>>>> Thus:
> > >>>>>
> > >>>>>   1. wrapper saves all CSRs
> > >>>>>   2. wrapper repositions stack arguments
> > >>>>>   3. __vdso_sgx_enter_enclave() modifies, but does not save %rbx
> > >>>>>   4. selftest zeros all CSRs
> > >>>>>   5. wrapper loads all CSRs
> > >>>>>
> > >>>>> I'd like to propose instead that the enclave be responsible for saving
> > >>>>> and restoring CSRs. So instead of the above we have:
> > >>>>>   1. __vdso_sgx_enter_enclave() saves %rbx
> > >>>>>   2. enclave saves CSRs
> > >>>>>   3. enclave loads CSRs
> > >>>>>   4. __vdso_sgx_enter_enclave() loads %rbx
> > >>>>>
> > >>>>> I know that lots of other stuff happens during enclave transitions,
> > >>>>> but at the very least we could reduce the number of instructions
> > >>>>> through this critical path.
> > >>>>
> > >>>> What Jethro said and also that it is a good general principle to cut
> > >>>> down the semantics of any vdso as minimal as possible.
> > >>>>
> > >>>> I.e. even if saving RBX would make somehow sense it *can* be left
> > >>>> out without loss in terms of what can be done with the vDSO.
> > >>>
> > >>> Please read the rest of the thread. Sean and I have hammered out some
> > >>> sensible and effective changes.
> > >>
> > >> I'm not sure they're sensible? By departing from the ENCLU calling
> > >> convention, both the VDSO and the wrapper become more complicated.
> > >
> > > For the vDSO, only marginally. I'm counting +4,-2 instructions in my
> > > suggestions. For the wrapper, things become significantly simpler.
> > >
> > >> The wrapper because now it needs to implement all kinds of logic for
> > >> different behavior depending on whether the VDSO is or isn't available.

How so?  The wrapper, if one is needed, will need to have dedicated logic
for the vDSO no matter what interface is defined by the vDSO.  Taking the
leaf in %rcx instead of %rax would at worst add a single instruction.  At
best, it would eliminate the wrapper entirely by making the vDSO callable
from C, e.g. for enclaves+runtimes that treat EENTER/ERESUME as glorified
function calls, i.e. more or less follow the x86-64 ABI.

> > > When isn't the vDSO available?
> >
> > When you're not on Linux. Or when you're on an old kernel.
> 
> I fail to see why the Linux kernel should degrade its new interfaces for
> those use cases.

There are effectively four related, but independent, changes to consider:

  1. Make the RSP fixup in the "return from handler" path relative instead
     of absolute.

  2. Preserve RBX in the vDSO.

  3. Use %rcx instead of %rax to pass @leaf.

  4. Allow the untrusted runtime to pass a parameter directly to its exit
     handler.


For me, #1 is an easy "yes".  It's arguably a bug fix, and the cost is one
uop.

My vote for #2 and #3 would also be a strong "yes".  Although passing @leaf
in %rcx technically diverges from ENCLU, I actually think it will make it
easier to swap between the vDSO and a bare ENCLU.  E.g. have the prototype
for the vDSO be the prototype for the assembly wrapper:

typedef void (*enter_enclave_fn)(unsigned long rdi, unsigned long rsi,
				 unsigned long rdx, unsigned int leaf,
				 unsigned long r8,  unsigned long r9,
				 void *tcs,
				 struct sgx_enclave_exception *e,
				 sgx_enclave_exit_handler_t handler);

int run_enclave(...)
{
	enter_enclave_fn enter_enclave;

	if (vdso)
		enter_enclave = vdso;
	else
		enter_enclave = my_wrapper;
	return enter_enclave(...);
}


I don't have a strong opinion on #4.  It seems superfluous, but if the
parameter is buried at the end of the prototype then it can be completely
ignored by runtimes that don't utilize a handler.

  reply index

Thread overview: 109+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-03 23:35 [PATCH v28 00/22] Intel SGX foundations Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 01/22] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 02/22] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 03/22] x86/cpufeatures: x86/msr: Intel SGX Launch Control " Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 04/22] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 05/22] x86/sgx: Add SGX microarchitectural data structures Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 06/22] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2020-03-09 21:14   ` Sean Christopherson
2020-03-03 23:35 ` [PATCH v28 07/22] x86/cpu/intel: Detect SGX supprt Jarkko Sakkinen
2020-03-09 21:56   ` Sean Christopherson
2020-03-11 17:03     ` Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 08/22] x86/sgx: Enumerate and track EPC sections Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 09/22] x86/sgx: Add functions to allocate and free EPC pages Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 10/22] mm: Introduce vm_ops->may_mprotect() Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 11/22] x86/sgx: Linux Enclave Driver Jarkko Sakkinen
2020-03-05 17:40   ` Sean Christopherson
2020-03-05 18:24     ` Jethro Beekman
2020-03-05 19:04       ` Sean Christopherson
2020-03-06 19:00       ` Jarkko Sakkinen
2020-03-19 18:22         ` Dr. Greg
2020-03-06 18:58     ` Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 12/22] docs: x86/sgx: Document SGX micro architecture and kernel internals Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 13/22] selftests/x86: Recurse into subdirectories Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 14/22] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-03-04 19:27   ` Nathaniel McCallum
2020-03-05 11:33     ` Jarkko Sakkinen
2020-03-06 15:42       ` Dr. Greg
2020-03-06 19:07         ` Jarkko Sakkinen
2020-03-07 17:42           ` Dr. Greg
2020-03-10 13:08             ` Jarkko Sakkinen
2020-03-11 13:28       ` Jarkko Sakkinen
     [not found]         ` <20200311164047.GG21852@linux.intel.com>
2020-03-13 19:24           ` Jarkko Sakkinen
2020-03-04 19:44   ` Nathaniel McCallum
2020-03-04 19:51   ` Nathaniel McCallum
2020-03-06  5:32   ` Dr. Greg
2020-03-06 19:04     ` Jarkko Sakkinen
2020-03-10 19:29     ` Haitao Huang
2020-03-11  9:13       ` Dr. Greg
2020-03-11 17:15         ` Haitao Huang
2020-03-17  1:07       ` Dr. Greg
2020-03-03 23:36 ` [PATCH v28 15/22] x86/sgx: Add provisioning Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 16/22] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-03-05 19:03   ` Sean Christopherson
2020-03-06 18:47     ` Jarkko Sakkinen
2020-03-12 18:38       ` Sean Christopherson
2020-03-15  0:27         ` Jarkko Sakkinen
2020-03-15  1:17           ` Jarkko Sakkinen
2020-03-09 21:16   ` Sean Christopherson
2020-03-03 23:36 ` [PATCH v28 17/22] x86/sgx: ptrace() support for the SGX driver Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 18/22] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 19/22] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 20/22] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen
2020-03-11 17:30   ` Nathaniel McCallum
2020-03-11 17:38     ` Jethro Beekman
2020-03-11 19:15       ` Nathaniel McCallum
2020-03-13 15:48       ` Nathaniel McCallum
2020-03-13 16:46         ` Sean Christopherson
2020-03-13 18:32           ` Nathaniel McCallum
2020-03-13 18:44             ` Sean Christopherson
2020-03-13 20:14               ` Nathaniel McCallum
2020-03-13 22:08                 ` Sean Christopherson
2020-03-14 14:10                   ` Nathaniel McCallum
2020-03-18 23:40                     ` Sean Christopherson
2020-03-19  0:38                       ` Xing, Cedric
2020-03-19  1:03                         ` Sean Christopherson
2020-03-20 13:55                       ` Nathaniel McCallum
2020-03-15  1:25     ` Jarkko Sakkinen
2020-03-15 17:53       ` Nathaniel McCallum
2020-03-16 13:31         ` Jethro Beekman
2020-03-16 13:57           ` Nathaniel McCallum
2020-03-16 13:59             ` Jethro Beekman
2020-03-16 14:03               ` Nathaniel McCallum
2020-03-16 17:17                 ` Sean Christopherson [this message]
2020-03-16 21:27             ` Jarkko Sakkinen
2020-03-16 21:29               ` Jarkko Sakkinen
2020-03-16 22:55           ` Sean Christopherson
2020-03-16 23:56             ` Xing, Cedric
2020-03-18 22:01               ` Jarkko Sakkinen
2020-03-18 22:18                 ` Jarkko Sakkinen
2020-03-16 13:56         ` Jarkko Sakkinen
2020-03-16 14:01           ` Nathaniel McCallum
2020-03-16 21:38             ` Jarkko Sakkinen
2020-03-16 22:53               ` Sean Christopherson
2020-03-16 23:50                 ` Xing, Cedric
2020-03-16 23:59                   ` Sean Christopherson
2020-03-17  0:18                     ` Xing, Cedric
2020-03-17  0:27                       ` Sean Christopherson
2020-03-17 16:37                         ` Nathaniel McCallum
2020-03-17 16:50                       ` Nathaniel McCallum
2020-03-17 21:40                         ` Xing, Cedric
2020-03-17 22:09                           ` Sean Christopherson
2020-03-17 22:36                             ` Xing, Cedric
2020-03-17 23:57                               ` Sean Christopherson
2020-03-17 22:23                         ` Xing, Cedric
2020-03-18 13:01                           ` Nathaniel McCallum
2020-03-20 15:53                             ` Nathaniel McCallum
2020-03-17 16:28                 ` Nathaniel McCallum
2020-03-18 22:58                   ` Jarkko Sakkinen
2020-03-18 22:39                 ` Jarkko Sakkinen
2020-03-11 19:30   ` Nathaniel McCallum
2020-03-13  0:52     ` Sean Christopherson
2020-03-13 16:07       ` Nathaniel McCallum
2020-03-13 16:33         ` Sean Christopherson
2020-03-03 23:36 ` [PATCH v28 22/22] selftests/x86: Add vDSO selftest for SGX Jarkko Sakkinen
2020-03-04 19:24 ` [PATCH v28 00/22] Intel SGX foundations Nathaniel McCallum
2020-03-17 16:00 ` Jordan Hand
2020-03-18 21:56   ` Jarkko Sakkinen
2020-03-19 17:16   ` Dr. Greg

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=20200316171720.GG24267@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=ckuehl@redhat.com \
    --cc=dave.hansen@intel.com \
    --cc=haitao.huang@intel.com \
    --cc=harald@redhat.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=lsturman@redhat.com \
    --cc=luto@amacapital.net \
    --cc=luto@kernel.org \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=puiterwijk@redhat.com \
    --cc=rientjes@google.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

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/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-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org
	public-inbox-index linux-sgx

Example config snippet for mirrors

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


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