Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for x86_64-fortanix-unknown-sgx target #738

Closed
wants to merge 5 commits into from

Conversation

akash-fortanix
Copy link

The x86_64-fortanix-unknown-sgx recently gained Tier 3 support upstream.

@briansmith
Copy link
Owner

briansmith commented Dec 12, 2018

@akash-fortanix Thanks.

I guess it is hopeless to expect to be able to test this in CI. However, I do have a SGX-enabled laptop myself (IIUC). Please supply a script that allows me to run the ring test suite in SGX that people can run manually.

@jethrogb
Copy link

jethrogb commented Dec 12, 2018

@briansmith Unfortunately, Rust's libtest crate is not yet supported on x86_64-fortanix-unknown-sgx. We are working on making it happen, but it'll take a few weeks (unwinding is one of the biggest dependencies).

@briansmith
Copy link
Owner

OK, thanks. I think that we can handle this like Android, where for some reason we had trouble getting the tests to run. Let's change the CI to add one new target that will do everything except run the tests, and then leave the lines that run the tests commented out.

@jethrogb
Copy link

jethrogb commented Dec 13, 2018

Right now, this is a tier 3 target, meaning no binaries for std are distributed. It will need to be built from source. Are you ok with installing xargo in CI to do the build?

@briansmith
Copy link
Owner

Right now, this is a tier 3 target, meaning no binaries for std are distributed. It will need to be built from source. Are you ok with installing xargo in CI to do the build?

We need some script that can be run by somebody who wants to test this code. If it requires Xargo then please include the installation of Xargo and its build dependencies in the script. I suggest just adding this https://github.com/rust-embedded/cross since cross already uses xargo for some plaforms.

@briansmith
Copy link
Owner

I am working on moving all of the non-i686/non-x86_64 builds to use cross on Travis CI, so I think adding support for this target to cross is the right thing to do. Then we would do cross test --no-run --target=x86_64-fortanix-unknown-sgx and then run the test executables locally.

@briansmith
Copy link
Owner

In the documentation for the ring::rand module, please describe how this is done (using RDRAND).

ring normally uses CPUID to detect CPU features. How is that supposed to work on this target? In particular, how does ring know whether or not it is OK to use AES-NI, AVX2, etc.

@jethrogb
Copy link

ring normally uses CPUID to detect CPU features. How is that supposed to work on this target? In particular, how does ring know whether or not it is OK to use AES-NI, AVX2, etc.

I don't know. The lack of feature detection is tracked in fortanix/rust-sgx#26. So far I've always assumed AES-NI will be available on SGX because EREPORT uses AES-CMAC, however I think you can disable AES-NI in BIOS and that will actually disable the instruction. You can call XGETBV(0) in the enclave and use those results, but that doesn't give you everything. Intel's runtime calls CPUID on the host, but I'm not too keen on that.

@briansmith
Copy link
Owner

Can we assume that at least SSSE3 is available? If so then the difference between a trustworthy CPUID and an untrustworthy one would be primarily performance + the possibility of crashing on an illegal instruction. If we can't even rely on SSSE3 always being available then that is more problematic.

In particular, in the AES-GCM code, there is logic to fall back from AES-NI to other implementations if AES-NI isn't detected. the "VPAES" implementation is (probably, mostly) free of side channels but it requires SSSE3. The "fallback" implementation isn't free of side-channels at all. So, it seems like on SGX the best course of action would be to do whatever SGX gives us for an untrustworthy CPUID, but then hard-code the "SSSE3 enabled" bit so that the VPAES implementation would always be used, and also completely disable via a #[cfg(target_arch)] the "fallback" implementation of AES.

Similarly, in gcm.rs, we probably want to assume in SGX that the hardware polynomial multiplication instructions are always available, again via #[cfg(target_arch)] .

We also need to document these assumptions somewhere and note the consequence, namely the possibility of getting an illegal instruction exception.

On Twitter, it was suggested that we could detect certain CPU instruction extensions via sigsetjmp and siglongjmp if they work in SGX. Do they?

@jethrogb
Copy link

