Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Sean Christopherson]

+Cc: linux-sgx, Haitao, Greg and Jethro

My apologies for neglecting to cc the SGX folks, original thread is here:

https://lkml.kernel.org/r/20181206221922.31012-1-sean.j.christopherson@intel.com

On Thu, Dec 06, 2018 at 02:50:01PM -0800, Andy Lutomirski wrote:
> On Thu, Dec 6, 2018 at 2:19 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
>  +
> > +       /*
> > +        * Invoke the caller's exit handler if one was provided.  The return
> > +        * value tells us whether to re-enter the enclave (EENTER or ERESUME)
> > +        * or to return (EEXIT).
> > +        */
> > +       if (exit_handler) {
> > +               leaf = exit_handler(exit_info, tcs, priv);
> > +               if (leaf == SGX_EENTER || leaf == SGX_ERESUME)
> > +                       goto enter_enclave;
> > +               if (leaf == SGX_EEXIT)
> > +                       return 0;
> > +               return -EINVAL;
> > +       } else if (leaf != SGX_EEXIT) {
> > +               return -EFAULT;
> > +       }
> 
> This still seems overcomplicated to me.  How about letting the
> requested leaf (EENTER or ERESUME) be a parameter to the function and
> then just returning here?  As it stands, you're requiring any ERESUME
> that gets issued (other than the implicit ones) to be issued in the
> same call stack, which is very awkward if you're doing something like
> forwarding the fault to a different task over a socket and then
> waiting in epoll_wait() or similar before resuming the enclave.

Ah, yeah, wasn't thinking about usage models where the enclave could
get passed off to a different thread.

What about supporting both, i.e. keep the exit handler but make it 100%
optional?  And simplify the exit_handler to effectively return a boolean,
i.e. "exit or continue".

Something like this:

notrace long __vdso_sgx_enter_enclave(u32 op, void *tcs, void *priv,
				      struct sgx_enclave_exit_info *exit_info,
				      sgx_enclave_exit_handler *exit_handler)
{
	u64 rdi, rsi, rdx;
	u32 leaf;
	long ret;

	if (!tcs || !exit_info)
		return -EINVAL;

enter_enclave:
	if (op != SGX_EENTER && op != SGX_ERESUME)
		return -EINVAL;

        <same core code>

	/*
	 * Invoke the caller's exit handler if one was provided.  The return
	 * value tells us whether to re-enter the enclave (EENTER or ERESUME)
	 * or to return (EEXIT).
	 */
	if (exit_handler) {
		if (exit_handler(exit_info, tcs, priv)) {
			op = exit_info->leaf;
			goto enter_enclave;
		}
	}

	if (exit_info->leaf == SGX_EEXIT)
		return -EFAULT;

	return 0;
}


I like that the exit handler allows userspace to trap/panic with the full
call stack in place, and in a dedicated path, i.e. outside of the basic
enter/exit code.  An exit handler probably doesn't fundamentally change
what userspace can do with respect to debugging/reporting, but I think
it would actually simplify some userspace implementations, e.g. I'd use
it in my tests like so:

long fault_handler(struct sgx_enclave_exit_info *exit_info, void *tcs, void *priv)
{
	if (exit_info->leaf == SGX_EEXIT)
		return 0;

	<report exception and die/hang>
}

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Andy Lutomirski]

On Fri, Dec 7, 2018 at 8:51 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> +Cc: linux-sgx, Haitao, Greg and Jethro
>
> My apologies for neglecting to cc the SGX folks, original thread is here:
>
> https://lkml.kernel.org/r/20181206221922.31012-1-sean.j.christopherson@intel.com
>
> On Thu, Dec 06, 2018 at 02:50:01PM -0800, Andy Lutomirski wrote:
> > On Thu, Dec 6, 2018 at 2:19 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >
> >  +
> > > +       /*
> > > +        * Invoke the caller's exit handler if one was provided.  The return
> > > +        * value tells us whether to re-enter the enclave (EENTER or ERESUME)
> > > +        * or to return (EEXIT).
> > > +        */
> > > +       if (exit_handler) {
> > > +               leaf = exit_handler(exit_info, tcs, priv);
> > > +               if (leaf == SGX_EENTER || leaf == SGX_ERESUME)
> > > +                       goto enter_enclave;
> > > +               if (leaf == SGX_EEXIT)
> > > +                       return 0;
> > > +               return -EINVAL;
> > > +       } else if (leaf != SGX_EEXIT) {
> > > +               return -EFAULT;
> > > +       }
> >
> > This still seems overcomplicated to me.  How about letting the
> > requested leaf (EENTER or ERESUME) be a parameter to the function and
> > then just returning here?  As it stands, you're requiring any ERESUME
> > that gets issued (other than the implicit ones) to be issued in the
> > same call stack, which is very awkward if you're doing something like
> > forwarding the fault to a different task over a socket and then
> > waiting in epoll_wait() or similar before resuming the enclave.
>
> Ah, yeah, wasn't thinking about usage models where the enclave could
> get passed off to a different thread.
>
> What about supporting both, i.e. keep the exit handler but make it 100%
> optional?  And simplify the exit_handler to effectively return a boolean,
> i.e. "exit or continue".
>
> Something like this:
>
> notrace long __vdso_sgx_enter_enclave(u32 op, void *tcs, void *priv,
>                                       struct sgx_enclave_exit_info *exit_info,
>                                       sgx_enclave_exit_handler *exit_handler)
> {
>         u64 rdi, rsi, rdx;
>         u32 leaf;
>         long ret;
>
>         if (!tcs || !exit_info)
>                 return -EINVAL;
>
> enter_enclave:
>         if (op != SGX_EENTER && op != SGX_ERESUME)
>                 return -EINVAL;
>
>         <same core code>
>
>         /*
>          * Invoke the caller's exit handler if one was provided.  The return
>          * value tells us whether to re-enter the enclave (EENTER or ERESUME)
>          * or to return (EEXIT).
>          */
>         if (exit_handler) {
>                 if (exit_handler(exit_info, tcs, priv)) {
>                         op = exit_info->leaf;
>                         goto enter_enclave;
>                 }
>         }
>
>         if (exit_info->leaf == SGX_EEXIT)
>                 return -EFAULT;
>
>         return 0;
> }
>
>
> I like that the exit handler allows userspace to trap/panic with the full
> call stack in place, and in a dedicated path, i.e. outside of the basic
> enter/exit code.  An exit handler probably doesn't fundamentally change
> what userspace can do with respect to debugging/reporting, but I think
> it would actually simplify some userspace implementations, e.g. I'd use
> it in my tests like so:
>
> long fault_handler(struct sgx_enclave_exit_info *exit_info, void *tcs, void *priv)
> {
>         if (exit_info->leaf == SGX_EEXIT)
>                 return 0;
>
>         <report exception and die/hang>
> }
>

