Support x86_64-unknown-uefi #30

josephlr · 2019-06-12T12:03:25Z

Closes #27 and adds support for the x86_64-unknown-uefi target. For our purposes, this target is quite similar to SGX in that the only source of RNG is RDRAND.

This PR:

Uses cargo xbuild to make sure we can build on this target.
Adds in feature detection using is_x86_feature_detected! macro. We use it from std_detect instead of std. Note that while this macro is not part of core, std_detect supports no_std targets on x86 (as it just makes cached calls to CPUID).

Note: we don't use EFI_RNG_PROTOCOL as most motherboard firmwares don't implement it, and those that do (i.e. EDK2) just call RDRAND anyway.

josephlr · 2019-06-13T10:09:19Z

Note that because this removed the compiler error when cfg(not(target_feature = "rdrand")) all the rdrand methods are now unsafe (as it is UD behavior to call them if rdrand is not supported). We also need to keep these functions unsafe as they need the #[target_feature(enable = "rdrand")] annotation to enable rdrand to be inlined (which is critical for performance in this case). See the difference:

No annotations, and therefore no inlining and terrible output
With annotations and cpuid-based detection
With annotations and target-feature=+rdrnd, CPUID checks are properly elided

Also, @dhardy @newpavlov to use std_detect (outside of std) requires nightly. Is this acceptable? If not I can just copy the rdrand detection code from there (it's only 5 lines).

dhardy · 2019-06-13T10:18:55Z

I'm still not convinced whether we should omit the CPU detection just to avoid a CPUID test. @nagisa?

I think there's no other reason we can't support SGX with stable compilers? If so then copying the code sounds best.

josephlr · 2019-06-13T10:43:27Z

I'm still not convinced whether we should omit the CPU detection just to avoid a CPUID test.

To be clear, the CPUID test is only omitted if the compiler already knows it can support RDRAND. This is crucial for SGX, as the CPUID instruction is illegal in an enclave.

If so then copying the code sounds best.

Will do

nagisa · 2019-06-13T12:00:20Z

You should just use the `is_x86_target_feature_detected` macro. It does the sane thing - caches results (globally) and does the detection in a target specific way which is more likely to work compared to whatever you might want to maintain yourselves.

…

On Thu, Jun 13, 2019, 13:43 Joseph Richey ***@***.***> wrote: I'm still not convinced whether we should omit the CPU detection just to avoid a CPUID test. To be clear, the CPUID test is only omitted if the compiler already knows it can support RDRAND. This is crucial for SGX, as the CPUID instruction is illegal in an enclave. If so then copying the code sounds best. Will do — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30?email_source=notifications&email_token=AAFFZURRWBWUAVIMAKGZYKLP2IQFBA5CNFSM4HXIH3TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXTJIWQ#issuecomment-501650522>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFFZUVR3GWDRZKEZXKMIH3P2IQFBANCNFSM4HXIH3TA> .

nagisa · 2019-06-13T12:02:58Z

Caveat: the macro is not supported on non-std targets. In that case you still want to maintain something similar.

…

On Thu, Jun 13, 2019, 14:59 Simonas Kazlauskas ***@***.***> wrote: You should just use the `is_x86_target_feature_detected` macro. It does the sane thing - caches results (globally) and does the detection in a target specific way which is more likely to work compared to whatever you might want to maintain yourselves. On Thu, Jun 13, 2019, 13:43 Joseph Richey ***@***.***> wrote: > I'm still not convinced whether we should omit the CPU detection just to > avoid a CPUID test. > > To be clear, the CPUID test is only omitted if the compiler already knows > it can support RDRAND. This is crucial for SGX, as the CPUID instruction is > illegal in an enclave. > > If so then copying the code sounds best. > > Will do > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#30?email_source=notifications&email_token=AAFFZURRWBWUAVIMAKGZYKLP2IQFBA5CNFSM4HXIH3TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXTJIWQ#issuecomment-501650522>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAFFZUVR3GWDRZKEZXKMIH3P2IQFBANCNFSM4HXIH3TA> > . >

josephlr · 2019-06-13T20:15:13Z

Caveat: the macro is not supported on non-std targets. In that case you
still want to maintain something similar.

Ya this is the issue, because most of the targets we would want to use RDRAND on are no_std (like UEFI, or SGX targets that aren't x86_64-fortanix-unknown-sgx). I added a comment linking to the stdsimd issue.

josephlr · 2019-06-13T20:34:32Z

@dhardy @nagisa does the is_rdrand_supported() look correct?

nagisa · 2019-06-13T21:04:20Z

https://github.com/nagisa/rust_rdrand/blob/fa7b3b13f393a1eaf345fb8d207631ca2a8a78bc/src/lib.rs#L143 is what I have in the rdrand library, your’s seems equivalent.

dhardy

Looks okay to me other than inverted logic

src/rdrand.rs

newpavlov

Is there a way to detect if EFI_RNG_PROTOCOL is implemented and use RDRAND as a fallback?

src/rdrand.rs

josephlr · 2019-06-14T10:23:51Z

Is there a way to detect if EFI_RNG_PROTOCOL is implemented and use RDRAND as a fallback?

Not without access to the EFI system table. Using EFI_RNG_PROTOCOL would be possible in theory if solving #4 gave us a way to externally provide randomness, but in practice EFI_RNG_PROTOCOL is almost always either unimplemented or aliased to RDRAND.

dhardy · 2019-06-14T12:04:30Z

Agreed, that sounds like a needless complication.

$ cargo build --target=x86_64-fortanix-unknown-sgx --all-features
   Compiling getrandom v0.1.3 (/home/travis/build/rust-random/getrandom)
error[E0537]: invalid predicate `and`
  --> src/rdrand.rs:34:7
   |
34 | #[cfg(and(target_env = "sgx", not(target_feature = "rdrand")))]
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think you want all.

src/lib.rs

josephlr · 2019-06-14T19:42:52Z

I think you want all.

Fixed

src/rdrand.rs

Cargo.toml

newpavlov · 2019-06-16T22:35:27Z

@dhardy
I think we can merge it?

tarcieri · 2019-07-09T18:20:46Z

src/rdrand.rs

-                return Ok(el.to_ne_bytes());
-            }
-        };
+        let mut el = mem::uninitialized();


