[PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX enclave call

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

Please read the rest of the thread. Sean and I have hammered out some
sensible and effective changes.

Have skimmed through that discussion but it comes down how much you get
by obviously degrading some of the robustness. Complexity of the calling
pattern is not something that should be emphasized as that is something
that is anyway hidden inside the runtime.

/Jarkko

Nathaniel McCallum

2020-03-16 14:01:38 UTC

On Mon, Mar 16, 2020 at 9:56 AM Jarkko Sakkinen

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

Please read the rest of the thread. Sean and I have hammered out some
sensible and effective changes.

My suggestions explicitly maintained robustness, and in fact increased
it. If you think we've lost capability, please speak with specificity
rather than in vague generalities. Under my suggestions we can:
1. call the vDSO from C
2. pass context to the handler
3. have additional stack manipulation options in the handler

The cost for this is a net 2 additional instructions. No existing
capability is lost.

Jarkko Sakkinen

2020-03-16 21:38:24 UTC

Post by Nathaniel McCallum
On Mon, Mar 16, 2020 at 9:56 AM Jarkko Sakkinen

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

Please read the rest of the thread. Sean and I have hammered out some
sensible and effective changes.

My suggestions explicitly maintained robustness, and in fact increased
it. If you think we've lost capability, please speak with specificity
1. call the vDSO from C
2. pass context to the handler
3. have additional stack manipulation options in the handler
The cost for this is a net 2 additional instructions. No existing
capability is lost.

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER. And since this has been kind of agreed by most of the
stakeholders doing something against the chosen strategy is
something I do hold some resistance.

I get the idea technically what you are suggesting. Please
understand these are orthogonal axes that I have to care about.

In coummunity sense, it opens a possibility to unknown unknowns [1].

[1]

/Jarkko

Sean Christopherson

2020-03-16 22:53:22 UTC

Post by Nathaniel McCallum
On Mon, Mar 16, 2020 at 9:56 AM Jarkko Sakkinen

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

Please read the rest of the thread. Sean and I have hammered out some
sensible and effective changes.

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER.

Yes and no. If we wanted to minimize the amount of wrapping around the
vDSO's ENCLU then we wouldn't have the exit handler shenanigans in the
first place. The whole process has been about balancing the wants of each
use case against the overall quality of the API and code.

Post by Jarkko Sakkinen
And since this has been kind of agreed by most of the
stakeholders doing something against the chosen strategy is
something I do hold some resistance.

Up until Nathaniel joined the party, the only stakeholder in terms of the
exit handler was the Intel SDK. There was a general consensus to pass
registers as-is when there isn't a strong reason to do otherwise. Note
that Nathaniel has also expressed approval of that approach.

So I think the question that needs to be answered is whether the benefits
of using %rcx instead of %rax to pass @leaf justify the "pass registers
as-is" guideline. We've effectively already given this waiver for %rbx,
as the whole reason why the TCS is passed in on the stack instead of via
%rbx is so that it can be passed to the exit handler. E.g. the vDSO
could take the TCS in %rbx and save it on the stack, but we're throwing
the baby out with the bathwater at that point.

The major benefits being that the vDSO would be callable from C and that
the kernel could define a legitimate prototype instead of a frankenstein
prototype that's half assembly and half C. For me, those are significant
benefits and well worth the extra MOV, PUSH and POP. For some use cases
it would eliminate the need for an assembly wrapper. For runtimes that
need an assembly wrapper for whatever reason, it's probably still a win as
a well designed runtime can avoid register shuffling in the wrapper. And
if there is a runtime that isn't covered by the above, it's at worst an
extra MOV.

Xing, Cedric

2020-03-16 23:50:26 UTC

Post by Nathaniel McCallum
On Mon, Mar 16, 2020 at 9:56 AM Jarkko Sakkinen

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

Please read the rest of the thread. Sean and I have hammered out some
sensible and effective changes.

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER.

The design of this vDSO API was NOT to minimize wrapping, but to allow
maximal flexibility. More specifically, we strove not to restrict how
info was exchanged between the enclave and its host process. After all,
calling convention is compiler specific - i.e. the enclave could be
built by a different compiler (e.g. MSVC) that doesn't share the same
list of CSRs as the host process. Therefore, the API has been
implemented to pass through virtually all registers except those used by
EENTER itself. Similarly, all registers are passed back from enclave to
the caller (or the exit handler) except those used by EEXIT. %rbp is an
exception because the vDSO API has to anchor the stack, using either
%rsp or %rbp. We picked %rbp to allow the enclave to allocate space on
the stack.

Sean Christopherson

2020-03-16 23:59:34 UTC

Post by Nathaniel McCallum
My suggestions explicitly maintained robustness, and in fact increased
it. If you think we've lost capability, please speak with specificity
1. call the vDSO from C
2. pass context to the handler
3. have additional stack manipulation options in the handler
The cost for this is a net 2 additional instructions. No existing
capability is lost.

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER.

The design of this vDSO API was NOT to minimize wrapping, but to allow
maximal flexibility. More specifically, we strove not to restrict how info
was exchanged between the enclave and its host process. After all, calling
convention is compiler specific - i.e. the enclave could be built by a
different compiler (e.g. MSVC) that doesn't share the same list of CSRs as
the host process. Therefore, the API has been implemented to pass through
virtually all registers except those used by EENTER itself. Similarly, all
registers are passed back from enclave to the caller (or the exit handler)
except those used by EEXIT. %rbp is an exception because the vDSO API has to
anchor the stack, using either %rsp or %rbp. We picked %rbp to allow the
enclave to allocate space on the stack.

And unless I'm missing something, using %rcx to pass @leaf would still
satisfy the above, correct? Ditto for saving/restoring %rbx.

I.e. a runtime that's designed to work with enclave's using a different
calling convention wouldn't be able to take advantage of being able to call
the vDSO from C, but neither would it take on any meaningful burden.

Xing, Cedric

2020-03-17 00:18:14 UTC

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER.

The design of this vDSO API was NOT to minimize wrapping, but to allow
maximal flexibility. More specifically, we strove not to restrict how info
was exchanged between the enclave and its host process. After all, calling
convention is compiler specific - i.e. the enclave could be built by a
different compiler (e.g. MSVC) that doesn't share the same list of CSRs as
the host process. Therefore, the API has been implemented to pass through
virtually all registers except those used by EENTER itself. Similarly, all
registers are passed back from enclave to the caller (or the exit handler)
except those used by EEXIT. %rbp is an exception because the vDSO API has to
anchor the stack, using either %rsp or %rbp. We picked %rbp to allow the
enclave to allocate space on the stack.

satisfy the above, correct? Ditto for saving/restoring %rbx.
I.e. a runtime that's designed to work with enclave's using a different
calling convention wouldn't be able to take advantage of being able to call
the vDSO from C, but neither would it take on any meaningful burden.

Not exactly.

If called directly from C code, the caller would expect CSRs to be
preserved. Then who should preserve CSRs? It can't be the enclave
because it may not follow the same calling convention. Moreover, the
enclave may run into an exception, in which case it doesn't have the
ability to restore CSRs. So it has to be done by the vDSO API. That
means CSRs will be overwritten upon enclave exits, which violates the
goal of "passing all registers back to the caller except those used by
EEXIT".

Sean Christopherson

2020-03-17 00:27:06 UTC

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER.

The design of this vDSO API was NOT to minimize wrapping, but to allow
maximal flexibility. More specifically, we strove not to restrict how info
was exchanged between the enclave and its host process. After all, calling
convention is compiler specific - i.e. the enclave could be built by a
different compiler (e.g. MSVC) that doesn't share the same list of CSRs as
the host process. Therefore, the API has been implemented to pass through
virtually all registers except those used by EENTER itself. Similarly, all
registers are passed back from enclave to the caller (or the exit handler)
except those used by EEXIT. %rbp is an exception because the vDSO API has to
anchor the stack, using either %rsp or %rbp. We picked %rbp to allow the
enclave to allocate space on the stack.

Not exactly.
If called directly from C code, the caller would expect CSRs to be
preserved. Then who should preserve CSRs? It can't be the enclave because it
may not follow the same calling convention. Moreover, the enclave may run
into an exception, in which case it doesn't have the ability to restore
CSRs. So it has to be done by the vDSO API. That means CSRs will be
overwritten upon enclave exits, which violates the goal of "passing all
registers back to the caller except those used by EEXIT".

IIUC, Nathaniel's use case is to run only enclaves that are compatible
with Linux's calling convention and to handle enclave exceptions in the
exit handler.

As I qualified above, there would certainly be runtimes and use cases that
would find no advantage in passing @leaf via %rcx and preserving %rbx. I'm
well aware the Intel SDK falls into that bucket. But again, the cost to
such runtimes is precisely one reg->reg MOV instruction.

Nathaniel McCallum

2020-03-17 16:37:54 UTC

On Mon, Mar 16, 2020 at 8:27 PM Sean Christopherson

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER.

The design of this vDSO API was NOT to minimize wrapping, but to allow
maximal flexibility. More specifically, we strove not to restrict how info
was exchanged between the enclave and its host process. After all, calling
convention is compiler specific - i.e. the enclave could be built by a
different compiler (e.g. MSVC) that doesn't share the same list of CSRs as
the host process. Therefore, the API has been implemented to pass through
virtually all registers except those used by EENTER itself. Similarly, all
registers are passed back from enclave to the caller (or the exit handler)
except those used by EEXIT. %rbp is an exception because the vDSO API has to
anchor the stack, using either %rsp or %rbp. We picked %rbp to allow the
enclave to allocate space on the stack.

Not exactly.
If called directly from C code, the caller would expect CSRs to be
preserved. Then who should preserve CSRs? It can't be the enclave because it
may not follow the same calling convention. Moreover, the enclave may run
into an exception, in which case it doesn't have the ability to restore
CSRs. So it has to be done by the vDSO API. That means CSRs will be
overwritten upon enclave exits, which violates the goal of "passing all
registers back to the caller except those used by EEXIT".

IIUC, Nathaniel's use case is to run only enclaves that are compatible
with Linux's calling convention and to handle enclave exceptions in the
exit handler.
As I qualified above, there would certainly be runtimes and use cases that
well aware the Intel SDK falls into that bucket. But again, the cost to
such runtimes is precisely one reg->reg MOV instruction.

It seems to me that some think my proposal represents a shift in
strategic direction. I do not see it that way. I affirm the existing
strategic direction. My proposal only represents a specific
optimization of that strategic direction that benefits certain use
cases without significant cost to all other use cases.

Nathaniel McCallum

2020-03-17 16:50:09 UTC

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER.

The design of this vDSO API was NOT to minimize wrapping, but to allow
maximal flexibility. More specifically, we strove not to restrict how info
was exchanged between the enclave and its host process. After all, calling
convention is compiler specific - i.e. the enclave could be built by a
different compiler (e.g. MSVC) that doesn't share the same list of CSRs as
the host process. Therefore, the API has been implemented to pass through
virtually all registers except those used by EENTER itself. Similarly, all
registers are passed back from enclave to the caller (or the exit handler)
except those used by EEXIT. %rbp is an exception because the vDSO API has to
anchor the stack, using either %rsp or %rbp. We picked %rbp to allow the
enclave to allocate space on the stack.

Not exactly.
If called directly from C code, the caller would expect CSRs to be
preserved.

Correct. This requires collaboration between the caller of the vDSO
and the enclave.

Post by Xing, Cedric
Then who should preserve CSRs?

The enclave.

Post by Xing, Cedric
It can't be the enclave
because it may not follow the same calling convention.

This is incorrect. You are presuming there is not tight integration
between the caller of the vDSO and the enclave. In my case, the
integration is total and complete. We have working code today that
does this.

Post by Xing, Cedric
Moreover, the
enclave may run into an exception, in which case it doesn't have the
ability to restore CSRs.

There are two solutions to this:
1. Write the handler in assembly and don't return to C on AEX.
2. The caller can simply preserve the registers. Nothing stops that.

We have implemented #1.

Post by Xing, Cedric
So it has to be done by the vDSO API.

Nope. See above.

Post by Xing, Cedric
That
means CSRs will be overwritten upon enclave exits, which violates the
goal of "passing all registers back to the caller except those used by
EEXIT".

All registers get passed to the handler in this scenario, not the caller.

The approach is as follows: the vDSO is callable by C so long as the
enclave respects the ABI *OR* the handler patches up any enclave
deviation from the ABI.

Xing, Cedric

2020-03-17 21:40:34 UTC

Hi Nathaniel,

I reread your email today and thought I might have misunderstood your
email earlier. What changes are you asking for exactly? Is that just
passing @leaf in %ecx rather than in %eax? If so, I wouldn't have any
problem. I agree with you that the resulted API would then be callable
from C, even though it wouldn't be able to return back to C due to
tampered %rbx. But I think the vDSO API can preserve %rbx too, given it
is used by both EENTER and EEXIT (so is unavailable for parameter
passing anyway). Alternatively, the C caller can setjmp() to be
longjmp()'d back from within the exit handler.

-Cedric

Sean Christopherson

2020-03-17 22:09:48 UTC

Post by Xing, Cedric
Hi Nathaniel,
I reread your email today and thought I might have misunderstood your email
in %ecx rather than in %eax? If so, I wouldn't have any problem. I agree
with you that the resulted API would then be callable from C, even though it
wouldn't be able to return back to C due to tampered %rbx. But I think the
vDSO API can preserve %rbx too, given it is used by both EENTER and EEXIT
(so is unavailable for parameter passing anyway). Alternatively, the C
caller can setjmp() to be longjmp()'d back from within the exit handler.

Yep, exactly. The other proposed change that is fairly straightforward is
to make the save/restore of %rsp across the exit handler call relative
instead of absolute, i.e. allow the exit handler to modify %rsp. I don't
think this would conflict with the Intel SDK usage model?

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

Xing, Cedric

2020-03-17 22:36:57 UTC

Yep, exactly. The other proposed change that is fairly straightforward is
to make the save/restore of %rsp across the exit handler call relative
instead of absolute, i.e. allow the exit handler to modify %rsp. I don't
think this would conflict with the Intel SDK usage model?
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

Yep. Though it looks a bit uncommon, I do think it will work.

Sean Christopherson

2020-03-17 23:57:11 UTC

Yep, exactly. The other proposed change that is fairly straightforward is
to make the save/restore of %rsp across the exit handler call relative
instead of absolute, i.e. allow the exit handler to modify %rsp. I don't
think this would conflict with the Intel SDK usage model?
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

Yep. Though it looks a bit uncommon, I do think it will work.

Heh, I had about the same level of confidence.

I'll put together a set of patches tomorrow and post them to linux-sgx (and
cc relevant parties). It'll be easier to continue the discussion with code
to look at and we can stop spamming LKML for a bit :-)

