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 random crate #582

Merged
merged 2 commits into from
Aug 23, 2022
Merged

Add random crate #582

merged 2 commits into from
Aug 23, 2022

Conversation

tsoutsman
Copy link
Member

The crate mimics the RngCore interface. It was named random to not
conflict with rand's symbols.

I've also replaced locally defined RNGs in the ixgbe and
ota_update_client crates, that used the ... HPET timer count as
a seed.

A further development could be to expose an add_entropy function.
I'm not sure it's a major concren because a CSPRNG seeded with 32 bytes
of entropy should be plenty secure. However, if RDSEED or RDRAND don't
exist, it is absolutely necessary. Adding entropy would require locking
the CSPRNG, generating a random seed, combining it with the entropy, and
reseeding the CSPRNG. This would somehow need to be arbitrated to ensure
the the CSPRNG isn't being constantly reseeded, impacting performance.

Our QEMU configuration currently doesn't support RDSEED, so the backup
RDRAND is used.

Signed-off-by: Klim Tsoutsman klim@tsoutsman.com

@NathanRoyer
Copy link
Member

This looks great!
Have you thought about implementing the RngCore trait?
Would that be a good thing?
https://docs.rs/rand_core/latest/rand_core/trait.RngCore.html

@tsoutsman
Copy link
Member Author

We only expose one source of randomness, so implementing the trait seems unnecessary. The functions would have to be converted to methods on a ZST, and the caller would have to import the RngCore trait. It also means we expose rand in the public API, which is notoriously unstable.

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Looks generally pretty good to me, I think it'll be an excellent addition to Theseus with a few suggested changes.

FWIW, I do think it would be beneficial to have tighter integration with the rand crate, perhaps by implementing one or more of its traits. We can do that in a future PR, but it seems like a quick & easy addition that would help to standardize random;s public interface.

kernel/ota_update_client/src/lib.rs Outdated Show resolved Hide resolved
kernel/random/src/lib.rs Show resolved Hide resolved
.or_else(|_| rdrand_seed())
.unwrap_or_else(|_| {
log::error!("Could not generate hardware seed - using a predetermined seed. THIS IS NOT OK.");
[1; 32]
Copy link
Member

Choose a reason for hiding this comment

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

this is okay, but it'd probably be better to explicitly initialize our RNG with a backup seed value obtained from something at least partially random, perhaps a current timer value (PIT/RTC/TSC/HPET) or something else.

For this, you can use the spin::Once type or OnceCell or something similar that allows you to pass in values to the init routine, instead of lazy_static. Might as well do that since you're initializing it from the captain anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea that's probably best. We'd need to ensure that rand is only initialised after the timers then.

@kevinaboos
Copy link
Member

So, to summarize the key points of our discussion:

  • Document why we have a single CSPRNG, and why we're using a CS one instead of non-CS
  • Describe how other crates would want to use this (use the CSPRNG to seed a non-CS PRNG)
  • Undo the changes to other crates that were using various random sources (we can address than in a new PR)
  • Add explicit initialization, perhaps from the captain, but another place could work too (device_manager?)

As part of a future PR we could:

  • Implement rand::RngCore for our CSPRNG such that other crates could use it to seed another instance of anything that implements rand::SeedableRng
  • Change crates across Theseus to use this new random crate, perhaps adding convenience interfaces to it that create new non-CS PRNG instances on demand for each caller

Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com>
@tsoutsman
Copy link
Member Author

tsoutsman commented Aug 20, 2022

I've changed the PR to simply add a new unused rand interface addressing the points of our discussion. I've added explicit initialization to captain but I'm not sure if we need it because the CSPRNG would just be initialised the first time we access it.

Running tsc_seed on my computer yields the following seed:

[
    1,
    110,
    179,
    224,
    12,
    55,
    96,
    138,
    175,
    208,
    241,
    18,
    51,
    84,
    117,
    148,
    179,
    212,
    243,
    18,
    49,
    80,
    111,
    142,
    175,
    206,
    237,
    29,
    60,
    93,
    126,
    157,
]

The bytes are very clearly increasing in a loop. I tried using just the last bit (calling tsc_ticks() 256 times) but this resulted in (I think) an even worse seed:

[
    177,
    169,
    181,
    85,
    170,
    170,
    85,
    90,
    170,
    85,
    74,
    170,
    85,
    170,
    165,
    85,
    170,
    85,
    74,
    170,
    85,
    42,
    169,
    85,
    42,
    173,
    85,
    170,
    213,
    82,
    170,
    85,
]

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

So now that you've made an infallible init function, we can simply use lazy_static as before. I didn't know you were going to go that route, hence why i suggested Once for an explicit init function that could fail. No need to call it from the captain either.

Also, kindly rename it back to random since rand is already a widely-known crate in the Rust ecosystem.

kernel/rand/src/lib.rs Outdated Show resolved Hide resolved
kernel/rand/src/lib.rs Outdated Show resolved Hide resolved
@tsoutsman
Copy link
Member Author

infallible init

That's because tsc is incorrectly infallible; it doesn't check whether the counter exists. But that's a separate issue.

@tsoutsman tsoutsman requested a review from kevinaboos August 22, 2022 23:13
@kevinaboos
Copy link
Member

That's because tsc is incorrectly infallible; it doesn't check whether the counter exists. But that's a separate issue.

Hmm, i see. While that technically might be a concern for ancient CPUs, I doubt it's of practical concern to us since we don't even support anything prior to 64-bit x86. AFAICT TSC has existed since Pentium was first introduced, so I think we'd be okay.

Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com>
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Excellent job with the docs, looks all good to me now.

@kevinaboos kevinaboos merged commit 35d5e9c into theseus-os:theseus_main Aug 23, 2022
github-actions bot pushed a commit that referenced this pull request Aug 23, 2022
* Currently based on the `ChaCha20Rng` from the `rand` project.

* To generate the initial seed, we use `rdseed`, then `rdrand`, then values from the TSC as a last resort.

Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com> 35d5e9c
github-actions bot pushed a commit to tsoutsman/Theseus that referenced this pull request Aug 23, 2022
* Currently based on the `ChaCha20Rng` from the `rand` project.

* To generate the initial seed, we use `rdseed`, then `rdrand`, then values from the TSC as a last resort.

Signed-off-by: Klim Tsoutsman <klim@tsoutsman.com> 35d5e9c
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.

3 participants