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

[cryptolib] csrng driver #14372

Merged
merged 2 commits into from
Aug 30, 2022
Merged

[cryptolib] csrng driver #14372

merged 2 commits into from
Aug 30, 2022

Conversation

moidx
Copy link
Contributor

@moidx moidx commented Aug 18, 2022

This commit introduces the a driver for the software instance of the
csrng. The driver interface supports regular DRBG functions, i.e.
instantiate, reseed, update and generate functions.

This driver uses the status.h module for error reporting/handling.

The focus of this commit is basic functionality. There are a few things that
will be implemented in follow up commits:

  • Handling of blocking operations. This is currently an area where we
    need to come up with a strategy for the crypto library as indefinite
    blocking operations may need an unconditional timeout when called in
    higher application layers.
  • alert and state checks.

Signed-off-by: Miguel Osorio [email protected]

@moidx moidx requested review from cfrantz and a team as code owners August 18, 2022 22:41
@moidx moidx removed the request for review from a team August 19, 2022 02:00
Copy link
Contributor

@jadephilipoom jadephilipoom left a comment

Choose a reason for hiding this comment

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

LGTM!

sw/device/lib/crypto/drivers/entropy_kat.c Outdated Show resolved Hide resolved
sw/device/lib/crypto/drivers/entropy_kat.c Outdated Show resolved Hide resolved
sw/device/lib/crypto/drivers/entropy_kat.c Show resolved Hide resolved
sw/device/lib/crypto/drivers/entropy_kat.c Show resolved Hide resolved
Copy link
Contributor

@alphan alphan left a comment

Choose a reason for hiding this comment

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

Thanks @moidx!

sw/device/lib/crypto/drivers/entropy.c Outdated Show resolved Hide resolved
#include "sw/device/lib/testing/test_framework/ottf_main.h"

OTTF_DEFINE_TEST_CONFIG();
bool test_main(void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not related to this PR but I think the return type of test_main() could be status_t, wdyt @timothytrippel ?

Copy link
Contributor

@timothytrippel timothytrippel Aug 26, 2022

Choose a reason for hiding this comment

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

It could, but I'm not sure what that would give us as the return type here gets piped through the OTTF to a test_status_t that gets backdoor read by the testbench (which has aligned SV code) or piped over UART for the FPGA, and all we really care about is whether the test passed or not don't we?

sw/device/lib/crypto/drivers/entropy.c Show resolved Hide resolved
Comment on lines +63 to +69
do {
reg = abs_mmio_read32(kBaseCsrng + CSRNG_SW_CMD_STS_REG_OFFSET);
cmd_ready = bitfield_bit32_read(reg, CSRNG_SW_CMD_STS_CMD_RDY_BIT);
} while (!cmd_ready);
Copy link
Contributor

Choose a reason for hiding this comment

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

hardening: this is a single bit, wdyt about something like this?

Copy link
Contributor Author

@moidx moidx Aug 29, 2022

Choose a reason for hiding this comment

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

It sounds like this should also be noted in #14542. Added to the list.

Comment on lines +25 to +26
* Number of words set in `data`. CSRNG will extend the `data` to zeros if the
* provided value is less than 12.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: will CSRNG pad at the start or end? Can we just require this from the clients for the sake of simplicity or do you anticipate a considerable difference in performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done by hardware at the moment. The padding is added at the end.

sw/device/lib/crypto/drivers/entropy.c Outdated Show resolved Hide resolved
sw/device/lib/crypto/drivers/entropy.c Show resolved Hide resolved
sw/device/lib/crypto/drivers/entropy.c Outdated Show resolved Hide resolved
sw/device/lib/crypto/drivers/entropy.c Show resolved Hide resolved
@@ -0,0 +1,169 @@
// Copyright lowRISC contributors.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I know "kat" stands for "known answer test" but wdyt about renaming this file to entropy_kat_test.c for the sake of clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to use the _test.c to avoid giving the impression that it is a functional test. wdyt?

Move the required args to the end and name them the "test commands."
Pass the test commands via an environment variable, and place them at
the end of the invocation.

This makes the test commands unable to be overridden by test_arg
arguments, and simultaneously, it allows the user to specify test_arg
arguments on the bazel command line to modify arguments to opentitantool
and friends. Those test_arg arguments can override or supplement the
test harness's arguments provided by the bazel rule.

Signed-off-by: Alexander Williams <[email protected]>
Signed-off-by: Miguel Osorio <[email protected]>
This commit introduces the a driver for the software instance of the
csrng. The driver interface supports regular DRBG functions, i.e.
instantiate, reseed, update and generate functions.

The focus of this commit is functionality, so there are a few things
that will be addressed in future commits:

- Handling of blocking operations. This is currently an area where we
  need to come up with a strategy for the crypto library as indefinite
  blocking operations may need an unconditional timeout when called in
  higher application layers.
- alert and state checks.

Signed-off-by: Miguel Osorio <[email protected]>
@moidx moidx merged commit 6be5b88 into lowRISC:master Aug 30, 2022
@moidx moidx deleted the cryptolib-driver-entropy branch August 30, 2022 03:10
@moidx moidx mentioned this pull request Aug 30, 2022
8 tasks
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