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

Implement basic support for the TRNG #99

Merged
merged 3 commits into from
Nov 17, 2020
Merged

Implement basic support for the TRNG #99

merged 3 commits into from
Nov 17, 2020

Conversation

AlyoshaVasilieva
Copy link
Contributor

@AlyoshaVasilieva AlyoshaVasilieva commented Nov 13, 2020

This adds support for the True Random Number Generator. It has a bunch of configurable things, few of which are safe to touch without reading the Security Reference Manual. The SRM requires a licensing agreement (and NDA) with NXP, so this is written with reference to the MCUXpresso SDK.

I think this driver should work for any i.MX chip other than the 1015 and 1021, which need TRNG_ACC in MCTL set to 1 after entering run mode. That field doesn't exist on other i.MX RT chips (it's UNUSED5). I think everything else is identical for the imxrt chips.

The output passes dieharder and practrand (out to 2GB, anyway). The TRNG is very slow and it took weeks to generate that much data.

This retrieves all 512 bits of entropy at once from the TRNG, rather than reading 32 bits at a time. This is done for two reasons:

  1. I don't know how to cleanly write it that way
  2. Reading the final register triggers new entropy generation, so this way using the entropy slowly won't ever actually block. Cost: 64 bytes of RAM.

Includes support for embedded-hal's RNG Read trait, which may change in 1.0.

Disabling the TRNG is slightly weird so I implemented support for that.

Hopefully I didn't miss anything in this.

Limited configurability but it works.
@teburd
Copy link
Member

teburd commented Nov 13, 2020

Very cool addition! I wonder if this could implement std::rand::RngCore so it could be used with the std rand module? Doesn't seem too far off, would need an impl for next_u64, fill_bytes, and try_fill_bytes in addition to the already existing next_u32

https://docs.rs/rand/0.7.3/rand/trait.RngCore.html

@AlyoshaVasilieva
Copy link
Contributor Author

AlyoshaVasilieva commented Nov 14, 2020

There's two issues with supporting RngCore that I know of:

  1. Cargo's current feature resolver unifies dependency features, so a dev-dep that enables rand_core's std feature (apparently fairly common) will cause a build failure unless using Cargo's unstable new feature resolver is used. Easy workaround: Make the RngCore implementation opt-in by a crate feature, with a note somewhere to warn users.
  2. The only function in RngCore that allows reporting errors is try_fill_bytes. While I never saw a TRNG error unless triggering one with bad settings, I don't know how rare they really are. Error handling would therefore be limited to either panicking on error or looping to wait for an Ok result. Maybe retry a few times then panic?

I'll add an RngCore implementation (probably as a wrapper created by into_rng on the TRNG struct) sometime soon, since interoperability with a more common RNG abstraction is useful. Do you have an opinion on which method to use for error handling? I'll probably go for a limited number of retries (some approximate fraction of a second) followed by a panic. Wait, the TRNG already has a hardware option for up to 15 retries. I'll probably just panic on error and point the user at that.

@AlyoshaVasilieva
Copy link
Contributor Author

AlyoshaVasilieva commented Nov 14, 2020

Added optional RngCore impl. Open to opinions on the struct's name. (Can't be an optional implementation on the original TRNG struct unless the original next_u32 has its name changed, maybe to try_next_u32; that's also one way forward and may be better.)

I used intradoc links in a couple places of the documentation. They aren't stable yet, but their stabilization is occurring with Rust 1.48 (around five days from now). TRNG docs render fine on beta.

Copy link
Member

@mciantyre mciantyre left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thanks for all the thorough testing and documentation.


How should this work?

let peripherals = imxrt1060_hal::Peripherals::take().unwrap();

let mut unclocked_trng = peripherals.trng;
// Set my TRNG settings, which I expect to persist
// between clocked & unclocked states.
unclocked_trng.set_retry_count(13);
unclocked_trng.set_sample_mode(SampleMode::Raw);

let mut trng = unclocked_trng.clock(&mut peripherals.ccm);
// Do random number things

let unclocked_again = trng.disable(&mut peripherals.ccm);
let mut trng = unclocked_again.clock(&mut peripherals.ccm);
// Retry count is 1, not 13
// Sample mode is VonNeumannRaw, not Raw

A user might expect the TRNG settings to persist across clocked and unclocked states. But it looks like we forget the previous settings on every transition from clocked to unclocked. This is OK with me; we might note this in the disable documentation.


/// The specified retry count was outside of the valid range of `1..=15`.
#[derive(Copy, Clone, Debug)]
pub struct InvalidRetryCountError;
Copy link
Member

Choose a reason for hiding this comment

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

This error struct could be constructed by anyone outside of the HAL crate. That's not a big deal, but we might want to give others the guarantee that we're the only people that can create this.

Suggested change
pub struct InvalidRetryCountError;
pub struct InvalidRetryCountError(());

Could demonstrate this with a failing doctest:

/// ```compile_fail
/// use imxrt1060_hal as hal;
/// use hal::trng::InvalidRetryCountError;
/// let err = InvalidRetryCountError(());
/// ```
#[cfg(doctest)]
struct UnconstructibleError;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that! Wasn't paying enough attention.

The config errors of I2C and SPI (ClockSpeedError, PinLowTimeoutError, BusIdleTimeoutError, ModeError) are also missing the private empty tuple. (I think I just mimicked them without using my brain.) Will make a PR to also make those unconstructible.

Added a slightly modified version of that test. With the (()) suffix it passed on both the wrongly constructible error and the correct private error.

@mciantyre mciantyre linked an issue Nov 15, 2020 that may be closed by this pull request
@AlyoshaVasilieva
Copy link
Contributor Author

AlyoshaVasilieva commented Nov 15, 2020

My original expectation for disable is that a user might turn the TRNG on, grab some entropy, feed it into a PRNG, and then turn the TRNG off for the rest of operation to potentially preserve some tiny amount of power. But it's possible they may want to turn it back on if they are doing cryptographic work and want more entropy. So, added preservation of settings. Rather than preserving them on the struct, I retrieve them from the registers during disable; this efficiency-complexity tradeoff may not be worth it, so I'll switch to just storing the settings on the TRNG struct if you'd prefer.

@teburd
Copy link
Member

teburd commented Nov 16, 2020

This looks good to me as well

@mciantyre mciantyre merged commit c3bb81c into imxrt-rs:master Nov 17, 2020
@mciantyre mciantyre mentioned this pull request Nov 17, 2020
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.

Support iMXRT1062 TRNG
3 participants