Xing, Cedric

2020-03-17 22:23:31 UTC

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER.

The design of this vDSO API was NOT to minimize wrapping, but to allow
maximal flexibility. More specifically, we strove not to restrict how info
was exchanged between the enclave and its host process. After all, calling
convention is compiler specific - i.e. the enclave could be built by a
different compiler (e.g. MSVC) that doesn't share the same list of CSRs as
the host process. Therefore, the API has been implemented to pass through
virtually all registers except those used by EENTER itself. Similarly, all
registers are passed back from enclave to the caller (or the exit handler)
except those used by EEXIT. %rbp is an exception because the vDSO API has to
anchor the stack, using either %rsp or %rbp. We picked %rbp to allow the
enclave to allocate space on the stack.

Not exactly.
If called directly from C code, the caller would expect CSRs to be
preserved.

Correct. This requires collaboration between the caller of the vDSO
and the enclave.

Post by Xing, Cedric
Then who should preserve CSRs?

The enclave.

Post by Xing, Cedric
It can't be the enclave
because it may not follow the same calling convention.

Post by Xing, Cedric
Moreover, the
enclave may run into an exception, in which case it doesn't have the
ability to restore CSRs.

1. Write the handler in assembly and don't return to C on AEX.
2. The caller can simply preserve the registers. Nothing stops that.
We have implemented #1.