jethrogb commented Dec 17, 2018

Can we assume that at least SSSE3 is available?

I don't think this may be assumed from the standpoint of the documentation, but all processors released so far with SGX have it.

On Twitter, it was suggested that we could detect certain CPU instruction extensions via sigsetjmp and siglongjmp if they work in SGX. Do they?

No.

Something that's been brewing in the back of my mind is to have an enclave that just tries to execute various instructions with an exception handler. If it succeeds, record that it's available. If you get #UD, the exception handler can be entered to skip over the instruction and record that it's not available, after which the enclave can be re-entered. This can be done securely. This could be run once on system boot and the status could be reported to other enclaves using local attestation.

@jethrogb
Copy link

I'm totally fine with requiring and documenting the existence of various features and relying on those in a hard-coded manner.

@jethrogb
Copy link

jethrogb commented Dec 24, 2018

It's not clear to me what rings story is w.r.t. std support, because of this:

// src/lib.rs
#![no_std]
...
extern crate std;

It looks like you briefly tried to be no_std in ae75675, but this was later broken in a281f58. Since then a fair amount of std-dependent code was added. I'm going to assume std is fair game.

In that case, I'm going to suggest GFp_cpuid_setup in cpu-intel.c is replaced by Rust code that fills in GFp_ia32cap_P based on the is_x86_feature_detected macro. This way, ring will automatically benefit from whatever CPUID-detection mechanism might or might not be added in the future to this or other SGX targets. The rdrand crate also uses this standard feature detection mechanism.

Somewhat relatedly, I think this is also how #357 should be implemented once is_aarch64_feature_detected/is_arm_feature_detected is stable.

@akash-fortanix akash-fortanix force-pushed the sgx-target branch 3 times, most recently from 80206fc to 2065c83 Compare December 24, 2018 10:49
@akash-fortanix
Copy link
Author

AppVeyor build failures are not caused by this PR.

@briansmith
Copy link
Owner

It looks like you briefly tried to be no_std in ae75675, but this was later broken in a281f58. Since then a fair amount of std-dependent code was added. I'm going to assume std is fair game.

There is no extern crate std in ring master. Definitely we try to support working without libstd in the --no-default-features configuration, but I've not tested lately (especially after the Rust 2018 Edition changes) whether it works.

In that case, I'm going to suggest GFp_cpuid_setup in cpu-intel.c is replaced by Rust code that fills in GFp_ia32cap_P based on the is_x86_feature_detected macro. This way, ring will automatically benefit from whatever CPUID-detection mechanism might or might not be added in the future to this or other SGX targets. The rdrand crate also uses this standard feature detection mechanism.

I would accept a PR to do that. (The main difficulty is figuring out which bits even need to be checked.)

@jethrogb
Copy link

jethrogb commented Dec 27, 2018

There is no extern crate std in ring master.

The current cfg_attr(..., no_std) almost never gets activated. You need to be Redox or UNIX but not Linux or Mac/IOS. Building for FreeBSD/Redox targets with --no-default-features is very broken right now.

error[E0433]: failed to resolve: use of undeclared type or module `std`
  --> src/ec/curve25519/ops.rs:22:5
   |
22 | use std::marker::PhantomData;
   |     ^^^ use of undeclared type or module `std`

error[E0432]: unresolved import `std`
  --> src/cpu.rs:19:13
   |
19 |         use std;
   |             ^^^ no `std` external crate

