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.

Sign up for GitHub

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

Jump to bottom

crypto/mbedtls: add hardware entropy config hooks #2184

Merged
merged 1 commit into from
Feb 26, 2020
Merged

crypto/mbedtls: add hardware entropy config hooks #2184

merged 1 commit into from
Feb 26, 2020

Conversation

nkaje
Copy link
Contributor

@nkaje nkaje commented Feb 7, 2020

No description provided.

nkaje
nkaje commented Feb 7, 2020
View reviewed changes
hw/drivers/trng/trng_da1469x/include/trng_da1469x.h Outdated
@@ -27,7 +27,7 @@ extern "C" {
#endif

int da1469x_trng_init(struct os_dev *dev, void *arg);

int mbedtls_hardware_poll( void *data, unsigned char *output, size_t len, size_t *olen );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guard with #ifdef MBEDTLS_ENTROPY_HARDWARE_ALT

@nkaje nkaje force-pushed the mbedtls_aes branch 2 times, most recently from e67ce82 to 40360ab Compare February 10, 2020 18:06
nkaje
nkaje commented Feb 10, 2020
View reviewed changes
crypto/mbedtls/syscfg.yml
@@ -125,6 +125,9 @@ syscfg.defs:
MBEDTLS_ENTROPY_C:
value: 1

MBEDTLS_ENTROPY_HARDWARE_ALT:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

by default 0, override from app's syscfg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comment.

kasjer
kasjer reviewed Feb 21, 2020
View reviewed changes
hw/drivers/trng/trng_da1469x/src/trng_da1469x.c Outdated

#ifdef MBEDTLS_ENTROPY_HARDWARE_ALT
int
mbedtls_hardware_poll( void *data, unsigned char *output, size_t len, size_t *olen ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space in argument list

hw/drivers/trng/trng_da1469x/src/trng_da1469x.c Outdated
trng = (struct trng_dev *)os_dev_lookup("trng");
da1469x_trng_init((struct os_dev *)trng, NULL);
int ret = da1469x_trng_read(trng, output, len);
if ( ret == len )
Copy link
Contributor

Choose a reason for hiding this comment

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

extra spaces and missing {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix

hw/drivers/trng/trng_da1469x/src/trng_da1469x.c Outdated
if ( ret == len )
*olen = len;
else
olen = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

olen = NULL dose not make sense here

hw/drivers/trng/trng_da1469x/src/trng_da1469x.c Outdated

trng = (struct trng_dev *)os_dev_lookup("trng");
da1469x_trng_init((struct os_dev *)trng, NULL);
int ret = da1469x_trng_read(trng, output, len);
Copy link
Contributor

Choose a reason for hiding this comment

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

in mynewt variable declaration are not placed in the middle of block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will change it

hw/drivers/trng/trng_da1469x/include/trng_da1469x.h Outdated
@@ -27,6 +27,9 @@ extern "C" {
#endif

int da1469x_trng_init(struct os_dev *dev, void *arg);
#ifdef MBEDTLS_ENTROPY_HARDWARE_ALT
int mbedtls_hardware_poll( void *data, unsigned char *output, size_t len, size_t *olen );
Copy link
Contributor

Choose a reason for hiding this comment

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

this prototype actually exists in entorpy_poll.h so placing it here again seems strange

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'll check on that

crypto/mbedtls/syscfg.yml
@@ -125,6 +125,9 @@ syscfg.defs:
MBEDTLS_ENTROPY_C:
value: 1

MBEDTLS_ENTROPY_HARDWARE_ALT:
value: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

it would not hurt to provide description field, but since this file lack any descriptions I don't mind after consideration

@nkaje nkaje changed the title WIP DONOT MERGE: crypto/mbedtls: Enable AES and SECP256R1 crypto/mbedtls: Enable AES and SECP256R1 Feb 24, 2020
@utzig
Copy link
Member

utzig commented Feb 25, 2020

The name "Enable AES and SECP256R1" has no relation with the changes...

@nkaje nkaje changed the title crypto/mbedtls: Enable AES and SECP256R1 crypto/mbedtls: add hardware entropy config hooks Feb 25, 2020
utzig
utzig reviewed Feb 25, 2020
View reviewed changes
hw/drivers/trng/src/mbed_entropy_alt.c
*olen = len;
} else {
*olen = 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

trng_read does not return errors, so this testing could be eliminated by simply doing *olen = trng_read(trng, output, len);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

hw/drivers/trng/src/mbed_entropy_alt.c
struct trng_dev *trng;
int ret;

trng = (struct trng_dev *)os_dev_lookup("trng");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe addding

if (trng == NULL) {
    return -1;
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add.

utzig
utzig reviewed Feb 25, 2020
View reviewed changes
hw/drivers/trng/src/mbed_entropy_alt.c Outdated
#include <trng/trng.h>
#include "mbedtls/config_mynewt.h"

#ifdef MBEDTLS_ENTROPY_HARDWARE_ALT
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid including mbedtls/config_mynewt.h and using instead #if MYNEWT_VAL(MBEDTLS_ENTROPY_HARDWARE_ALT) here.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah thats a good point

nkaje
nkaje commented Feb 25, 2020
View reviewed changes
crypto/mbedtls/include/mbedtls/config_mynewt.h
@@ -292,6 +293,10 @@ extern "C" {
#undef MBEDTLS_ENTROPY_C
#endif

#if MYNEWT_VAL(MBEDTLS_ENTROPY_HARDWARE_ALT) == 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though not used now, keeping this for completeness.

crypto/mbedtls/include/mbedtls/config_mynewt.h
@@ -292,6 +293,10 @@ extern "C" {
#undef MBEDTLS_ENTROPY_C
#endif

#if MYNEWT_VAL(MBEDTLS_ENTROPY_HARDWARE_ALT) == 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though not used now, keeping this for completeness.

utzig
utzig reviewed Feb 25, 2020
View reviewed changes
hw/drivers/trng/src/mbed_entropy_alt.c Outdated
*/

#include <trng/trng.h>
#include "mbedtls/config_mynewt.h"
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed? Maybe you could just use "os/mynewt.h" now?

@nkaje nkaje force-pushed the mbedtls_aes branch 2 times, most recently from 5078c02 to 9f4981d Compare February 25, 2020 20:46
@utzig
Copy link
Member

utzig commented Feb 25, 2020

Looks good, If it passes CI I will merge.

@nkaje
crypto/mbedtls: add hardware entropy config hooks
78e2529 
Hardware entropy config hooks added so that it can be
enabled by the application.

Signed-off-by: Naveen Kaje <naveen.kaje@juul.com>
@apache-mynewt-bot
Copy link

Style check summary

No suggestions at this time!

@utzig utzig merged commit 8c071c6 into apache:master Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasjer kasjer kasjer left review comments

@utzig utzig utzig approved these changes

@agross-korg agross-korg agross-korg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nkaje @utzig @apache-mynewt-bot @kasjer @agross-korg