A bit late to the party here, but I was curious why this uses mem::uninitialized() instead of mem::zeroed().

Zeroing seems relatively lightweight compared to RDRAND itself, would make RNG failures more obvious, and eliminates the potential risk of using attacker-controlled values which appear plausibly random at first glance as e.g. cryptographic keys.

Yeah, zeroed could be a better choice. Although in practice it does not matter, as both zeroed and uninitialized get optimized out.

Fixed in the latest commit.

@tarcieri good catch! It also gets rid of a deprecation warning on the latest nightly.

josephlr force-pushed the uefi branch from 4200c9f to 79339e1 Compare June 12, 2019 12:06

josephlr mentioned this pull request Jun 12, 2019

Remove TLS by using lazy_static #25

Merged

josephlr force-pushed the uefi branch 3 times, most recently from fec48c1 to c075ea6 Compare June 13, 2019 09:04

dhardy reviewed Jun 14, 2019

View reviewed changes

src/rdrand.rs Outdated Show resolved Hide resolved

newpavlov reviewed Jun 14, 2019

View reviewed changes

src/rdrand.rs Outdated Show resolved Hide resolved

newpavlov reviewed Jun 14, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

newpavlov approved these changes Jun 14, 2019

View reviewed changes

dhardy reviewed Jun 15, 2019

View reviewed changes

src/rdrand.rs Outdated Show resolved Hide resolved

josephlr added 5 commits June 15, 2019 21:24

Support x86_64-unknown-uefi

0381ee0

Use custom implementation of is_rdrand_supported

94440e1

Add missing not

fc8ab62

Add error for sgx && !rdrand

5c6294a

Fix cfg guards

25bd5a5

josephlr force-pushed the uefi branch from 5af954a to 5587f8c Compare June 16, 2019 04:45

newpavlov reviewed Jun 16, 2019

View reviewed changes

Cargo.toml Show resolved Hide resolved

josephlr force-pushed the uefi branch from 5587f8c to 53f1420 Compare June 16, 2019 21:25

Improve compile_error! message

8053d7e

josephlr force-pushed the uefi branch from 53f1420 to 8053d7e Compare June 16, 2019 22:24

dhardy merged commit 0b87a09 into rust-random:master Jun 17, 2019

josephlr deleted the uefi branch June 17, 2019 08:28

tarcieri reviewed Jul 9, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support x86_64-unknown-uefi #30

Support x86_64-unknown-uefi #30

josephlr commented Jun 12, 2019 •

edited

Loading

josephlr commented Jun 13, 2019

dhardy commented Jun 13, 2019

josephlr commented Jun 13, 2019

nagisa commented Jun 13, 2019 via email

nagisa commented Jun 13, 2019 via email

josephlr commented Jun 13, 2019

josephlr commented Jun 13, 2019

nagisa commented Jun 13, 2019

dhardy left a comment

newpavlov left a comment

josephlr commented Jun 14, 2019

dhardy commented Jun 14, 2019

josephlr commented Jun 14, 2019

newpavlov commented Jun 16, 2019

tarcieri Jul 9, 2019

newpavlov Jul 9, 2019

newpavlov Jul 9, 2019

josephlr Jul 9, 2019

Support x86_64-unknown-uefi #30

Support x86_64-unknown-uefi #30

Conversation

josephlr commented Jun 12, 2019 • edited Loading

josephlr commented Jun 13, 2019

dhardy commented Jun 13, 2019

josephlr commented Jun 13, 2019

nagisa commented Jun 13, 2019 via email

nagisa commented Jun 13, 2019 via email

josephlr commented Jun 13, 2019

josephlr commented Jun 13, 2019

nagisa commented Jun 13, 2019

dhardy left a comment

Choose a reason for hiding this comment

newpavlov left a comment

Choose a reason for hiding this comment

josephlr commented Jun 14, 2019

dhardy commented Jun 14, 2019

josephlr commented Jun 14, 2019

newpavlov commented Jun 16, 2019

tarcieri Jul 9, 2019

Choose a reason for hiding this comment

newpavlov Jul 9, 2019

Choose a reason for hiding this comment

newpavlov Jul 9, 2019

Choose a reason for hiding this comment

josephlr Jul 9, 2019

Choose a reason for hiding this comment

josephlr commented Jun 12, 2019 •

edited

Loading