Initial commit of RSA Key support #2839

alistair23 · 2021-09-24T02:52:18Z

Pull Request Overview

This is a split from #2693 adding RSA Key support.

Why add a trait and capsule?

Public/Private keys are too complex and critical to be handled in plain buffers. Using plain buffers opens us up to easy mistakes of mismatching keys type or extracting useful information.

For this reason this PR adds a generic HIL for public/private key pairs. Using this generic HIL we also add a RSA specific HIL and two implementations of RSA keys. We have two implementations to support the case where we have both public/private keys and the case where we only have a public key. This way we can avoid overhead of private key storage if we aren't going to use it.

Why add a new enum type?

The goal is to support 3 main use cases:

Keys stored on flash. The keys are stored at some address in read only flash and we want to "import" them and use them in the kernel
The app specifies a key. A userspace application obtains a key and passes it to the kernel to use for crypto operations
We generate a key pair while running

In order to support all three we need to accept both mutable buffers (for userspace driver capsules and generating keys) and immutable buffers (for data stored in flash).

In order to support both I have added a new enum called MutImutBuffer. This allows us to pass around either a mutable static buffer or an immutable static buffer. This can be passed all the way down to the chip driver which can then unpack the buffers and write the data to hardware.

Testing Strategy

This has been tested with the OpenTitan RSA implementation: #2899

TODO or Help Wanted

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make prepush.

kernel/src/hil/public_key_crypto/keys.rs

capsules/src/public_key_crypto/rsa_keys.rs

alistair23 · 2021-09-27T07:09:55Z

Ok, I'm a little stuck.

There are three main use cases I think we need to support:

Keys stored on flash. The keys are stored at some address in read only flash and we want to "import" them and use them in the kernel
The app specifies a key. A userspace application obtains a key and passes it to the kernel to use for crypto operations
We generate a key pair while running

Ideally we want to support all 3 without too much overhead.

My original plan was that a board would create a single RSA2048Keys struct. It would then copy over a key from flash, perform operations and then move on to the next key. This has the large downside that we have to store the key in memory. My hope though was that we could create a very small number of structs and re-use them. So for example:

let key = RSA2048Keys::new()
key.import(&flash_key_one);
key.do_stuff();
key.import(&flash_key_two);
key.do_stuff();

This means we need to keep a single key in memory, but can support any number of keys. We can also support all 3 use cases above. The problem is I think this is too much over head as we have to store large keys in memory.

The other option is to have an interface that takes a static key, like this:

    /// Import an existing public key. This will override any existing keys.
    ///
    /// The `public_key` is stored internally and can be retrieved with
    /// `pub_key()`.
    ///
    /// The possible ErrorCodes are:
    ///     - `ALREADY`: A key is already imported or in the process of being
    ///                  generated.
    ///     - `INVAL`: An invalid key was supplied.
    ///     - `SIZE`: An invalid key size was supplied.
    ///     - `NOMEM`: The key can not be stored.
    fn import_public_key(
        &'a self,
        public_key: &'static [u8],
    ) -> Result<(), (&'static [u8], ErrorCode)>;

    /// Return the public key supplied by `import_public_key()` or
    /// `generate()`.
    ///
    /// On success the return value is `Ok(())`.
    /// On failure the possible ErrorCodes are:
    ///     - `NODEVICE`: The key does not exist
    ///     - `SIZE`: An invalid buffer size was supplied.
    ///     - `NOMEM`: The key can not be stored.
    fn pub_key(&'a self) -> Result<&'static [u8], ErrorCode>;

The problem here is mutability. We need a mutable buffer to allow generating keys and working with apps, but that won't work with read only flash. It's a similar issue to tock/libtock-rs#334

We could have read and read/write traits based on where the key is coming from. Am I missing something, does anyone have a better idea?

alistair23 · 2021-11-12T01:39:05Z

Marking this as ready for review as there is now a full implementation

phil-levis

This should be updated to not use MutImutBuffer.

capsules/src/public_key_crypto/rsa_keys.rs

alistair23 · 2021-12-02T03:24:57Z

Updated to not use MutImutBuffer in any HIL

2852: kernel: utilities: Initial commit of mut_imut_buffer r=hudson-ayers a=alistair23 ### Pull Request Overview There are a few times inside the Tock kernel where we want to support either a `'static mut` buffer or a `'static` buffer. Examples of this include the [RSA key support](#2839) but also the [OTBN](https://github.com/tock/tock/blob/master/chips/lowrisc/src/otbn.rs#L153). In order to support both I have added a new enum called MutImutBuffer. This allows us to pass around either a mutable static buffer or an immutable static buffer. This can be passed all the way down to the chip driver which can then unpack the buffers and write the data to hardware. ### Testing Strategy CI and RSA key implementation ### TODO or Help Wanted ### Documentation Updated - [X] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [X] Ran `make prepush`. Co-authored-by: Alistair Francis <alistair.francis@wdc.com>

alistair23 · 2021-12-10T01:30:04Z

Rebased on master and all comments should be addresses

phil-levis · 2022-01-14T05:11:47Z

Hey, getting back to this now. I think we resolved all of the issues with keys, but I suspect that rsa_math is going to require some more work due to the mutable/immutable problem. Can you cut that file so I can approve this?

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

alistair23 · 2022-01-14T07:13:43Z

Done!

phil-levis

I think the TRD needs a lot of work, but that can be improved later.

alistair23 · 2022-01-31T03:54:18Z

Can this be merged?

hudson-ayers · 2022-02-16T15:50:32Z

bors r+

bors · 2022-02-16T16:09:05Z

Build succeeded:

2899: OpenTitan: Add support for hardware RSA operations r=phil-levis a=alistair23 ### Pull Request Overview This PR builds on top of: * #2898 * #2892 * #2839 and adds support for RSA hardware operations using the OTBN. ### Testing Strategy Running the RSA test in this PR on the CW310 FPGA. ### TODO or Help Wanted ### Documentation Updated - [X] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [X] Ran `make prepush`. Co-authored-by: Alistair Francis <alistair.francis@wdc.com>

github-actions bot added the HIL This affects a Tock HIL interface. label Sep 24, 2021

alistair23 force-pushed the alistair/rsa-keys branch from dcad160 to 651a1c0 Compare September 24, 2021 02:59

github-actions bot added the kernel label Sep 24, 2021