Hmm.  That't not totally silly, although you could accomplish almost
the same thing by wrapping the vDSO helper in another function.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Sean Christopherson]

On Fri, Dec 07, 2018 at 09:56:09AM -0800, Andy Lutomirski wrote:
> On Fri, Dec 7, 2018 at 8:51 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > I like that the exit handler allows userspace to trap/panic with the full
> > call stack in place, and in a dedicated path, i.e. outside of the basic
> > enter/exit code.  An exit handler probably doesn't fundamentally change
> > what userspace can do with respect to debugging/reporting, but I think
> > it would actually simplify some userspace implementations, e.g. I'd use
> > it in my tests like so:
> >
> > long fault_handler(struct sgx_enclave_exit_info *exit_info, void *tcs, void *priv)
> > {
> >         if (exit_info->leaf == SGX_EEXIT)
> >                 return 0;
> >
> >         <report exception and die/hang>
> > }
> >
> 
> Hmm.  That't not totally silly, although you could accomplish almost
> the same thing by wrapping the vDSO helper in another function.

True, but I think there's value in having the option to intercept an
exception at the exact(ish) point of failure, without having to return
up the stack.

The enclave has full access to the process' memory space, including the
untrsuted stack.  It's not too far fetched to envision a scenario where
the enclave corrupts the stack even if the enclave isn't intentionally
using the stack, e.g. the host passes in variable that a points at the
stack instead of whatever memory is supposed to be shared between the
enclave and host.  It'd be nice to have the ability to triage something
like that without having to e.g. configure breakpoints on the stack.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Andy Lutomirski]

> On Dec 7, 2018, at 11:02 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
>> On Fri, Dec 07, 2018 at 09:56:09AM -0800, Andy Lutomirski wrote:
>> On Fri, Dec 7, 2018 at 8:51 AM Sean Christopherson
>> <sean.j.christopherson@intel.com> wrote:
>>> I like that the exit handler allows userspace to trap/panic with the full
>>> call stack in place, and in a dedicated path, i.e. outside of the basic
>>> enter/exit code.  An exit handler probably doesn't fundamentally change
>>> what userspace can do with respect to debugging/reporting, but I think
>>> it would actually simplify some userspace implementations, e.g. I'd use
>>> it in my tests like so:
>>> 
>>> long fault_handler(struct sgx_enclave_exit_info *exit_info, void *tcs, void *priv)
>>> {
>>>        if (exit_info->leaf == SGX_EEXIT)
>>>                return 0;
>>> 
>>>        <report exception and die/hang>
>>> }
>>> 
>> 
>> Hmm.  That't not totally silly, although you could accomplish almost
>> the same thing by wrapping the vDSO helper in another function.
> 
> True, but I think there's value in having the option to intercept an
> exception at the exact(ish) point of failure, without having to return
> up the stack.
> 
> The enclave has full access to the process' memory space, including the
> untrsuted stack.  It's not too far fetched to envision a scenario where
> the enclave corrupts the stack even if the enclave isn't intentionally
> using the stack, e.g. the host passes in variable that a points at the
> stack instead of whatever memory is supposed to be shared between the
> enclave and host.  It'd be nice to have the ability to triage something
> like that without having to e.g. configure breakpoints on the stack.

Ah, I see. You’re saying that, if the non-enclave stare is corrupted such that RIP  is okay and RSP still points somewhere reasonable but the return address is garbage, then we can at least get to the fault handler and print something?  This only works if the fault handler pointer itself is okay, though, which somewhat limits the usefulness, given that its pointer is quite likely to be on the stack very close to the return address.

