Expose EGETKEY/EREPORT #15

jethrogb · 2018-11-20T06:11:39Z

These could be in std::arch or std::os::sgx.

The text was updated successfully, but these errors were encountered:

jethrogb · 2018-12-14T06:41:24Z

@gnzlbg @alexcrichton I'd like your input on the design for additions to core::arch or std::os. I don't think an RFC is required because the end result is either some obvious platform-specified additions to core::arch or changes to an unstable platform-specific module inside std::os which historically have not required RFCs. If you think an RFC would be better I'd be glad to turn this writeup into one.

The Intel SGX instruction set extension adds a bunch of instructions. See Intel SDM, Volume 3, Chapter 40. There are 3 new opcodes for instructions ENCLU, ENCLS and ENCLV. All these instructions choose a "leaf instruction" based value of EAX. These leaf instructions exhibit wildly different behavior from each other. It is therefore generally more useful to think of these leaf instructions as individual instructions.In documentation, these leaf instructions are generally referred to as instr[leaf], for example ENCLU[EREPORT], or sometimes just EREPORT.

ENCLS and ENCLV can only be used in ring 0, so I'm going to ignore these for this discussion. ENCLU has the following leafs, split into two categories:

ENCLU leafs that may be called from non-SGX applications:

EENTER
ERESUME

There might also be platform-dependent ways to call these leafs.

Leafs that may be called from SGX applications:

EREPORT
EGETKEY
EEXIT
EACCEPT
EMODPE
EACCEPTCOPY

I want to expose at least the EGETKEY and EREPORT leaf instructions for use by crate authors in the x86_64-fortanix-unknown-sgx target.

There are no LLVM intrinsics for these instructions.

Option 1. Add `enclu` function in `coresimd::x86_64`

/// Returns ZF
fn enclu(eax: &mut u32, rbx: &mut u64, rcx: &mut u64, rdx: &mut u64) -> bool;

Most flexible, but since each leaf is so very different, this is pretty weird. Leafs may have completely different register/flags usage. Also, some instructions (i.e. EENTER, ERESUME, EEXIT) have such weird effects (with regards to execution flow and possible exceptions) that they don't really make sense as an intrinsic.

Edit: If this option is chosen, users will always want to use a wrapper function around this (like Option 3).

Option 2. Add limited `enclu_LEAF` functions in `coresimd::x86_64`

For example:

#[repr(align(512))]
pub struct Keyrequest([u8; 512]);
#[repr(align(16))]
pub struct Key([u8; 16]);

pub unsafe fn enclu_egetkey(keyrequest: *const Keyrequest, key: *mut Key) -> u32;

#[repr(align(512))]
pub struct Targetinfo([u8; 512]);
#[repr(align(128))]
pub struct Reportdata([u8; 64]);
#[repr(align(512))]
pub struct Report([u8; 432]);

pub unsafe fn enclu_ereport(targetinfo: *const Targetinfo, reportdata: *const Reportdata, report: *mut Report);

These functions would be implemented in inline assembly.

It's not clear to me whether it makes sense to expose these at this level. The two instructions I described here can only be called from SGX applications and will trigger #GP if called from non-SGX applications. It does not make sense to use runtime detection for these features (see “CPUID considerations” below). It doesn't fit well with the way the rest of core::arch is documented.

I haven't added EEXIT/EACCEPT/EMODPE/EACCEPTCOPY since most crate authors would not call those functions, they will only be used by std internals or no_std users.

Option 3. Add functions in `std::os::sgx`

For example:

extern crate sgx_isa;

pub use sgx_isa::{Keyrequest, Key, Targetinfo, Reportdata, Report, ErrorCode};

pub fn egetkey(keyrequest: &Keyrequest, key: &mut Key) -> ErrorCode;
pub fn ereport(targetinfo: &Targetinfo, reportdata: &Reportdata) -> Report;

These functions would be implemented in inline assembly.

Since std may have dependencies, I've added the sgx-isa crate here which has the full field definitions of the structures used here.

Once again, I haven't added EEXIT/EACCEPT/EMODPE/EACCEPTCOPY since crate authors would not call those functions, they will only be used by std internals.

CPUID considerations

Support for the ENCLU instruction could be detected using CPUID.

Non-SGX applications will need to interact with the OS before the ENCLU instruction can be meaningfully executed. These OS APIs will return an error if SGX is not supported. Therefore, it should never be necessary for a non-SGX application to check support using CPUID.

