-
Notifications
You must be signed in to change notification settings - Fork 3k
RNG HAL addition #2765
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
RNG HAL addition #2765
Conversation
@@ -0,0 +1,29 @@ | |||
/* mbed Microcontroller Library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a more descriptive filename such as mbed_rng.c
. wrapper.c
isn't very descriptive.
I'm also not sure this is the best place to put the file (although I have no better suggestions yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mbed_rng.c
+1 , will rebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I'll rebase (correct this name) once the review is done .
|
||
int mbedtls_hardware_poll( void *data, unsigned char *output, size_t len, size_t *olen ) { | ||
rng_t rng_obj; | ||
rng_init(&rng_obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's a good idea to bring up and teardown the RNG per poll, dependent on hardware.
A lousy RNG might start giving us the same sequence of values after each initialisation. Better for an initialisation function called earlier by mbed TLS, and maybe a suspend/awake pair of functions for power management.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was taken from mbedtls_hardware_poll
implementation within mbed. Most of the functions in HAL did it - init, get numbers, deinit. I havent seen any other function that we could connect init/free
into. I assume this would be a new addition?
|
||
#if defined(MBEDTLS_ENTROPY_HARDWARE_ALT) && defined(DEVICE_RNG) | ||
|
||
int mbedtls_hardware_poll( void *data, unsigned char *output, size_t len, size_t *olen ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we tie the RNG into the entropy source API, it becomes our only possible entropy source. Is that what we want to do?
I think the RNG driver should be the default entropy source, but do we want it to be the only one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we make it not the only choice? How is this configurable with mbedtls?
|
||
#if DEVICE_RNG | ||
|
||
/** RNG HAL structure. rng_s is declared in the target's HAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout, we call this the mbed RNG HAL API. Can we make it clear this needs to be for a TRNG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the API and vendors naming, most of peripherals I have seen are called RNG.
Can we make it clear this needs to be for a TRNG
Proposing to add a comment or to rename the prefix to trng_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trng_api.h
|
||
#include "hal/rng_api.h" | ||
|
||
#if defined(MBEDTLS_ENTROPY_HARDWARE_ALT) && defined(DEVICE_RNG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change DEVICE_RNG
to DEVICE_HW_RNG
? I'd like to be clearer that it's got to be a TRNG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from mbed, where we use DEVICE_
for instance DEVICE_SPI
. Anything starting with that prefix, is HAL related
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEVICE_TRNG then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please change the symbol to DEVICE_TRNG
?
* @param output_length The length of generated data | ||
* @return 0 success, -1 fail | ||
*/ | ||
int rng_get_numbers(rng_t *obj, uint8_t *output, size_t length, size_t *output_length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be thread safe? (open question and needs to be checked)
mbed TLS, as a library, often leaves concurrency issues to the application software above it, or the hardware drivers underneath it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above layer (caller) should implement thread safety. mbed HAL is not thread-safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use numbers! get_random() or get_random_bytes() or get_bytes()... But don't use numbers (anywhere in the code for that matter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also although our interface looks the same right now, but we may add a new int*
parameter in order to make it possible to indicate the number of bits of entropy in the output buffer, because we will need it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_bytes()
sounds good, will rename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also although our interface looks the same right now, but we may add a new int* parameter in order to make it possible to indicate the number of bits of entropy in the output buffer, because we will need it in the future.
What would the new API look like, please paste here a declaration.
Something like:
int rng_get_numbers(rng_t *obj, uint8_t *output, size_t length, size_t *output_length, uint8_t *output_number_bits);
?
#include "device.h" | ||
|
||
#if DEVICE_RNG | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid user application software taking all the entropy, we should make it clear in some comment that this API is for the exclusive use of mbed TLS when present.
Ideally, we should have some lock to restrict it to mbed TLS only. (Although I'm not sure how we could do that without cost).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I add a comment there that there should be only one consumer of this API , would that be sufficient?
mbed HAL is not user-facing API (there has been always a warning that this API might change, and should not be used in the user space), there're mbed drivers there .
|
||
#include "hal/rng_api.h" | ||
|
||
#if defined(MBEDTLS_ENTROPY_HARDWARE_ALT) && defined(DEVICE_RNG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should require MBEDTLS_ENTROPY_HARDWARE_ALT
if we have DEVICE_RNG
. I think mbed TLS should self-configure. That would require a change to mbed TLS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some review feedback needs discussion.
* @param obj The RNG object | ||
* @param seed_value Entropy value to be set | ||
*/ | ||
void rng_set_seed(rng_t *obj, uint32_t seed_value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like this function. mbed TLS doesn't need to seed it, and it's supposed to be a TRNG. If a hardware block needs seeding, maybe better to let the driver handle this itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a hardware block needs seeding, maybe better to let the driver handle this itself.
That's what I was not certain about and added it here with a question mark. get_numbers
would implement seeding if required as you stated.
PRNG_DISABLE_INT(); | ||
NVIC_DisableIRQ(CRPT_IRQn); | ||
// CLK_DisableModuleClock(CRPT_MODULE); | ||
|
||
//CLK_DisableModuleClock(CRPT_MODULE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out line? This doesn't look good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I havent cleaned yet that file, there are lot of commented lines, will do
* @param seed_value Entropy value to be set | ||
*/ | ||
void rng_set_seed(rng_t *obj, uint32_t seed_value); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also have a function for a self-test that mbed TLS could run as part of it's own self-verification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the name and purpose? I don't follow what self-verification means
#if DEVICE_RNG | ||
|
||
/** RNG HAL structure. rng_s is declared in the target's HAL | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mbed TLS has an interface for an entropy source, whilst this is an interface for a driver for a HW RNG. Is that correct, or do we really want an interface for an entropy source?
@pjbakker suggested it makes most sense for this to remain as a TRNG API, which I think is correct, but if so, partners may not want to implement this API and instead stick with the existing API - so we'd still need both means of porting an entropy source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be for HW RNG. Interface for entropy would be built on top of it.
partners may not want to implement this API and instead stick with the existing API
Can you elaborate? Do you mean that they have implemented mbedtls_hardware_poll
and want to use that one?
@@ -565,7 +565,7 @@ | |||
"inherits": ["Target"], | |||
"progen": {"target": "frdm-k64f"}, | |||
"detect_code": ["0240"], | |||
"device_has": ["ANALOGIN", "ANALOGOUT", "ERROR_RED", "I2C", "I2CSLAVE", "INTERRUPTIN", "LOWPOWERTIMER", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "RTC", "SERIAL", "SERIAL_FC", "SLEEP", "SPI", "SPISLAVE", "STDIO_MESSAGES", "STORAGE"], | |||
"device_has": ["ANALOGIN", "ANALOGOUT", "ERROR_RED", "I2C", "I2CSLAVE", "INTERRUPTIN", "LOWPOWERTIMER", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "RTC", "SERIAL", "SERIAL_FC", "SLEEP", "SPI", "SPISLAVE", "STDIO_MESSAGES", "STORAGE", "RNG"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR contradicts our own to introduce the self tests #2716. This is because we are introducing a new macro DEVICE_ENTROPY_SOURCE
to tell whether the config.h in use should be the default or a trimmed-down one that only enables features that do not require and entropy source to work. If we merge both PR we have two symbols that then would overlap (RNG
and DEVICE_ENTROPY_SOURCE
). That is, if we remove one of the symbols in both PRs, that would cause some issues because the RNG is not the only entropy source that could be supported in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are you suggesting?
|
||
/* Set "Interrupt Mask", "High Assurance" and "Go", | ||
* unset "Clear interrupt" and "Sleep" */ | ||
RNG->CR = RNG_CR_INTM_MASK | RNG_CR_HA_MASK | RNG_CR_GO_MASK; | ||
|
||
for( i = 0; i < len; i++ ) | ||
rng_get_byte( output + i ); | ||
for (i = 0; i < length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are going to change the formatting in this function, it would be ideal if you also fix rng_get_byte()
*/ | ||
int mbedtls_hardware_poll( void *data, | ||
unsigned char *output, size_t len, size_t *olen ) | ||
int rng_get_numbers(rng_t *obj, uint8_t *output, size_t length, size_t *output_length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not compiled this, but I believe this would probably cause an unused parameter obj
warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will "fix"
*/ | ||
int mbedtls_hardware_poll( void *data, | ||
unsigned char *output, size_t len, size_t *olen ) | ||
int rng_get_numbers(rng_t *obj, uint8_t *output, size_t length, size_t *output_length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, this would probably cause an unused parameter obj
warning.
@@ -61,6 +61,10 @@ struct dac_s { | |||
DACName dac; | |||
}; | |||
|
|||
struct rng_s { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why mbedtls has a void *
as a first parameter. Indeed, the API should even work when the value of the parameter is NULL
. However, I am not entirely sure why this API has a first parameter which is a completely empty structure. Perhaps, at least there should be a way to tell whether it has been initialised or something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the C HAL convention to avoid having global data in C HAL. Not all HAL implementation require private data, therefore for some platforms it might be empty structure.
|
||
int mbedtls_hardware_poll( void *data, | ||
unsigned char *output, size_t len, size_t *olen ) | ||
int rng_get_numbers(rng_t *obj, uint8_t *output, size_t length, size_t *output_length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as before with parameter obj
.
if (length < 32) { | ||
unsigned char tmpBuff[32]; | ||
rng_get(tmpBuff); | ||
memcpy( output, &tmpBuff, length ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use different coding styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That file needs a clean up , will do. Did not intend to make more noise changing the style, I did for some lines I touched though :-)
Pushed an update (to answer some of the above comments) |
Removed set seed function and rename RNG to TRNG as requested and rebased to resolve build errors. |
1ed1dd6
to
9ed82d6
Compare
@@ -0,0 +1,29 @@ | |||
/* mbed Microcontroller Library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file name to be mbed_trng.c ?
|
||
#if DEVICE_TRNG | ||
|
||
/** tRNG HAL structure. trng_s is declared in the target's HAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t to T ?
#endif | ||
|
||
/** | ||
* \defgroup hal_trng tRNG hal functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t to T ?
* @{ | ||
*/ | ||
|
||
/** Initialize the tRNG peripheral |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t to T ?
|
||
/** Initialize the tRNG peripheral | ||
* | ||
* @param obj The tRNG object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t to T ?
@@ -26,12 +26,27 @@ | |||
#include "cmsis.h" | |||
#include "fsl_common.h" | |||
#include "fsl_clock.h" | |||
#include "trng_api.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file name to be trng_api.c?
@@ -26,58 +26,58 @@ | |||
#include "cmsis.h" | |||
#include "fsl_common.h" | |||
#include "fsl_clock.h" | |||
#include "trng_api.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file name to be trng_api.c?
@@ -0,0 +1,99 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file name to be trng_api.c?
@@ -25,58 +25,53 @@ | |||
defined(TARGET_STM32F479xx) | |||
#include <stdlib.h> | |||
#include "cmsis.h" | |||
#include "trng_api.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file name to be trng_api.c?
@@ -21,58 +21,52 @@ | |||
|
|||
#include <stdlib.h> | |||
#include "cmsis.h" | |||
#include "trng_api.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file name to be trng_api.c?
9ed82d6
to
7803ddd
Compare
The renames were fixed and rebased on top of master to resolve conflicts. Fixing all build problems now |
7803ddd
to
b9f34ad
Compare
#include <stddef.h> | ||
#include "device.h" | ||
|
||
#if DEVICE_TRNG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might just be a cosmetic change, but perhaps this should be #if defined(DEVICE_TRNG)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to change it this morning then realized this is how it is within HAL (if we go that path, should be fixed everywhere). lets keep it as it is
int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length); | ||
|
||
/**@}*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also would need a int trng_get_min_entropy()
function that returns an estimate on the min entropy of the TRNG. The amount of entropy returned by each call of trng_get_bytes
MUST be at least this much. This is usually a static value, a characteristic of the hardware and the driver. (The amount of entropy in a single returned byte can be anything between 0 and 8)
mbed TLS will call this function automatically and use this value to calculate how many times we have to poll to achieve the configured level of cryptographic strength.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to put a reference in a comment on how to estimate the entropy of a source. Section 6 in the standard: http://csrc.nist.gov/publications/drafts/800-90/sp800-90b_second_draft.pdf describes it in great detail with easy to follow implementation instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably discussed this with the others, but just to be sure I write it here too: The trng_get_min_entropy
function is not necessary for this release. It is postponed and it will be redesigned before implementation.
7bc695a
to
5c09ba7
Compare
@yanesca I added |
int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length); | ||
|
||
/** Get minimum entropy bits per byte returned via get_bytes function | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to put a reference in a comment on how to estimate the entropy of a source. Section 6 in the standard: http://csrc.nist.gov/publications/drafts/800-90/sp800-90b_second_draft.pdf describes it in great detail with easy to follow implementation instructions.
What may worth pointing out:
- these tests described in the standard shouldn't be performed by this function
- they should be performed separately "offline" and the resulting estimate supplied as a constant in the implementation of this function
- this value must be a lower estimate
trng_get_bytes
must be implemented in a way that it returns at least one bit of entropy (Example: if the entropy estimate for a 4 byte value is 0.35 bit, thentrng_get_bytes
must load at least 12 bytes in the output buffer)
* | ||
* @param obj The TRNG object | ||
* @param output The pointer to an output array | ||
* @param length The length of output data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to clarify here that this is the size of the output buffer, they don't have to fill this buffer before returning, this value is only there only to help them avoid a buffer overwrite.
*/ | ||
int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length); | ||
|
||
/** Get minimum entropy bits per byte returned via get_bytes function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should return the number of bits of entropy per call returned via get_bytes
function and not per bytes.
@yanesca Docs fixed according to your comments above |
*/ | ||
int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length); | ||
|
||
/** Get minimum entropy bits per call of the function get_bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function needs to be removed as we're deferring the feature to Q4.
@@ -0,0 +1,29 @@ | |||
/* mbed Microcontroller Library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is floating around in the mbed TLS root directory. Could we create a directory, mbed-os/features/mbedtls/platform
for mbed OS specific code related to mbed TLS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
platform
will do . This needs also rebase, so I'll do changes, and rebase on top of master.
fc19adc
to
cba85cc
Compare
Rebased. @sbutcher-arm : minimum entropy function was removed, and |
Result: FAILUREYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 961 Build failed! |
bc0e20a
to
537ccbb
Compare
Otherwise it's implementation specific, IAR fails to compile.
MBEDTLS_ENTROPY_HARDWARE_ALT will be defined via config in mbedtls, the mbed wrapper should use DEVICE_TRNG.
537ccbb
to
1b95c67
Compare
All rebased again (while rebasing the last 24 hours, I lost some of my work thus those failues, I resolved them again). Will trigger morph test once the basic checks are done |
+1 |
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 963 All builds and test passed! |
Tests all green, ready for merging @andresag01 Please confirm that mbedtls tests are working with this changeset |
@0xc0170 I have tested this change together with #2716 and Mbed-TLS/mbedtls#615 and it seems to work correctly. However, it would be ideal if #2716 is reviewed and approved and Mbed-TLS/mbedtls#615 is merged before this PR is merged. |
It has disappeeared with #2765
It has disappeeared with #2765
Description
Add RNG HAL API + mbedtls wrapper to use it. There are 4 functions, 3 are implemented for all targets (port from the old implementation to the new one):
Status
IN DEVELOPMENT
Todos
@sg- @sbutcher-arm @andresag01 @yanesca @pjbakker - please review