What if the enclave cannot proceed due to an unhandled exception so the
execution has to get back to the C caller of the vDSO API?

It seems to me the caller has to preserve CSRs by itself, otherwise it
cannot continue execution after any enclave exception. Passing @leaf in
%ecx will allow saving/restoring CSRs in C by setjmp()/longjmp(), with
the help of an exit handler. But if the C caller has already preserved
CSRs, why preserve CSRs again inside the enclave? It looks to me things
can be simplified only if the host process handles no enclave exceptions
(or exceptions inside the enclave will crash the calling thread). Thus
the only case of enclave EEXIT'ing back to its caller is considered
valid, hence the enclave will always be able to restore CSRs, so that
neither vDSO nor its caller has to preserve CSRs.

Is my understanding correct?

Nathaniel McCallum

2020-03-17 16:28:58 UTC

On Mon, Mar 16, 2020 at 6:53 PM Sean Christopherson

Post by Nathaniel McCallum
On Mon, Mar 16, 2020 at 9:56 AM Jarkko Sakkinen

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

Please read the rest of the thread. Sean and I have hammered out some
sensible and effective changes.

My vague generality in this case is just that the whole design
approach so far has been to minimize the amount of wrapping to
EENTER.

Post by Jarkko Sakkinen
And since this has been kind of agreed by most of the
stakeholders doing something against the chosen strategy is
something I do hold some resistance.

Up until Nathaniel joined the party, the only stakeholder in terms of the
exit handler was the Intel SDK.

I would hope that having additional stakeholders would ease the path
to adoption.

Post by Sean Christopherson
There was a general consensus to pass
registers as-is when there isn't a strong reason to do otherwise. Note
that Nathaniel has also expressed approval of that approach.

I still approve that approach.

Post by Sean Christopherson
So I think the question that needs to be answered is whether the benefits
as-is" guideline. We've effectively already given this waiver for %rbx,
as the whole reason why the TCS is passed in on the stack instead of via
%rbx is so that it can be passed to the exit handler. E.g. the vDSO
could take the TCS in %rbx and save it on the stack, but we're throwing
the baby out with the bathwater at that point.
The major benefits being that the vDSO would be callable from C and that
the kernel could define a legitimate prototype instead of a frankenstein
prototype that's half assembly and half C. For me, those are significant
benefits and well worth the extra MOV, PUSH and POP. For some use cases
it would eliminate the need for an assembly wrapper. For runtimes that
need an assembly wrapper for whatever reason, it's probably still a win as
a well designed runtime can avoid register shuffling in the wrapper. And
if there is a runtime that isn't covered by the above, it's at worst an
extra MOV.

Nathaniel McCallum

2020-03-16 13:57:28 UTC

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

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.