SGX applications know--by virtue of running--that SGX is supported. Therefore, it should never be necessary for an SGX application to check support using CPUID.

gnzlbg · 2018-12-14T10:48:00Z

I don't think an RFC is required

I think the RFC for std::arch (x86/x86_64) would apply here, so a mini-FCP is enough. Either way, I wouldn't worry about this now. There is no need for any of this to add an unstable implementation to stdsimd - once that is finish, writing a small mini-FCP is not that much work.

A couple of things.

Automatic verification: all x86 and x86_64 intrinsics are automatically-verified (type signature and actual emitted machine code) against the Intel spec. The spec signatures for encl{u,v,s} differ widely from the intrinsics proposed here. Deviating from the spec would require rationale, in the form of an RFC. It would be much easier if the intrinsics exposed would stick to the spec.

Testing: all stable x86 and x86_64 intrinsics have run-time tests that run IIRC. Testing ring 0 is hard, how easy / hard would it be to add tests for these using the SGX target to stdsimd Travis-CI ?

I also have a question. Can x86/x86_64 binaries that are not compiled for SGX contain code that uses encl{u,v,s} in the same binary ? E.g. is it possible to write a binary like ~this:

fn main() {
    if in_sgx_enclave() {
        sgx_main()
    } else {
        non_sgx_main()
    }
}

? And for example, compile it against x86_64-unknown-linux-gnu, such that it can run both inside and outside an sgx enclave ?

If the answer is yes, then we are going to need #[target_feature] attributes and run-time feature detection for these intrinsics.

jethrogb · 2018-12-14T11:11:14Z

@gnzlbg Thanks for your input.

Automatic verification: all x86 and x86_64 intrinsics are automatically-verified (type signature and actual emitted machine code) against the Intel spec. The spec signatures for encl{u,v,s} differ widely from the intrinsics proposed here. Deviating from the spec would require rationale, in the form of an RFC. It would be much easier if the intrinsics exposed would stick to the spec.

I didn't realize Intel specified an intrinsic here. After looking at it, I have several concerns:

It requires passing an pointer to an array (*mut [size_t; 3]) for things that are passed as registers at the instruction level. I know an optimizing compiler can probably fix this but it still seems weird, I'm much more comfortable with three individual pointers being passed.
LLVM doesn't implement the intrinsics this way (or at all, really) (Edit: GCC does: https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/sgxintrin.h )
It's not clear to me how ZF is being returned from this intrinsic.
See comments regarding "Option 1" for having a non-leaf intrinsic.
Even Intel itself doesn't use this intrinsic in their software: https://github.com/intel/linux-sgx/search?q=_enclu_u32&unscoped_q=_enclu_u32

Testing: all stable x86 and x86_64 intrinsics have run-time tests that run IIRC. Testing ring 0 is hard, how easy / hard would it be to add tests for these using the SGX target to stdsimd Travis-CI ?

I don't think Travis supports VMs with Intel SGX enabled.

I also have a question. Can x86/x86_64 binaries that are not compiled for SGX contain code that uses encl{u,v,s} in the same binary ? E.g. is it possible to write a binary like ~this:
fn main() {
    if in_sgx_enclave() {
        sgx_main()
    } else {
        non_sgx_main()
    }
}
? And for example, compile it against x86_64-unknown-linux-gnu, such that it can run both inside and outside an sgx enclave ?

If the answer is yes, then we are going to need #[target_feature] attributes and run-time feature detection for these intrinsics.

I'm not sure which of two different questions you're asking here. I've copied your first question here and rephrased your second question below.

Question 1: "Can x86/x86_64 binaries that are not compiled for SGX contain code that uses encl{u,v,s} in the same binary ?"

Yes, non-SGX applications can call SGX instructions. Ring0 code is by definition not an SGX application and can call all ENCLS and ENCLV leafs. As mentioned above, ring3 user code can call ENCLU[EENTER] and ENCLU[ERESUME]. See my comment above regarding why runtime feature detection doesn't make sense: you always know from other context whether calling SGX instructions is going to succeed or not.

You would never do this:

if is_x86_feature_detected!("sgx") {
	enclu(...)
}

You will always do something like this:

if let Ok(dev) = File::open("/dev/sgx") {
	enclu(...)
}

Or more likely yet (Linux example):