I really wish the ENCLU instruction had seen fit to preserve some registers.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Sean Christopherson]

On Fri, Dec 07, 2018 at 11:23:10AM -0800, Andy Lutomirski wrote:
> 
> > On Dec 7, 2018, at 11:02 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> >> On Fri, Dec 07, 2018 at 09:56:09AM -0800, Andy Lutomirski wrote:
> >> On Fri, Dec 7, 2018 at 8:51 AM Sean Christopherson
> >> <sean.j.christopherson@intel.com> wrote:
> >>> I like that the exit handler allows userspace to trap/panic with the full
> >>> call stack in place, and in a dedicated path, i.e. outside of the basic
> >>> enter/exit code.  An exit handler probably doesn't fundamentally change
> >>> what userspace can do with respect to debugging/reporting, but I think
> >>> it would actually simplify some userspace implementations, e.g. I'd use
> >>> it in my tests like so:
> >>> 
> >>> long fault_handler(struct sgx_enclave_exit_info *exit_info, void *tcs, void *priv)
> >>> {
> >>>        if (exit_info->leaf == SGX_EEXIT)
> >>>                return 0;
> >>> 
> >>>        <report exception and die/hang>
> >>> }
> >>> 
> >> 
> >> Hmm.  That't not totally silly, although you could accomplish almost
> >> the same thing by wrapping the vDSO helper in another function.
> > 
> > True, but I think there's value in having the option to intercept an
> > exception at the exact(ish) point of failure, without having to return
> > up the stack.
> > 
> > The enclave has full access to the process' memory space, including the
> > untrsuted stack.  It's not too far fetched to envision a scenario where
> > the enclave corrupts the stack even if the enclave isn't intentionally
> > using the stack, e.g. the host passes in variable that a points at the
> > stack instead of whatever memory is supposed to be shared between the
> > enclave and host.  It'd be nice to have the ability to triage something
> > like that without having to e.g. configure breakpoints on the stack.
> 
> Ah, I see. You’re saying that, if the non-enclave stare is corrupted such
> that RIP  is okay and RSP still points somewhere reasonable but the return
> address is garbage, then we can at least get to the fault handler and print
> something?

Yep.  Even for something more subtle like GPR corruption it could dump the
entire call stack before attempting to return back up.

> This only works if the fault handler pointer itself is okay, though, which
> somewhat limits the usefulness, given that its pointer is quite likely to
> be on the stack very close to the return address.

Yeah, it's not a silver bullet by any means, but it does seem useful for at
least some scenarios.  Even exploding when invoking the handler instead of
at a random point might prove useful, e.g. "calling my exit handler exploded,
maybe my enclave corrupted the stack!".
 
> I really wish the ENCLU instruction had seen fit to preserve some registers.

Speaking of preserving registers, the asm blob needs to mark RBX as
clobbered since it's modified for EEXIT.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Andy Lutomirski]

> On Dec 7, 2018, at 12:09 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
>> On Fri, Dec 07, 2018 at 11:23:10AM -0800, Andy Lutomirski wrote:
>> 
>>>> On Dec 7, 2018, at 11:02 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>>> 
>>>> On Fri, Dec 07, 2018 at 09:56:09AM -0800, Andy Lutomirski wrote:
>>>> On Fri, Dec 7, 2018 at 8:51 AM Sean Christopherson
>>>> <sean.j.christopherson@intel.com> wrote:
>>>>> I like that the exit handler allows userspace to trap/panic with the full
>>>>> call stack in place, and in a dedicated path, i.e. outside of the basic
>>>>> enter/exit code.  An exit handler probably doesn't fundamentally change
>>>>> what userspace can do with respect to debugging/reporting, but I think
>>>>> it would actually simplify some userspace implementations, e.g. I'd use
>>>>> it in my tests like so:
>>>>> 
>>>>> long fault_handler(struct sgx_enclave_exit_info *exit_info, void *tcs, void *priv)
>>>>> {
>>>>>       if (exit_info->leaf == SGX_EEXIT)
>>>>>               return 0;
>>>>> 
>>>>>       <report exception and die/hang>
>>>>> }
>>>>> 
>>>> 
>>>> Hmm.  That't not totally silly, although you could accomplish almost
>>>> the same thing by wrapping the vDSO helper in another function.
>>> 
>>> True, but I think there's value in having the option to intercept an
>>> exception at the exact(ish) point of failure, without having to return
>>> up the stack.
>>> 
>>> The enclave has full access to the process' memory space, including the
>>> untrsuted stack.  It's not too far fetched to envision a scenario where
>>> the enclave corrupts the stack even if the enclave isn't intentionally
>>> using the stack, e.g. the host passes in variable that a points at the
>>> stack instead of whatever memory is supposed to be shared between the
>>> enclave and host.  It'd be nice to have the ability to triage something
>>> like that without having to e.g. configure breakpoints on the stack.
>> 
>> Ah, I see. You’re saying that, if the non-enclave stare is corrupted such
>> that RIP  is okay and RSP still points somewhere reasonable but the return
>> address is garbage, then we can at least get to the fault handler and print
>> something?
> 
> Yep.  Even for something more subtle like GPR corruption it could dump the
> entire call stack before attempting to return back up.
> 
>> This only works if the fault handler pointer itself is okay, though, which
>> somewhat limits the usefulness, given that its pointer is quite likely to
>> be on the stack very close to the return address.
> 
> Yeah, it's not a silver bullet by any means, but it does seem useful for at
> least some scenarios.  Even exploding when invoking the handler instead of
> at a random point might prove useful, e.g. "calling my exit handler exploded,
> maybe my enclave corrupted the stack!".

