Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: "Xing, Cedric" <cedric.xing@intel.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>,
	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>,
	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: Wed, 18 Mar 2020 17:38:29 -0700
Message-ID: <858d19ed-868b-b4be-cbac-6cb92349d8fb@intel.com> (raw)
In-Reply-To: <20200318234057.GE26164@linux.intel.com>

On 3/18/2020 4:40 PM, Sean Christopherson wrote:
> On Sat, Mar 14, 2020 at 10:10:26AM -0400, Nathaniel McCallum wrote:
>> On Fri, Mar 13, 2020 at 6:08 PM Sean Christopherson
>> <sean.j.christopherson@intel.com> wrote:
>>>>>> 4. sub/add to %rsp rather than save/restore
>>>>>
>>>>> Can you elaborate on why you want to sub/add to %rsp instead of having the
>>>>> enclave unwind the stack?  Preserving %rsp across EEXIT/ERESUME seems more
>>>>> in line with function call semantics, which I assume is desirable?  E.g.
>>>>>
>>>>>    push param3
>>>>>    push param2
>>>>>    push param1
>>>>>
>>>>>    enclu[EEXIT]
>>>>>
>>>>>    add $0x18, %rsp
>>>>
>>>> Before enclave EEXIT, the enclave restores %rsp to the value it had
>>>> before EENTER was called. Then it pushes additional output arguments
>>>> onto the stack. The enclave calls EENCLU[EEXIT].
>>>>
>>>> We are now in __vdso...() on the way back to the caller. However, %rsp
>>>> has a different value than we entered the function with. This breaks
>>>> x86_64 ABI, obviously. The handler needs to fix this up: how does it
>>>> do so?
> 
> Circling back to this request, because I just realized that the above is
> handled by saving %rsp into %rbp and requiring the enclave and handler
> to preserve %rbp at all times.
> 
> So the below discussion on making the %rsp adjustment relative is moot,
> at least with respect to getting out of __vdso() if the enclave has mucked
> with the untrusted stack.
> 
I didn't follow the discussion closely enough to understand the 
motivation behind "add/sub" rather than "restore" %rsp. Now I understand 
and I agree with you that the requested change is unnecessary.

>>>> In the current code, __vdso..() saves the value of %rsp, calls the
>>>> handler and then restores %rsp. The handler can fix up the stack by
>>>> setting the correct value to %rbx and returning without restoring it.
>>>
>>> Ah, you're referring to the patch where the handler decides to return all
>>> the way back to the caller of __vdso...().
>>>
>>>> But this requires internal knowledge of the __vdso...() function,
>>>> which could theoretically change in the future.
>>>>
>>>> If instead the __vdso...() only did add/sub, then the handler could do:
>>>> 1. pop return address
>>>> 2. pop handler stack params
>>>> 3. pop enclave additional output stack params
>>>> 4. push handler stack params
>>>> 5. push return address
> 
> Per above, this is unnecessary when returning to the caller of __vdso().
> It would be necessary if the enclave wasn't smart enough to do it's own
> stack cleanup, but that seems like a very bizarre contract between the
> enclave and its runtime.
> 
> The caveat is if %rbx is saved/restored by __vdso().  If we want a
> traditional frame pointer, then %rbx would be restored from the stack
> before %rsp itself is restored (from %rbp), in which case the exit handler
> would need to adjust %rsp using a sequence similar to what you listed
> above.
> 
> If __vdso() uses a non-standard frame pointer, e.g.
> 
>    push %rbp
>    push %rbx
>    mov  %rsp, %rbp
> 
> then %rbx would come off the stack after %rsp is restored from %rbp, i.e.
> would be guaranteed to be restored to the pre-EENTER value (unless the
> enclave or handler mucks with %rbp).
> 
> Anyways, we can discuss how to implement the frame pointer in the context
> of the patches, just wanted to point this out here for completeness.
> 
%rbx can always be restored as long as it is saved at a fixed offset 
from %rbp. For example, given the standard prolog below:

	push	%rbp
	mov	%rsp, %rbp
	push	%rbx

It can be paired with the following standard epilog:

	mov	-8(%rbp), %rbx
	leave
	ret

Alternatively, given "red zone" of 128 bytes, the following epilog will 
also work:

	leave
	mov	-0x10(%rsp), %rbx
	ret

In no cases do we have to worry about enclave mucking the stack as long 
as %rbp is preserved.

>>>> While this is more work, it is standard calling convention work that
>>>> doesn't require internal knowledge of __vdso..(). Alternatively, if we
>>>> don't like the extra work, we can document the %rbx hack explicitly
>>>> into the handler documentation and make it part of the interface. But
>>>> we need some explicit way for the handler to pop enclave output stack
>>>> params that doesn't depend on internal knowledge of the __vdso...()
>>>> invariants.
>>>
>>> IIUC, this is what you're suggesting?  Having to align the stack makes this
>>> a bit annoying, but it's not bad by any means.
>>>
>>> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
>>> index 94a8e5f99961..05d54f79b557 100644
>>> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
>>> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
>>> @@ -139,8 +139,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>>>          /* Pass the untrusted RSP (at exit) to the callback via %rcx. */
>>>          mov     %rsp, %rcx
>>>
>>> -       /* Save the untrusted RSP in %rbx (non-volatile register). */
>>> +       /* Save the untrusted RSP offset in %rbx (non-volatile register). */
>>>          mov     %rsp, %rbx
>>> +       and     $0xf, %rbx
>>>
>>>          /*
>>>           * Align stack per x86_64 ABI. Note, %rsp needs to be 16-byte aligned
>>> @@ -161,8 +162,8 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>>>          mov     0x20(%rbp), %rax
>>>          call    .Lretpoline
>>>
>>> -       /* Restore %rsp to its post-exit value. */
>>> -       mov     %rbx, %rsp
>>> +       /* Undo the post-exit %rsp adjustment. */
>>> +       lea     0x20(%rsp,%rbx), %rsp
>>>

Per discussion above, this is useful only if the enclave has problem 
cleaning up its own mess left on the untrusted stack, and the exit 
handler wants to EENTER the enclave again by returning to __vdso...(). 
It sounds very uncommon to me, and more like a bug than an expected 
behavior. Are there any existing code doing this or any particular 
application that needs this. If no, I'd say not to do it.

  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 [this message]
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
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=858d19ed-868b-b4be-cbac-6cb92349d8fb@intel.com \
    --to=cedric.xing@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --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=sean.j.christopherson@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

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