if let Ok(dev) = File::open("/dev/sgx") {
	__vdso_sgx_enter_enclave(...)
}

Question 2: Can the same application binary be run in both SGX and non-SGX mode?

No. You have to compile to different targets and would use conditional compilation to distinguish. For example:

fn main() {
    #[cfg(target_env="sgx")] {
        sgx_main()
    }
    #[cfg(not(target_env="sgx"))]{
        non_sgx_main()
    }
}

gnzlbg · 2018-12-14T11:20:47Z

Yes, non-SGX applications can call SGX instructions.

I see, so I think we need a #[target_feature(...)] for these instructions. Do you know which features would be needed? (just sgx? or what do clang and gcc or intel use?). I wonder how the lack of use of is_feature_detected would interact with a future "portability" check, but this is something that we can clarify as we go.

See my comment above regarding why runtime feature detection doesn't make sense: you always know from other context whether calling SGX instructions is going to succeed or not.

This makes sense.

Thank you for the answers. So I think you can send a PR to stdsimd with the proposed implementation, and we can discuss them there in more detail. The "automatic verification" part is not a hard requirement, we can also do "manual verification", but since so to speak you are "inventing" a new API for these intrinsics deviating for the spec, once things work and are ready for stabilization we might need to have a small RFC explaining why things are the way they are just like you have done here :)

jethrogb · 2018-12-14T12:18:19Z

Do you know which features would be needed? (just sgx? or what do clang and gcc or intel use?).

LLVM has an sgx feature that covers all of ENCLU/ENCLS/ENCLV. I don't think this is appropriate since the set of processors that support ENCLV is a subset of the set of processors that support ENCLS/ENCLU. Also, this might be an argument for leaf-specific intrinsics (Option 2) above, because different leaves have different hardware support. This could be indicated by different target features.

gnzlbg · 2018-12-14T12:21:47Z

We can add multiple target features for this - the mapping from Rust to LLVM target features is customizable. In particular, we don't have to map a Rust target feature to any LLVM target feature if LLVM does not have one yet, but it might be worth it to open an LLVM issue in their bugzilla to ask about what their plans here are, and maybe for feedback about what should we do.

In the mean time, the initial implementation would be unstable and feature gated, so we can feature gate the features as well, and revise this topic before stabilization once we have more experience, and maybe some feedback from LLVM upstream.

jethrogb · 2018-12-14T12:42:12Z

In #15 (comment) I was proposing three different options, not 3 steps that would all be implemented. Sorry if this wasn't clear initially, I've edited it for clarity.

alexcrichton · 2018-12-14T16:47:44Z

I don't have much to add over @gnzlbg except to echo that if we can stick to the spec that'd be great because that's what we're trying to do for all other platforms and intrinsics!

jethrogb · 2018-12-14T18:08:43Z

Ok, if you both feel strongly about adding the documented intrinsic regardless of the usability disadvantages that has we can go for it.

gnzlbg · 2018-12-14T18:52:19Z

@jethrogb we have exceptionally minimally changed the API of some intrinsics in the past that had a buggy API and the bug was acknowledge by Intel - these changes have been very minimal, e.g., an unsigned integer where the spec said a signed integer had to be used, etc.

If you feel strongly about the Intel APIs being inadequate for your purposes, you might want to report the "bug" here: https://software.intel.com/en-us/forums/intel-isa-extensions/topic/363747

@alexcrichton point is that the rule is that we only expose the vendor APIs "as they designed", therefore, we are not designing anything, and adding stuff here doesn't really need an RFC, which speeds up the process a lot. So if exposing the Intel intrinsics in core::arch allows you to write a nice crate in crates.io that provides a better API, my recommendation is to do that - the core::arch intrinsics might not be nice, but you can just use this wrapper instead.

You have hinted that they might not really allow you to do that. And arguably, these are only three intrinsics that we can verify manually, and also arguably, your project is going to be its main user, so if they don't enable you to do what you need to do, then what's the point? If Intel acknowledges this in the bug report page, or if any other C compiler also deviates from the spec, then I'd say you'd have a pretty good case to deviate from the spec as well.

This only matters when thinking about stabilization though, as long as this is unstable, we can tune the API of these intrinsics as they get used.

jethrogb · 2018-12-17T06:42:48Z

I think I'll just go with option 3 for now while we coalesce on a design for intrinsics.

I really don't think the 3 Intel-defined intrinsics are appropriate, because the leafs are so different and should really be considered different instructions. No other instructions that have defined intrinsics have this issue.

Different leafs do completely different things
Different leafs have completely different operands
Different leafs may have different hardware support (in terms of CPU features)

Features

CPUID.(EAX=12H,ECX=0):EAX.SGX1 [bit 0]
CPUID.(EAX=12H,ECX=0):EAX.SGX2 [bit 1]
CPUID.(EAX=12H,ECX=0):EAX.OVERSUB_V [bit 5]
CPUID.(EAX=12H,ECX=0):EAX.OVERSUB_S [bit 6]

Main instructions

Instr	Feature
ENCLS	SGX1
ENCLU	SGX1
ENCLV	OVERSUB_V

Leaf instructions

Note regarding the register usage: pointer alignment requirements differ a lot based on the actual leaf instruction.

Instr	Leaf	Feature	CPU mode	EAX out?	RBX	RCX	RDX
ENCLS	ECREATE	SGX1	ring0		PAGEINFO (In)	EPCPAGE (In)
ENCLS	EADD	SGX1	ring0		PAGEINFO (In)	EPCPAGE (In)
ENCLS	EINIT	SGX1	ring0	✔️	SIGSTRUCT (In)	SECS (In)	EINITTOKEN (In)
ENCLS	EREMOVE	SGX1	ring0			EPCPAGE (In)
ENCLS	EDBGRD	SGX1	ring0		Result Data (Out)	EPCPAGE (In)
ENCLS	EDBGWR	SGX1	ring0		Source Data (In)	EPCPAGE (In)
ENCLS	EEXTEND	SGX1	ring0		SECS (In)	EPCPAGE (In)
ENCLS	ELDB	SGX1	ring0	✔️	PAGEINFO (In)	EPCPAGE (In)	VERSION (In)
ENCLS	ELDU	SGX1	ring0	✔️	PAGEINFO (In)	EPCPAGE (In)	VERSION (In)
ENCLS	EBLOCK	SGX1	ring0	✔️		EPCPAGE (In)
ENCLS	EPA	SGX1	ring0		PT_VA (In)	EPCPAGE (In)
ENCLS	EWB	SGX1	ring0	✔️	PAGEINFO (In)	EPCPAGE (In)	VERSION (In)
ENCLS	ETRACK	SGX1	ring0	✔️		EPCPAGE (In)
ENCLS	EAUG	SGX2	ring0		PAGEINFO (In)	EPCPAGE (In)
ENCLS	EMODPR	SGX2	ring0	✔️	SECINFO (In)	EPCPAGE (In)
ENCLS	EMODT	SGX2	ring0	✔️	SECINFO (In)	EPCPAGE (In)
ENCLS	ERDINFO	OVERSUB_S	ring0		RDINFO (In)	EPCPAGE (In)
ENCLS	ETRACKC	OVERSUB_S	ring0	✔️		EPCPAGE (In)
ENCLS	ELDBC	OVERSUB_S	ring0	✔️	PAGEINFO (In)	EPCPAGE (In)	VERSION (In)
ENCLS	ELDUC	OVERSUB_S	ring0	✔️	PAGEINFO (In)	EPCPAGE (In)	VERSION (In)
ENCLU	EREPORT	SGX1	ring3 (enclave)		TARGETINFO (In)	REPORTDATA (In)	OUTPUTDATA (In)
ENCLU	EGETKEY	SGX1	ring3 (enclave)		KEYREQUEST (In)	KEY (In)
ENCLU	EENTER	SGX1	ring3 (user)	✔️	TCS (In)	AEP (In)/Ret (Out)
ENCLU	ERESUME	SGX1	ring3 (user)		TCS (In)	AEP (In)
ENCLU	EEXIT	SGX1	ring3 (enclave)		Target (In)	Current AEP (Out)
ENCLU	EACCEPT	SGX2	ring3 (enclave)	✔️	SECINFO (In)	EPCPAGE (In)
ENCLU	EMODPE	SGX2	ring3 (enclave)		SECINFO (In)	EPCPAGE (In)
ENCLU	EACCEPTCOPY	SGX2	ring3 (enclave)	✔️	SECINFO (In)	EPCPAGE (In)	EPCPAGE (In)
ENCLV	EDECVIRTCHILD	OVERSUB_V	VMX root		EPCPAGE (In)	SECS (In)
ENCLV	EINCVIRTCHILD	OVERSUB_V	VMX root		EPCPAGE (In)	SECS (In)
ENCLV	ESETCONTEXT	OVERSUB_V	VMX root			EPCPAGE (In)	Context Value (In)