Here’s another idea: calculate some little hash or other checksum of RSP, RBP, and perhaps a couple words on the stack, and do:

call __vdso_enclave_corrupted_state

If you get a mismatch after return. That function could be:

call __vdso_enclave_corrupted_state:
  ud2

And now the debug trace makes it very clear what happened.

This may or may not be worth the effort. But ISTM the enclave is almost as likely to corrupt the host state and the. EEXIT as it is to corrupt the host state and then fault.

BTW, I read through Fortanix’s documentation of the Windows SGX calling conventions, and it seems to want RSI and RDI as out params.  Letting the vDSO be used to invoke Windows-targeted enclaves seems useful.

> 
>> I really wish the ENCLU instruction had seen fit to preserve some registers.
> 
> Speaking of preserving registers, the asm blob needs to mark RBX as
> clobbered since it's modified for EEXIT.

Have fun with that.  The x86_32 compiler seems to really like having its PIC register preserved, and you may get some lovely compiler errors.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Sean Christopherson]

On Fri, Dec 07, 2018 at 12:16:59PM -0800, Andy Lutomirski wrote:
> 
> > On Dec 7, 2018, at 12:09 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> > Speaking of preserving registers, the asm blob needs to mark RBX as
> > clobbered since it's modified for EEXIT.
> 
> Have fun with that.  The x86_32 compiler seems to really like having its
> PIC register preserved, and you may get some lovely compiler errors.

Tagentinally related, as-is the SGX vDSO is only compiled for x86_64
since CONFIG_SGX depends on CONFIG_X86_64.  Mapping the EPC in 32-bit
mode complicates things and no one is asking for SGX support on 32-bit
builds, so...

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Sean Christopherson]

On Fri, Dec 07, 2018 at 12:16:59PM -0800, Andy Lutomirski wrote:
> 
> 
> > On Dec 7, 2018, at 12:09 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> >> On Fri, Dec 07, 2018 at 11:23:10AM -0800, Andy Lutomirski wrote:
> >> 
> >> Ah, I see. You’re saying that, if the non-enclave stare is corrupted such
> >> that RIP  is okay and RSP still points somewhere reasonable but the return
> >> address is garbage, then we can at least get to the fault handler and print
> >> something?
> > 
> > Yep.  Even for something more subtle like GPR corruption it could dump the
> > entire call stack before attempting to return back up.
> > 
> >> This only works if the fault handler pointer itself is okay, though, which
> >> somewhat limits the usefulness, given that its pointer is quite likely to
> >> be on the stack very close to the return address.
> > 
> > Yeah, it's not a silver bullet by any means, but it does seem useful for at
> > least some scenarios.  Even exploding when invoking the handler instead of
> > at a random point might prove useful, e.g. "calling my exit handler exploded,
> > maybe my enclave corrupted the stack!".
> 
> Here’s another idea: calculate some little hash or other checksum of
> RSP, RBP, and perhaps a couple words on the stack, and do:

Corrupting RSP and RBP as opposed to the stack memory seems much less
likely since the enclave would have to poke into the save state area.
And as much as I dislike the practice of intentionally manipulating
SSA.RSP, preventing the user from doing something because we're "helping"
doesn't seem right.

> call __vdso_enclave_corrupted_state
> 
> If you get a mismatch after return. That function could be:
> 
> call __vdso_enclave_corrupted_state:
>   ud2
> 
> And now the debug trace makes it very clear what happened.
>
> This may or may not be worth the effort.

Running a checksum on the stack for every exit doesn't seem like it'd
be worth the effort, especially since this type of bug should be quite
rare, at least in production environments.

If we want to pursue the checksum idea I think the easiest approach
would be to combine it with an exit_handler and do a simple check on
the handler.  It'd be minimal overhead in the fast path and would flag
cases where invoking exit_handle() would explode, while deferring all
other checks to the user.

E.g. something like this:

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.c b/arch/x86/entry/vdso/vsgx_enter_enclave.c
index d5145e5c5a54..c89dd3cd8da9 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.c
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.c
@@ -42,10 +42,13 @@ enum sgx_enclu_leaf {
        SGX_EEXIT       = 4,
 };

+#define VDSO_MAGIC 0xa5a5a5a5a5a5a5a5UL
+
 notrace long __vdso_sgx_enter_enclave(u32 op, void *tcs, void *priv,
                                      struct sgx_enclave_exit_info *exit_info,
                                      sgx_enclave_exit_handler *exit_handler)
 {
+       volatile unsigned long hash;
        u64 rdi, rsi, rdx;
        u32 leaf;
        long ret;
@@ -53,6 +56,9 @@ notrace long __vdso_sgx_enter_enclave(u32 op, void *tcs, void *priv,
        if (!tcs || !exit_info)
                return -EINVAL;

+       /* Always hash the handler.  XOR is much cheaper than Jcc. */
+       hash = (unsigned long)exit_handler ^ VDSO_MAGIC;
+
 enter_enclave:
        if (op != SGX_EENTER && op != SGX_ERESUME)
                return -EINVAL;
@@ -107,6 +113,8 @@ notrace long __vdso_sgx_enter_enclave(u32 op, void *tcs, void *priv,
         * or to return (EEXIT).
         */
        if (exit_handler) {
+               if (hash != ((unsigned long)exit_handler ^ VDSO_MAGIC))
+                       asm volatile("ud2\n");
                if (exit_handler(exit_info, tcs, priv)) {
                        op = exit_info->leaf;
                        goto enter_enclave;

> But ISTM the enclave is almost as likely to corrupt the host state and
> the. EEXIT as it is to corrupt the host state and then fault.

Agreed, I would say even more likely.  But the idea is that the
exit_handler is called on any exit, not just exceptions.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Andy Lutomirski]

On Fri, Dec 7, 2018 at 1:26 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, Dec 07, 2018 at 12:16:59PM -0800, Andy Lutomirski wrote:
> >
> >
> > > On Dec 7, 2018, at 12:09 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > >
> > >> On Fri, Dec 07, 2018 at 11:23:10AM -0800, Andy Lutomirski wrote:
> > >>
> > >> Ah, I see. You’re saying that, if the non-enclave stare is corrupted such
> > >> that RIP  is okay and RSP still points somewhere reasonable but the return
> > >> address is garbage, then we can at least get to the fault handler and print
> > >> something?
> > >
> > > Yep.  Even for something more subtle like GPR corruption it could dump the
> > > entire call stack before attempting to return back up.
> > >
> > >> This only works if the fault handler pointer itself is okay, though, which
> > >> somewhat limits the usefulness, given that its pointer is quite likely to
> > >> be on the stack very close to the return address.
> > >
> > > Yeah, it's not a silver bullet by any means, but it does seem useful for at
> > > least some scenarios.  Even exploding when invoking the handler instead of
> > > at a random point might prove useful, e.g. "calling my exit handler exploded,
> > > maybe my enclave corrupted the stack!".
> >
> > Here’s another idea: calculate some little hash or other checksum of
> > RSP, RBP, and perhaps a couple words on the stack, and do:
>
> Corrupting RSP and RBP as opposed to the stack memory seems much less
> likely since the enclave would have to poke into the save state area.
> And as much as I dislike the practice of intentionally manipulating
> SSA.RSP, preventing the user from doing something because we're "helping"
> doesn't seem right.
>
> > call __vdso_enclave_corrupted_state
> >
> > If you get a mismatch after return. That function could be:
> >
> > call __vdso_enclave_corrupted_state:
> >   ud2
> >
> > And now the debug trace makes it very clear what happened.
> >
> > This may or may not be worth the effort.
>
> Running a checksum on the stack for every exit doesn't seem like it'd
> be worth the effort, especially since this type of bug should be quite
> rare, at least in production environments.
>
> If we want to pursue the checksum idea I think the easiest approach
> would be to combine it with an exit_handler and do a simple check on
> the handler.  It'd be minimal overhead in the fast path and would flag
> cases where invoking exit_handle() would explode, while deferring all
> other checks to the user.

How about this variant?

#define MAGIC 0xaaaabbbbccccddddul
#define RETADDR_HASH ((unsigned long)__builtin_return_address(0) ^ MAGIC)

void foo(void)
{
    volatile unsigned long hash = RETADDR_HASH;

    /* placeholder for your actual code */
    asm volatile ("nop");

    if (hash != RETADDR_HASH)
        asm volatile ("ud2");
}

But I have a real argument for dropping exit_handler: in this new age
of Spectre, the indirect call is a retpoline, and it's therefore quite
slow.  So I'm not saying NAK, but I do think it's unnecessary.

I don't suppose you've spent a bunch of time programming in the
continuation-passing style? :)

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Sean Christopherson]

On Fri, Dec 07, 2018 at 03:33:57PM -0800, Andy Lutomirski wrote:
> On Fri, Dec 7, 2018 at 1:26 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > Running a checksum on the stack for every exit doesn't seem like it'd
> > be worth the effort, especially since this type of bug should be quite
> > rare, at least in production environments.
> >
> > If we want to pursue the checksum idea I think the easiest approach
> > would be to combine it with an exit_handler and do a simple check on
> > the handler.  It'd be minimal overhead in the fast path and would flag
> > cases where invoking exit_handle() would explode, while deferring all
> > other checks to the user.
> 
> How about this variant?
> 
> #define MAGIC 0xaaaabbbbccccddddul
> #define RETADDR_HASH ((unsigned long)__builtin_return_address(0) ^ MAGIC)
> 
> void foo(void)
> {
>     volatile unsigned long hash = RETADDR_HASH;
> 
>     /* placeholder for your actual code */
>     asm volatile ("nop");
> 
>     if (hash != RETADDR_HASH)
>         asm volatile ("ud2");
> }
> 
> But I have a real argument for dropping exit_handler: in this new age
> of Spectre, the indirect call is a retpoline, and it's therefore quite
> slow.

Technically slower, but would the extra CALL+RET pair even be noticeable
in the grand scheme of SGX?

> So I'm not saying NAK, but I do think it's unnecessary.

Ya, definitely not necessary, but it does allow userspace do things that
are otherwise cumbersome or impossible to do with the vanilla vDSO.  How
much value that actually adds is another question...

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Andy Lutomirski]

On Tue, Dec 11, 2018 at 11:31 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, Dec 07, 2018 at 03:33:57PM -0800, Andy Lutomirski wrote:
> > On Fri, Dec 7, 2018 at 1:26 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >
> > > Running a checksum on the stack for every exit doesn't seem like it'd
> > > be worth the effort, especially since this type of bug should be quite
> > > rare, at least in production environments.
> > >
> > > If we want to pursue the checksum idea I think the easiest approach
> > > would be to combine it with an exit_handler and do a simple check on
> > > the handler.  It'd be minimal overhead in the fast path and would flag
> > > cases where invoking exit_handle() would explode, while deferring all
> > > other checks to the user.
> >
> > How about this variant?
> >
> > #define MAGIC 0xaaaabbbbccccddddul
> > #define RETADDR_HASH ((unsigned long)__builtin_return_address(0) ^ MAGIC)
> >
> > void foo(void)
> > {
> >     volatile unsigned long hash = RETADDR_HASH;
> >
> >     /* placeholder for your actual code */
> >     asm volatile ("nop");
> >
> >     if (hash != RETADDR_HASH)
> >         asm volatile ("ud2");
> > }
> >
> > But I have a real argument for dropping exit_handler: in this new age
> > of Spectre, the indirect call is a retpoline, and it's therefore quite
> > slow.
>
> Technically slower, but would the extra CALL+RET pair even be noticeable
> in the grand scheme of SGX?

But it's CALL, CALL, MOV to overwrite return address, intentionally
midpredicted RET, and RET because Spectre.  That whole sequence seems
to be several tens of cycles, so it's a lot worse than just CALL+RET.
Whether it's noticeable overall is a fair question, though.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Sean Christopherson]

On Tue, Dec 11, 2018 at 12:04:15PM -0800, Andy Lutomirski wrote:
> On Tue, Dec 11, 2018 at 11:31 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Fri, Dec 07, 2018 at 03:33:57PM -0800, Andy Lutomirski wrote:
> > > On Fri, Dec 7, 2018 at 1:26 PM Sean Christopherson
> > > <sean.j.christopherson@intel.com> wrote:
> > > >
> > > > Running a checksum on the stack for every exit doesn't seem like it'd
> > > > be worth the effort, especially since this type of bug should be quite
> > > > rare, at least in production environments.
> > > >
> > > > If we want to pursue the checksum idea I think the easiest approach
> > > > would be to combine it with an exit_handler and do a simple check on
> > > > the handler.  It'd be minimal overhead in the fast path and would flag
> > > > cases where invoking exit_handle() would explode, while deferring all
> > > > other checks to the user.
> > >
> > > How about this variant?
> > >
> > > #define MAGIC 0xaaaabbbbccccddddul
> > > #define RETADDR_HASH ((unsigned long)__builtin_return_address(0) ^ MAGIC)
> > >
> > > void foo(void)
> > > {
> > >     volatile unsigned long hash = RETADDR_HASH;
> > >
> > >     /* placeholder for your actual code */
> > >     asm volatile ("nop");
> > >
> > >     if (hash != RETADDR_HASH)
> > >         asm volatile ("ud2");
> > > }
> > >
> > > But I have a real argument for dropping exit_handler: in this new age
> > > of Spectre, the indirect call is a retpoline, and it's therefore quite
> > > slow.
> >
> > Technically slower, but would the extra CALL+RET pair even be noticeable
> > in the grand scheme of SGX?
> 
> But it's CALL, CALL, MOV to overwrite return address, intentionally
> midpredicted RET, and RET because Spectre.  That whole sequence seems
> to be several tens of cycles, so it's a lot worse than just CALL+RET.
> Whether it's noticeable overall is a fair question, though.

I was thinking of the case where the handler re-entered the enclave vs.
leaving and re-calling the vDSO, which would be RET+CALL and some other
stuff.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Andy Lutomirski]

