by Jethro Beekman

[permalink] [raw]

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

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

Attachments:

smime.p7s (3.90 kB)
S/MIME Cryptographic Signature

2018-12-07 18:21:58

On Fri, Dec 7, 2018 at 1:26 PM Sean Christopherson
<[email protected]> wrote:
>
> On Fri, Dec 07, 2018 at 12:16:59PM -0800, Andy Lutomirski wrote:
> >
> >
> > > On Dec 7, 2018, at 12:09 PM, Sean Christopherson <[email protected]> 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? :)

2018-12-08 08:17:01

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.