gnzlbg · 2018-12-17T22:28:15Z

FWIW I think it should be possible for stable Rust code to use these intrinsics someday without having to use inline asm!. An API that enables this does not have to be nice, it only has to make it possible to create a nice API for these in a third-party stable crate.

Add `io` and `arch` modules to `std::os::fortanix_sgx` This PR adds two more (unstable) modules to `std::os::fortanix_sgx` for the `x86_64-fortanix-unknown-sgx` target. ### io `io` allows conversion between raw file descriptors and Rust types, similar to `std::os::unix::io`. ### arch `arch` exposes the `ENCLU[EREPORT]` and `ENCLU[EGETKEY]` instructions. The current functions are very likely not going to be the final form of these functions (see also fortanix/rust-sgx#15), but this should be sufficient to enable experimentation in libraries. I tried using the actual types (from the [`sgx-isa` crate](https://crates.io/crates/sgx-isa)) instead of byte arrays, but that would make `std` dependent on the `bitflags` crate which I didn't want to do at this time.

jethrogb added enhancement std/rustc Requires changes in Rust std/rustc labels Nov 20, 2018

jethrogb added this to the PROD-1267 milestone Dec 11, 2018

jethrogb added the needs-design label Dec 14, 2018

This was referenced Dec 25, 2018

Add io and arch modules to std::os::fortanix_sgx rust-lang/rust#57119

Merged

Tracking issue: platform-specific APIs for SGX target rust-lang/rust#56975

Open

jethrogb removed this from the PROD-1267 milestone Jan 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose EGETKEY/EREPORT #15

Expose EGETKEY/EREPORT #15

jethrogb commented Nov 20, 2018

jethrogb commented Dec 14, 2018 •

edited

Loading

gnzlbg commented Dec 14, 2018 •

edited

Loading

jethrogb commented Dec 14, 2018 •

edited

Loading

gnzlbg commented Dec 14, 2018 •

edited

Loading

jethrogb commented Dec 14, 2018 •

edited

Loading

gnzlbg commented Dec 14, 2018

jethrogb commented Dec 14, 2018

alexcrichton commented Dec 14, 2018

jethrogb commented Dec 14, 2018

gnzlbg commented Dec 14, 2018

jethrogb commented Dec 17, 2018

gnzlbg commented Dec 17, 2018 •

edited

Loading

Expose EGETKEY/EREPORT #15

Expose EGETKEY/EREPORT #15

Comments

jethrogb commented Nov 20, 2018

jethrogb commented Dec 14, 2018 • edited Loading

Option 1. Add enclu function in coresimd::x86_64

Option 2. Add limited enclu_LEAF functions in coresimd::x86_64

Option 3. Add functions in std::os::sgx

CPUID considerations

gnzlbg commented Dec 14, 2018 • edited Loading

jethrogb commented Dec 14, 2018 • edited Loading

Question 1: "Can x86/x86_64 binaries that are not compiled for SGX contain code that uses encl{u,v,s} in the same binary ?"

Question 2: Can the same application binary be run in both SGX and non-SGX mode?

gnzlbg commented Dec 14, 2018 • edited Loading

jethrogb commented Dec 14, 2018 • edited Loading

gnzlbg commented Dec 14, 2018

jethrogb commented Dec 14, 2018

alexcrichton commented Dec 14, 2018

jethrogb commented Dec 14, 2018

gnzlbg commented Dec 14, 2018

jethrogb commented Dec 17, 2018

Features

Main instructions

Leaf instructions

gnzlbg commented Dec 17, 2018 • edited Loading

jethrogb commented Dec 14, 2018 •

edited

Loading

Option 1. Add `enclu` function in `coresimd::x86_64`

Option 2. Add limited `enclu_LEAF` functions in `coresimd::x86_64`

Option 3. Add functions in `std::os::sgx`

gnzlbg commented Dec 14, 2018 •

edited

Loading

jethrogb commented Dec 14, 2018 •

edited

Loading

gnzlbg commented Dec 14, 2018 •

edited

Loading

jethrogb commented Dec 14, 2018 •

edited

Loading

gnzlbg commented Dec 17, 2018 •

edited

Loading