Post by Jethro Beekman
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.

When isn't the vDSO available? Once the patches are merged it will
always be available. Then we also get to live with this interface
forever. I'd rather have a good, usable interface for the long term.

Post by Jethro Beekman
I agree with Jarkko that everything should be kept small and simple. Calling a couple extra instructions is going to have a negligible effect compared to the actual time EENTER/EEXIT take.

We all agree on small and simple. Nothing I've proposed fails either
of those criteria.

Post by Jethro Beekman
Can someone remind me why we're not passing TCS in RBX but on the stack?

If you do that, the vDSO will never be callable from C. And, as you've
stated above, calling a couple extra instructions is going to have a
negligible effect.

Jethro Beekman

2020-03-16 13:59:28 UTC

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

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.

Post by Jethro Beekman
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.

When isn't the vDSO available?

When you're not on Linux. Or when you're on an old kernel.

--
Jethro Beekman | Fortanix

Nathaniel McCallum

2020-03-16 14:03:31 UTC

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

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.

Post by Jethro Beekman
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.

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.

Sean Christopherson

2020-03-16 17:17:20 UTC

On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen

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.

Post by Jethro Beekman
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.

Post by Nathaniel McCallum
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.

Jarkko Sakkinen

2020-03-16 21:27:03 UTC

Post by Nathaniel McCallum
For the vDSO, only marginally. I'm counting +4,-2 instructions in my
suggestions. For the wrapper, things become significantly simpler.

Simpler is not a quality that has very high importance here except
when it comes to vDSO.

At least it is not enough to change to vDSO. What else?

Anyway, I think the documentation should fixed and streamlined 1st.

It is way too verbose prose in some places and in some it completely
lacks the information e.g.

"Debug Exceptions (#DB) and Breakpoints (#BP) are ever fixed up and are
always delivered via standard signals."

Never should state things like that without explaining the reasons.

On the other hand:

"Most exceptions reported on ENCLU, including those that occur within
the enclave, are fixed up and reported synchronously instead of being
delivered via a standard signal. Debug Exceptions (#DB) and Breakpoints
(#BP) are never fixed up and are always delivered via standard signals.
On synchrously reported exceptions, -EFAULT is returned and details
about the exception are recorded in @e, the optional
sgx_enclave_exception struct."

Duplicates information already elsewhere (e.g. return values) and is
just pain to read and comprehend in general.

/Jarkko

Jarkko Sakkinen

2020-03-16 21:29:51 UTC

Post by Nathaniel McCallum
For the vDSO, only marginally. I'm counting +4,-2 instructions in my
suggestions. For the wrapper, things become significantly simpler.

Simpler is not a quality that has very high importance here except
when it comes to vDSO.
At least it is not enough to change to vDSO. What else?

~~
the

In any case, where I stand is that the vDSO implementation itself is
exactly how it should be. The process to get it to this form was
tedious. Now we have a form that the known userbase can live with.

The documentation sucks, agreed. I think by fixing that this would
be a wholelot better.

/Jarkko

Sean Christopherson

2020-03-16 22:55:34 UTC

Post by Jethro Beekman
Can someone remind me why we're not passing TCS in RBX but on the stack?

I finally remembered why. It's pulled off the stack and passed into the
exit handler. I'm pretty sure the vDSO could take it in %rbx and manually
save it on the stack, but I'd rather keep the current behavior so that the
vDSO is callable from C (assuming @leaf is changed to be passed via %rcx).

Xing, Cedric

2020-03-16 23:56:42 UTC

Post by Jethro Beekman
Can someone remind me why we're not passing TCS in RBX but on the stack?

The idea is that the caller of this vDSO API is C callable, hence it
cannot receive TCS in %rbx anyway. Then it has to either MOV to %rbx or
PUSH to stack. Either way the complexity is the same. The vDSO API
however has to always save it on stack for exit handler. So receiving it
via stack ends up in simplest code.

Dr. Greg

2020-03-17 01:07:18 UTC

On Tue, Mar 10, 2020 at 02:29:41PM -0500, Haitao Huang wrote:

Good evening, I hope the week is going well for everyone.

Just as a clarification, are you testing the new driver against
signed production class enclaves in .so format that also include
metadata layout directives or is the driver just getting tested
against the two page toy enclave that copies a word of memory from
one memory location to another?

We (Intel SGX SDK/PSW team) tested this driver for enclaves in .so
format with metadata. Our 2.8 release supports v24 and 2.9 supports
v25+. Both production signed and debug signed enclaves worked.
*Note* we did make some code changes in our runtime for v24+, mainly
dealing with src & EPC page alignment for EADD, open one fd per
enclave, use -z noexecstack linker option, etc. You can see the
changes on GitHub.

Lots of knobs getting turned at the same time but we sorted out all
the issues and our runtime is now passing its regression tests with
the new driver, with an exception that we note below.

I suspect that we might have the only complete and architecturally
independent runtime implementation so if the new driver is working
against yours and ours it would seem to be a reasonable test spectrum
for the driver.

We see the same behavior from both our unit test enclaves and the
Quoting Enclave from the Intel SGX runtime.

We did not see any issue loading QE in our tests. Please directly
email me on this test if you have specific questions.

As it turns out the major problem we were running into with respect to
the QE test was the fact that generic use of atexit() handlers was
disabled by changes that went into the 2.8 SDK. Our runtime and SDK
assume that enclave atexit() handling works.

The enclave UNINIT ECALL is only allowed on runtimes that are
advertising EDMM support. That seems excessively restrictive since
atexit() handling is generically useful for enclaves that are not
using EDMM. Our runtime allows EDMM to be disabled and we have
enclaves that gate on that for security purposes.

On a quasi-related note, it appears that the 1.4 compatibility
metadata created by post 2.0 signing tools is leaking layout
descriptors that a version 1.4 runtime doesn't understand.

Do you want to exchange e-mail on this or should we direct
conversations about these issues with others on your SDK team.

Have a good remainder of the week.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker SGX secured infrastructure and
Enjellic Systems Development, LLC autonomously self-defensive
4206 N. 19th Ave. platforms.
Fargo, ND 58102
PH: 701-281-1686 EMAIL: ***@enjellic.com

Jordan Hand

2020-03-17 16:00:15 UTC