On Tue, Dec 11, 2018 at 2:00 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Dec 11, 2018 at 12:04:15PM -0800, Andy Lutomirski wrote:
> > On Tue, Dec 11, 2018 at 11:31 AM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >
> > > On Fri, Dec 07, 2018 at 03:33:57PM -0800, Andy Lutomirski wrote:
> > > > On Fri, Dec 7, 2018 at 1:26 PM Sean Christopherson
> > > > <sean.j.christopherson@intel.com> wrote:
> > > > >
> > > > > Running a checksum on the stack for every exit doesn't seem like it'd
> > > > > be worth the effort, especially since this type of bug should be quite
> > > > > rare, at least in production environments.
> > > > >
> > > > > If we want to pursue the checksum idea I think the easiest approach
> > > > > would be to combine it with an exit_handler and do a simple check on
> > > > > the handler.  It'd be minimal overhead in the fast path and would flag
> > > > > cases where invoking exit_handle() would explode, while deferring all
> > > > > other checks to the user.
> > > >
> > > > How about this variant?
> > > >
> > > > #define MAGIC 0xaaaabbbbccccddddul
> > > > #define RETADDR_HASH ((unsigned long)__builtin_return_address(0) ^ MAGIC)
> > > >
> > > > void foo(void)
> > > > {
> > > >     volatile unsigned long hash = RETADDR_HASH;
> > > >
> > > >     /* placeholder for your actual code */
> > > >     asm volatile ("nop");
> > > >
> > > >     if (hash != RETADDR_HASH)
> > > >         asm volatile ("ud2");
> > > > }
> > > >
> > > > But I have a real argument for dropping exit_handler: in this new age
> > > > of Spectre, the indirect call is a retpoline, and it's therefore quite
> > > > slow.
> > >
> > > Technically slower, but would the extra CALL+RET pair even be noticeable
> > > in the grand scheme of SGX?
> >
> > But it's CALL, CALL, MOV to overwrite return address, intentionally
> > midpredicted RET, and RET because Spectre.  That whole sequence seems
> > to be several tens of cycles, so it's a lot worse than just CALL+RET.
> > Whether it's noticeable overall is a fair question, though.
>
> I was thinking of the case where the handler re-entered the enclave vs.
> leaving and re-calling the vDSO, which would be RET+CALL and some other
> stuff.

Fair enough, although the case where we do an EENTER, an AEP, a kernel
entry, an IRET, and an ERESUME will be so slow that the CALL+RET seems
even less relevant.  The EENTER+EEXIT case at least avoids the round
trip through x86's amazingly performant exception handling mechanism
:)

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]

On 2018-12-07 03:49, Sean Christopherson wrote:

...

> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.c b/arch/x86/entry/vdso/vsgx_enter_enclave.c
> new file mode 100644
> index 000000000000..896c2eb079bb
> --- /dev/null
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.c

...

> +enter_enclave:
> +	asm volatile(
> +		/*
> +		 * When an event occurs in an enclave, hardware first exits the
> +		 * enclave to the AEP, switching CPU context along the way, and
> +		 * *then* delivers the event as usual.  As part of the context
> +		 * switching, registers are loaded with synthetic state (except
> +		 * BP and SP, which are saved/restored).  The defined synthetic
> +		 * state loads registers so that simply executing ENCLU will do
> +		 * ERESUME, e.g. RAX=4, RBX=TCS and RCX=AEP after an AEE.  So,
> +		 * we only need to load RAX, RBX and RCX for the initial entry.
> +		 * The AEP can point at that same ENCLU, fixup will jump us out
> +		 * if an exception was unhandled.
> +		 */
> +		"	lea	1f(%%rip), %%rcx\n"
> +		"1:	enclu\n"
> +		"2:\n"
> +
> +		".pushsection .fixup, \"ax\" \n"
> +		"3:	jmp 2b\n"
> +		".popsection\n"
> +		_ASM_VDSO_EXTABLE_HANDLE(1b, 3b)
> +
> +		: "=a"(leaf), "=D" (rdi), "=S" (rsi), "=d" (rdx)
> +		: "a" (leaf), "b" (tcs), "D" (priv)
> +		: "cc", "memory",
> +		  "rcx", "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
> +	);

This is not sufficient to support the Fortanix SGX ABI calling 
convention, which was designed to be mostly compatible with the SysV 
64-bit calling convention. The following registers need to be passed in 
to an enclave from userspace: RDI, RSI, RDX, R8, R9, R10. The following 
registers need to be passed out from an enclave to userspace: RDI, RSI, 
RDX, R8, R9.

You can find the ABI specification at 
https://github.com/fortanix/rust-sgx/blob/master/doc/FORTANIX-SGX-ABI.md#enclave-calling-convention

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Dave Hansen]

On 12/7/18 10:15 AM, Jethro Beekman wrote:
> This is not sufficient to support the Fortanix SGX ABI calling
> convention, which was designed to be mostly compatible with the SysV
> 64-bit calling convention. The following registers need to be passed in
> to an enclave from userspace: RDI, RSI, RDX, R8, R9, R10. The following
> registers need to be passed out from an enclave to userspace: RDI, RSI,
> RDX, R8, R9.

Are you asking nicely to change the new Linux ABI to be consistent with
your existing ABI?  Or, are you saying that the new ABI *must* be
compatible with this previous out-of-tree implementation?

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Andy Lutomirski]

> On Dec 7, 2018, at 10:44 AM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
>> On 12/7/18 10:15 AM, Jethro Beekman wrote:
>> This is not sufficient to support the Fortanix SGX ABI calling
>> convention, which was designed to be mostly compatible with the SysV
>> 64-bit calling convention. The following registers need to be passed in
>> to an enclave from userspace: RDI, RSI, RDX, R8, R9, R10. The following
>> registers need to be passed out from an enclave to userspace: RDI, RSI,
>> RDX, R8, R9.
> 
> Are you asking nicely to change the new Linux ABI to be consistent with
> your existing ABI?  Or, are you saying that the new ABI *must* be
> compatible with this previous out-of-tree implementation?

I think that allowing the enclave to return at least a few registers is quite reasonable, but I don’t have a strong opinion.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On 2018-12-08 00:14, Dave Hansen wrote:
> On 12/7/18 10:15 AM, Jethro Beekman wrote:
>> This is not sufficient to support the Fortanix SGX ABI calling
>> convention, which was designed to be mostly compatible with the SysV
>> 64-bit calling convention. The following registers need to be passed in
>> to an enclave from userspace: RDI, RSI, RDX, R8, R9, R10. The following
>> registers need to be passed out from an enclave to userspace: RDI, RSI,
>> RDX, R8, R9.
> 
> Are you asking nicely to change the new Linux ABI to be consistent with
> your existing ABI?  Or, are you saying that the new ABI *must* be
> compatible with this previous out-of-tree implementation?

What's being discussed here is one of the alternatives for SGX fault 
handling, meant to improve the current status quo of having to use a 
signal handler.

I'm merely providing a data point that the currently proposed solution 
is not sufficient to support current use of the (ring 3) ENCLU 
instruction. You might find this useful in determining whether proposed 
kernel features will actually be used by users, and in further 
developing this solution or other solutions to the fault handling issue.

If going with the vDSO solution, I think something with semantics closer 
to the actual instruction would be preferred, like the following:

notrace __attribute__((naked)) long __vdso_sgx_enclu_with_aep()
{
	asm volatile(
		"	lea	2f(%%rip), %%rcx\n"
		"1:	enclu\n"
		"2:     ret\n"
		".pushsection .fixup, \"ax\" \n"
		"3:	jmp 2b\n"
		".popsection\n"
		_ASM_VDSO_EXTABLE_HANDLE(1b, 3b)
		:::
	);
}

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Sean Christopherson]

On Sat, Dec 08, 2018 at 08:15:38AM +0000, Jethro Beekman wrote:
> On 2018-12-08 00:14, Dave Hansen wrote:
> >On 12/7/18 10:15 AM, Jethro Beekman wrote:
> >>This is not sufficient to support the Fortanix SGX ABI calling
> >>convention, which was designed to be mostly compatible with the SysV
> >>64-bit calling convention. The following registers need to be passed in
> >>to an enclave from userspace: RDI, RSI, RDX, R8, R9, R10. The following
> >>registers need to be passed out from an enclave to userspace: RDI, RSI,
> >>RDX, R8, R9.
> >
> >Are you asking nicely to change the new Linux ABI to be consistent with
> >your existing ABI?  Or, are you saying that the new ABI *must* be
> >compatible with this previous out-of-tree implementation?
> 
> What's being discussed here is one of the alternatives for SGX fault
> handling, meant to improve the current status quo of having to use a signal
> handler.
> 
> I'm merely providing a data point that the currently proposed solution is
> not sufficient to support current use of the (ring 3) ENCLU instruction. You
> might find this useful in determining whether proposed kernel features will
> actually be used by users, and in further developing this solution or other
> solutions to the fault handling issue.
> 
> If going with the vDSO solution, I think something with semantics closer to
> the actual instruction would be preferred, like the following:
> 
> notrace __attribute__((naked)) long __vdso_sgx_enclu_with_aep()
> {
> 	asm volatile(
> 		"	lea	2f(%%rip), %%rcx\n"
> 		"1:	enclu\n"
> 		"2:     ret\n"
> 		".pushsection .fixup, \"ax\" \n"
> 		"3:	jmp 2b\n"
> 		".popsection\n"
> 		_ASM_VDSO_EXTABLE_HANDLE(1b, 3b)
> 		:::
> 	);
> }

Part of me likes this idea, but it's a documentation nightmare since
it's a completely customer register ABI.  And the caller's exception
handling gets a bit weird since RAX implicitly defines whether or not
an exception occurred.  I also think there's value in making the vDSO
function callable from standard C.

Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On 2018-12-07 22:01, Dr. Greg wrote:
> Baidu and Fortanix are working on Trusted RunTime Systems (TRTS) based
> on RUST, I believe, so this will affect them to the extent that they
> are implementing their own low level enclave runtime support or they
> may be simply building on top of the low level Intel TRTS.  Perhaps
> Jethro would comment on these issues if he could.

As far as I know, Baidu merely provides Rust bindings to the Intel SDK. 
As far as our requirements, I've sent those in my previous email.

> I'm assuming that in the proposed model the URTS would interrogate the
> VDSO to determine the availability of entry and exception handling
> support and then setup the appropriate infrastructure and exit
> handler?  VDSO's are typically the domain of the system library.
> Given the nature of SGX I couldn't even conceive of Glibc offering
> support and, if it was acceptable to provide support, the potential
> timeframe that would be involved in seeing deployment in the field.
> 
> As a result, do you anticipate the need for a 'flag day' with respect
> to URTS/PSW/SDK support for all of this?

It is my understanding that the use of the vDSO enclave entry will be 
optional. i.e., if your application/library/enclave combination installs 
a signal handler and calls ENCLU directly, that would still work. Of 
course, using the vDSO will be very strongly recommended.

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]