[for_v29,v2,0/5] x86/sgx: Make vDSO callable from C

Message

Sean Christopherson March 30, 2020, 6:08 p.m. UTC

Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
close to being callable from C (with caveats and a cooperative enclave).
The missing pieces are preserving %rbx and taking @leaf as a standard
parameter.

v2:
  - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
  - Add CFI directive for RBX. [Cedric]

v1:
  - https://patchwork.kernel.org/cover/11446355/

Sean Christopherson (5):
  x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code
  x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave
  selftests/sgx: Pass EENTER to vDSO wrapper instead of hardcoding
  selftests/sgx: Stop clobbering non-volatile registers
  selftests/sgx: Add selftest to invoke __vsgx_enter_enclave() from C

 arch/x86/entry/vdso/vsgx_enter_enclave.S      | 64 ++-----------------
 arch/x86/include/uapi/asm/sgx.h               | 61 ++++++++++++++++++
 tools/testing/selftests/sgx/call.S            |  1 -
 tools/testing/selftests/sgx/defines.h         |  1 +
 tools/testing/selftests/sgx/main.c            | 20 ++++--
 tools/testing/selftests/sgx/main.h            |  2 +-
 .../selftests/sgx/test_encl_bootstrap.S       |  6 +-
 7 files changed, 84 insertions(+), 71 deletions(-)

Comments

Jarkko Sakkinen March 30, 2020, 8:48 p.m. UTC | #1

On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> close to being callable from C (with caveats and a cooperative enclave).
> The missing pieces are preserving %rbx and taking @leaf as a standard
> parameter.
> 
> v2:
>   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
>   - Add CFI directive for RBX. [Cedric]

I'm sorry for throwing stick's constantly but I think having a real
ELF loader is for better.

/Jarkko

Nathaniel McCallum March 30, 2020, 9:42 p.m. UTC | #2

On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
<[e-mail]> wrote:
>
> On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > close to being callable from C (with caveats and a cooperative enclave).
> > The missing pieces are preserving %rbx and taking @leaf as a standard
> > parameter.
> >
> > v2:
> >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> >   - Add CFI directive for RBX. [Cedric]
>
> I'm sorry for throwing stick's constantly but I think having a real
> ELF loader is for better.

Why is it one or the other?

Jarkko Sakkinen March 31, 2020, 11:58 a.m. UTC | #3

On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> <[e-mail]> wrote:
> >
> > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > close to being callable from C (with caveats and a cooperative enclave).
> > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > parameter.
> > >
> > > v2:
> > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > >   - Add CFI directive for RBX. [Cedric]
> >
> > I'm sorry for throwing stick's constantly but I think having a real
> > ELF loader is for better.
> 
> Why is it one or the other?

Hmm... Not sure I understand what you want to know.

A raw binary requires a hardcoded way to interpret it. Obviously ELF
is for better.

/Jarkko

Nathaniel McCallum March 31, 2020, 1:40 p.m. UTC | #4

On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
<[e-mail]> wrote:
>
> On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > <[e-mail]> wrote:
> > >
> > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > parameter.
> > > >
> > > > v2:
> > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > >   - Add CFI directive for RBX. [Cedric]
> > >
> > > I'm sorry for throwing stick's constantly but I think having a real
> > > ELF loader is for better.

This statement seems like you are juxtaposing having
__vdso_sgx_enter_enclave() be potentially C-compatible with having an
ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
be C-callable *and* you can have an ELF loader.

> > Why is it one or the other?
>
> Hmm... Not sure I understand what you want to know.
>
> A raw binary requires a hardcoded way to interpret it. Obviously ELF
> is for better.

We (Enarx) implement an ELF loader.

Jarkko Sakkinen April 1, 2020, 8:17 a.m. UTC | #5

On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> <[e-mail]> wrote:
> >
> > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > <[e-mail]> wrote:
> > > >
> > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > parameter.
> > > > >
> > > > > v2:
> > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > >   - Add CFI directive for RBX. [Cedric]
> > > >
> > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > ELF loader is for better.
> 
> This statement seems like you are juxtaposing having
> __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> be C-callable *and* you can have an ELF loader.

I'm not honestly sure what this is about but my comment was about heavy
rebasing of the GIT tree as I rewrote the selftest last week.

/Jarkko

Nathaniel McCallum April 1, 2020, 1:06 p.m. UTC | #6

On Wed, Apr 1, 2020 at 4:18 AM Jarkko Sakkinen
<[e-mail]> wrote:
>
> On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> > On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> > <[e-mail]> wrote:
> > >
> > > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > > <[e-mail]> wrote:
> > > > >
> > > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > > parameter.
> > > > > >
> > > > > > v2:
> > > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > > >   - Add CFI directive for RBX. [Cedric]
> > > > >
> > > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > > ELF loader is for better.
> >
> > This statement seems like you are juxtaposing having
> > __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> > ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> > be C-callable *and* you can have an ELF loader.
>
> I'm not honestly sure what this is about but my comment was about heavy
> rebasing of the GIT tree as I rewrote the selftest last week.

Okay. Let's chalk it up to miscommunication then. :)

Sean Christopherson April 1, 2020, 2:49 p.m. UTC | #7

On Wed, Apr 01, 2020 at 09:06:38AM -0400, Nathaniel McCallum wrote:
> On Wed, Apr 1, 2020 at 4:18 AM Jarkko Sakkinen
> <[e-mail]> wrote:
> >
> > On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> > > On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> > > <[e-mail]> wrote:
> > > >
> > > > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > > > parameter.
> > > > > > >
> > > > > > > v2:
> > > > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > > > >   - Add CFI directive for RBX. [Cedric]
> > > > > >
> > > > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > > > ELF loader is for better.
> > >
> > > This statement seems like you are juxtaposing having
> > > __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> > > ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> > > be C-callable *and* you can have an ELF loader.
> >
> > I'm not honestly sure what this is about but my comment was about heavy
> > rebasing of the GIT tree as I rewrote the selftest last week.
> 
> Okay. Let's chalk it up to miscommunication then. :)

Ha, I was in the same boat as Nathaniel.  We thought the "having a real
ELF loader comment" was a comment on the patch itself, i.e. that you
disagreed with it in some way because it didn't support an ELF loader,
hence our confusion.

Now I realize you were refering to the rebase needed due to rewriting the
selftest to use an ELF loader.  Crisis aborted :-)

Jarkko Sakkinen April 2, 2020, 7:49 p.m. UTC | #8

On Wed, Apr 01, 2020 at 09:06:38AM -0400, Nathaniel McCallum wrote:
> On Wed, Apr 1, 2020 at 4:18 AM Jarkko Sakkinen
> <[e-mail]> wrote:
> >
> > On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> > > On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> > > <[e-mail]> wrote:
> > > >
> > > > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > > > parameter.
> > > > > > >
> > > > > > > v2:
> > > > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > > > >   - Add CFI directive for RBX. [Cedric]
> > > > > >
> > > > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > > > ELF loader is for better.
> > >
> > > This statement seems like you are juxtaposing having
> > > __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> > > ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> > > be C-callable *and* you can have an ELF loader.
> >
> > I'm not honestly sure what this is about but my comment was about heavy
> > rebasing of the GIT tree as I rewrote the selftest last week.
> 
> Okay. Let's chalk it up to miscommunication then. :)

Yeah, lets bring a background :-)

Last week basically rewrote the self-test. It now directly loads ELF and
dynamically signs that. Thus, the codebase has been kind of moving
target for a while. I'm absolutely for these changes as soon as the
form fits to the existing codebase.

/Jarkko

Jarkko Sakkinen April 2, 2020, 8:01 p.m. UTC | #9

On Wed, Apr 01, 2020 at 07:49:38AM -0700, Sean Christopherson wrote:
> On Wed, Apr 01, 2020 at 09:06:38AM -0400, Nathaniel McCallum wrote:
> > On Wed, Apr 1, 2020 at 4:18 AM Jarkko Sakkinen
> > <[e-mail]> wrote:
> > >
> > > On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> > > > On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> > > > <[e-mail]> wrote:
> > > > >
> > > > > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > > > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > > > > <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > > > > parameter.
> > > > > > > >
> > > > > > > > v2:
> > > > > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > > > > >   - Add CFI directive for RBX. [Cedric]
> > > > > > >
> > > > > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > > > > ELF loader is for better.
> > > >
> > > > This statement seems like you are juxtaposing having
> > > > __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> > > > ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> > > > be C-callable *and* you can have an ELF loader.
> > >
> > > I'm not honestly sure what this is about but my comment was about heavy
> > > rebasing of the GIT tree as I rewrote the selftest last week.
> > 
> > Okay. Let's chalk it up to miscommunication then. :)
> 
> Ha, I was in the same boat as Nathaniel.  We thought the "having a real
> ELF loader comment" was a comment on the patch itself, i.e. that you
> disagreed with it in some way because it didn't support an ELF loader,
> hence our confusion.
> 
> Now I realize you were refering to the rebase needed due to rewriting the
> selftest to use an ELF loader.  Crisis aborted :-)

Awesome :-)

/Jarkko