error[E0658]: imports can only refer to extern crate names passed with `--extern` on stable channel (see issue #53130)
   --> src/rand.rs:208:21
    |
191 |     use std;
    |         --- not an extern crate passed with `--extern`
...
208 |                 use std::io::Read;
    |                     ^^^
    |
    = help: add #![feature(uniform_paths)] to the crate attributes to enable
note: this import refers to the unresolved item imported here
   --> src/rand.rs:191:9
    |
191 |     use std;
    |         ^^^

error[E0432]: unresolved import `std`
   --> src/rand.rs:191:9
    |
191 |     use std;
    |         ^^^ no `std` external crate

I would accept a PR to do that.

Ok. Do you want to land that with this or separately?

@briansmith
Copy link
Owner

src/ec/curve25519/ops.rs:22:5 | 22 | use std::marker::PhantomData;

Fixed in master, along with a couple of other use std instances.

src/cpu.rs:19:13 | 19 | use std;

This still remains; we depend on std::sync::Once, which isn't available in libcore. Probably we could work around this.

src/rand.rs:208:21 | 191 | use std; | --- not an extern crate passed with --extern

That's in the dev/urandom code. Currently, every target needs to submit a PR to ring to tell it how to accesss the PRNG, basically.

@briansmith
Copy link
Owner

Ok. Do you want to land that with this or separately?

It should be done in a separate PR. I think it is a non-trivial amount of work because the person making the PR needs to communicate clearly how to interpret what the OpenSSL/BoringSSL (assembly) language code is doing, so the reviewer can efficiently verify the correctness of the changes. That means, basically, documenting every access of GFp_ia32cap_P.

@briansmith
Copy link
Owner

is_x86_feature_detected

According to rust-lang/stdarch#464 (comment), we can't use is_x86_feature_detected; instead we need to use whatever lower-level interface that libcore provides.

@jethrogb
Copy link

Just FYI, the error output in #738 (comment) was just an excerpt. There were many more errors, I didn't want to put all the lines in my comment. You can trivially discover this by running cargo build --target x86_64-unknown-freebsd --no-default-features or cargo build --target x86_64-unknown-redox --no-default-features.

According to rust-lang/stdarch#464, we can't use is_x86_feature_detected; instead we need to use whatever lower-level interface that libcore provides.

Well, yes, in my discussion I think I was pretty clear that std support was required for this. I thought this was ok because ring doesn't actually work without std. There is no "lower level interface" in libcore, feature detection requires std on several platforms.

@briansmith
Copy link
Owner

Just FYI, the error output in #738 (comment) was just an excerpt. There were many more errors, I didn't want to put all the lines in my comment. You can trivially discover this by running cargo build --target x86_64-unknown-freebsd --no-default-features or cargo build --target x86_64-unknown-redox --no-default-features.

Thanks. I've done this and reduced (in my private tree) the instances of problems to the need for std::sync::Once, which is probably generically solvable, and for a rand implementation, which needs more thought. However, generally it seems like we should be able to work on many no_std systems without much effort. I want to keep working towards that goal.

Well, yes, in my discussion I think I was pretty clear that std support was required for this. I thought this was ok because ring doesn't actually work without std. There is no "lower level interface" in libcore, feature detection requires std on several platforms.

I need to think more about this. In general I would like to use the Rust built-in feature detection instead of implementing it in ring, but there are several issues to consider, of which no_std support is only one.

@jethrogb
Copy link

std::sync::Once

You might want to consider https://crates.io/crates/spin

@briansmith
Copy link
Owner

I fixed most, but not all, of the no_std related build breakage, in 7ad3bb7, abe7b2d, and b486965.

The issue of how to make ring working in no_std configurations is now #744.

A proposal for changing how we do feature detection should be in a separate issue.

I'll email @jethrogb next week about how make progress on this.

@jethrogb
Copy link

NB I've documented how to build & run for the target at fortanix/rust-sgx#49. As you can see, it's kind of unwieldy right now, but I expect things to get better quickly.

@akash-fortanix
Copy link
Author

@briansmith Any update on how to take this ahead?

@jethrogb
Copy link

I've completed all the tasks mentioned in #775 for x86_64-fortanix-unknown-sgx.

This still needs rust-lang/rust#57914 to land in the compiler to work with LTO. Alternatively, I can disable LTO for the SGX target in CI for now to make the tests pass.

Travis is configured to allow failures of nightly. I don't know how to configure Travis to not allow failures of SGX even though nightly is used.

@jethrogb jethrogb force-pushed the sgx-target branch 2 times, most recently from ef7853f to 98b51c2 Compare January 30, 2019 13:14
@jethrogb
Copy link

jethrogb commented Jan 30, 2019

NB. The commit message of d15e9d2 has an audit of all the CPU features currently used by ring. Some of them are pretty strange.

@jethrogb
Copy link

Filed a PR for MOVBE: rust-lang/rust#57999

@jethrogb jethrogb force-pushed the sgx-target branch 2 times, most recently from 78eb140 to 4cf77cc Compare February 1, 2019 06:11
@jethrogb
Copy link

jethrogb commented Feb 1, 2019

Well, looks like LTO is now failing on every platform. But on the plus side, it should work for SGX on the latest nightly (if you disable incremental compilation).

@jethrogb
Copy link

jethrogb commented Feb 4, 2019

Fixed merge conflicts (i.e. duplicate work) with ab0726d, 03e6ef8, 32cf372, 1194b80, bd96baa.

@jethrogb
Copy link

jethrogb commented Feb 4, 2019

@briansmith this PR has been open for 2 months. What is blocking merging this?

src/rand.rs Outdated

#[must_use]
#[repr(transparent)]
pub struct OneMeansSuccess(c::int);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use bssl::Result instead.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks

src/rand.rs Outdated

if !rest.is_empty() {
let mut buf = [0; 8];
Result::from(CRYPTO_rdrand(&mut buf))?;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to do everything withCRYPTO_rdrand and avoid CRYPTO_rdrand_multiple8_buf. Then we can implement the retry 10 times advice from Intel in this Rust code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Jethro Beekman and others added 5 commits February 7, 2019 10:26
I agree to license my contributions to each file under the terms given
at the top of each file I changed.

Co-authored-by: Akash Anand <akash.anand@fortanix.com>
I agree to license my contributions to each file under the terms given
at the top of each file I changed.
Audit of all CPUID features use by *ring* at the time of the commit:

|              | chacha | aesni | mont5 | mont | ghash | sha512 | poly1305 | Rust |
|--------------|--------|-------|-------|------|-------|--------|----------|------|
| ADX          |        |       | *     | *    |       |        |          |      |
| AES          |        |       |       |      |       |        |          | *    |
| AVX          |        | *     |       |      |       | *      | *        | *    |
| AVX2         | *      |       |       | *    |       | *      | *        |      |
| AVX512F      | *      |       |       |      |       |        |          |      |
| BMI1         |        |       | *     |      |       | *      |          |      |
| BMI2         |        |       | *     | *    |       | *      |          |      |
| FXSR         |        |       |       |      |       |        |          | 5    |
| “intel CPU”  |        |       |       |      |       | *      |          |      |
| MOVBE        | 1,2    | *     |       |      | 1,3   |        |          | 6    |
| PCLMULQDQ    |        |       |       |      |       |        |          | *    |
| SHA          |        |       |       |      |       | *      |          |      |
| SSSE3        | *      |       |       |      |       | *      |          | *    |
| XOP          |        | 4     |       |      |       |        |          |      |
| XSAVE        | 1      | 1     |       |      | 1     |        |          |      |

1. Instruction not used, only used to detect Atom processors
2. If Atom, change the input lengths for which different code paths are taken (presumably for performance)
3. If Atom, avoid one code path for performance
4. Instruction not used
5. Instruction not used, only used to detect PCLMULQDQ
6. Instruction not used, only used to detect AVX

I agree to license my contributions to each file under the terms given
at the top of each file I changed.
I agree to license my contributions to each file under the terms given
at the top of each file I changed.
I agree to license my contributions to each file under the terms given
at the top of each file I changed.
@jethrogb
Copy link

Ping...

@jethrogb
Copy link

jethrogb commented Mar 5, 2019

@briansmith do you have any other comments on this PR?

@kostko
Copy link

kostko commented Mar 13, 2019

I've been successfully using this branch for the past few weeks, it would be great if it could be merged barring any other concerns?

@briansmith
Copy link
Owner

I think this is substantially easier now. See #744 (comment) and the review I did for UEFI support in #1406.

@briansmith
Copy link
Owner

Closing due to inactivity.

@briansmith briansmith closed this Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants