[NRF52840]: TRNG support #4056

nvlsianpu · 2017-03-28T17:25:26Z

Description

TRNG hal support for nRF52840 using nordic's TRNG peripheral.

0xc0170 · 2017-03-28T17:49:13Z

targets/TARGET_NORDIC/TARGET_NRF5_SDK13/trng_api.c

+    uint8_t bytes_available;
+    nrf_drv_rng_bytes_available(&bytes_available);
+    if ((bytes_available < length) || (nrf_drv_rng_rand(output, length) == NRF_ERROR_NOT_FOUND))
+    {


codestyle nit ,

if (condition) { }

0xc0170 · 2017-03-28T17:49:51Z

How was this tested ? share test results pls if possible

bridadan · 2017-03-30T01:33:25Z

/morph test

mbed-bot · 2017-03-30T08:30:49Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1791

All builds and test passed!

nvlsianpu · 2017-03-30T08:46:44Z

@0xc0170 Output from mbed TLS Authenticated Encryption example:
plaintext message: 536f6d65207468696e67732061726520626574746572206c65667420756e7265616400
ciphertext: 35f5665e10490f8d2c0f876a19dd521a7e40002ed8082cb05c755990e04354fd705ad86246f37b8dfcb5b64a99cba6576dcd724717279a98ac518d
decrypted: 536f6d65207468696e67732061726520626574746572206c65667420756e7265616400

DONE

plaintext message: 536f6d65207468696e67732061726520626574746572206c65667420756e7265616400
ciphertext: 2b6a4fff10260a627969b2c5de906c296f6e61661f15eb1690b89b77df3d1c1147805894f16686ed4370f5c12ecf93d34b75d5602e8176bdff2650
decrypted: 536f6d65207468696e67732061726520626574746572206c65667420756e7265616400

DONE

yanesca · 2017-03-31T14:14:06Z

targets/TARGET_NORDIC/TARGET_NRF5_SDK13/trng_api.c

+
+    nrf_drv_rng_bytes_available(&bytes_available);
+
+    if ((bytes_available < length) || (nrf_drv_rng_rand(output, length) == NRF_ERROR_NOT_FOUND)) {


It is not necessary to return length bytes, it is perfectly fine to return with less. The length input variable only indicate the length of the output buffer.

I disagree. There is the indirect limit for the time of providing all of requested bytes for entropy module in mbedtls -> entropy.c, entropy.h. Nordic TRNG module is too slow and therefore it return 0 bytes to many times (this will get worse if nordic BLE stack is runing) for this condition.

Sorry, I am not sure if I understand. I am just saying that if this function returns with some nonzero but less than length bytes, then it might be enough, mbed TLS won't poll until it gets length bytes of random data. The amount of bytes it needs is usually less.

It was my more generic remark for mbed-os/mbedtls entropy implementation. The code in this PR didn't present this case.

0xc0170 · 2017-04-05T08:53:04Z

@yanesca Pleas review again

0xc0170 · 2017-04-05T08:53:25Z

/morph test

kl-cruz · 2017-04-05T09:24:02Z

targets/TARGET_NORDIC/TARGET_NRF5_SDK13/trng_api.c

@@ -42,29 +42,47 @@

 void trng_init(trng_t *obj)
 {
+    (void) obj;
+
    (void)nrf_drv_rng_init(NULL);


NULL value passed in config pointer causes that rng peripheral will be configured without bias correction. Maybe we should consider to enable it to be sure about statistically uniform distribution of the random values? Of course enabling this function causes slow down generation of random numbers.

Nice hint. Discussed out-of -band with @kl-cruz . We will keep this as is for this PR.

mbed-bot · 2017-04-05T12:39:04Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1835

All builds and test passed!

0xc0170 · 2017-04-05T16:30:51Z

@yanesca Pleas review again

Happy with the changes?

0xc0170 · 2017-04-10T11:06:44Z

@yanesca @RonEld bump

sg- · 2017-04-10T16:45:54Z

@nvlsianpu Can you resolve the conflict?

improve coding style.

nvlsianpu · 2017-04-11T06:08:31Z

I rebased it onto mbed-os-workshop-17q2.

RonEld

I added one small comment to verify with experts.
Assuming nrf_drv_rng_rand results in enough entropy, I approve this, but I would like @yanesca to review as well

RonEld · 2017-04-12T07:51:28Z

targets/TARGET_NORDIC/TARGET_NRF5_SDK13/trng_api.c

+
+    if (bytes_available == 0) {
+        nrf_drv_rng_block_rand(output, 1);
+        *output_length = 1;


If I understand correct, this means gathering total one byte. This is not enough entropy.
the mbedtls_entropy_func will try to gather more later, so I am less worried about the threshold ( on expense of performance), I am more concerned about whether collecting one byte every time will result in enough entropy, considering that there is a trng_init and trng_free between every collection. Please verify with your experts

@RonEld
I consulted this doubt. There ain't obstacles to using Nordic TRNG According to mentioned algorithm:

The RNG module produces 1 byte of random data per event. It is not correlated with previous data, so the entropy is not affected by the number of bytes gathered. However, it will obviously take more time to gather more bytes.

theotherjimmy · 2017-04-24T15:40:44Z

bump @RonEld Are you happy with this now?

RonEld · 2017-04-25T08:17:51Z

@theotherjimmy I have already approved this from my side. I only wanted my question answered, for verification, and I am happy with the answer. Keep in mind that I am not the mbed TLS gate keeper though

andresag01

The TRNG implementation seems to be OK. I have the following questions:

What are the requirements of this implementation regarding thread safety?
What is the amount of entropy returned?
If we want to actually initialise the device in the trng_init() function, I think we need to add a return code to that function as it is currently not easy to check whether the initialisation call was successful or not.

andresag01 · 2017-05-24T10:40:20Z

targets/TARGET_NORDIC/TARGET_NRF5_SDK13/trng_api.c

+{
+    (void) obj;
+
+    (void)nrf_drv_rng_init(NULL);


It would be nice to check the return code for this function. The function does not have a return code though....

0xc0170 · 2017-05-24T14:53:37Z

What are the requirements of this implementation regarding thread safety?

HAL is not thread safe, thus a consumer needs to provide it

andresag01 · 2017-05-24T14:56:30Z

@0xc0170: Thanks for the information, I think that slightly conflicts with the mbed TLS policy.

0xc0170 · 2017-05-24T20:32:43Z

@0xc0170: Thanks for the information, I think that slightly conflicts with the mbed TLS policy.

please verify that. we got mbed wrapper in mbedtls that should take care of it ! If not, we shall fix it

andresag01 · 2017-05-25T10:13:03Z

Well, as it says here:

most functions use an explicit context. Most of the time, as long as this context is not shared between threads, you're safe. However, be aware that sometimes a context can be share indirectly, for example an SSL context can point to an RSA context (our private key).

Also, the mbed TLS wrapper around the TRNG API is here and its usage here is not guarded. However, this is for mbed TLS, I am not sure what the policy is when integrating with mbed OS.

gilles-peskine-arm

Looks fine to me

yanesca · 2017-06-01T11:08:41Z

Looks good to me

0xc0170 reviewed Mar 28, 2017

View reviewed changes

0xc0170 added the needs: review label Mar 28, 2017

sg- added needs: work and removed needs: review labels Mar 28, 2017

0xc0170 removed the needs: work label Mar 29, 2017

0xc0170 requested review from yanesca and simonbutcher March 29, 2017 11:44

0xc0170 added the needs: review label Mar 29, 2017

sg- added needs: CI and removed needs: review labels Mar 29, 2017

0xc0170 added ready for merge and removed needs: CI labels Mar 30, 2017

yanesca reviewed Mar 31, 2017

View reviewed changes

0xc0170 added needs: review and removed ready for merge labels Apr 5, 2017

kl-cruz reviewed Apr 5, 2017

View reviewed changes

kl-cruz and others added 4 commits April 11, 2017 08:04

[nRF52840] added trng implementation for nrf52840

83fa2bc

[nRF52840] Fixed length in trng in every mode

056648f

[NRF52840]: fix non-blocking mode of TRNG driver

9c33e93

improve coding style.

[NRF5840] change: trng_get_bytes returns at last one random byte.

9955701

nvlsianpu force-pushed the mbed-os-workshop-17q2-nordic-trng branch from aeeb02e to 9955701 Compare April 11, 2017 06:07

RonEld approved these changes Apr 12, 2017

View reviewed changes

0xc0170 removed the needs: review label Apr 25, 2017

0xc0170 merged commit 870aa34 into ARMmbed:mbed-os-workshop-17q2 Apr 25, 2017

andresag01 reviewed May 24, 2017

View reviewed changes

gilles-peskine-arm reviewed May 29, 2017

View reviewed changes

c1728p9 mentioned this pull request Jun 7, 2017

NRF52840: TRNG support #4495

Merged


		nrf_drv_rng_bytes_available(&bytes_available);

		if ((bytes_available < length) \|\| (nrf_drv_rng_rand(output, length) == NRF_ERROR_NOT_FOUND)) {

[NRF52840]: TRNG support #4056

[NRF52840]: TRNG support #4056

Uh oh!

Conversation

nvlsianpu commented Mar 28, 2017

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Mar 28, 2017

Uh oh!

bridadan commented Mar 30, 2017

Uh oh!

mbed-bot commented Mar 30, 2017

Output

Uh oh!

nvlsianpu commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 5, 2017

Uh oh!

0xc0170 commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbed-bot commented Apr 5, 2017

Output

Uh oh!

0xc0170 commented Apr 5, 2017

Uh oh!

0xc0170 commented Apr 10, 2017

Uh oh!

sg- commented Apr 10, 2017

Uh oh!

nvlsianpu commented Apr 11, 2017

Uh oh!

RonEld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvlsianpu Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Apr 24, 2017

Uh oh!

RonEld commented Apr 25, 2017

Uh oh!

andresag01 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented May 24, 2017

Uh oh!

andresag01 commented May 24, 2017

Uh oh!

0xc0170 commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andresag01 commented May 25, 2017

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

yanesca commented Jun 1, 2017

Uh oh!

nvlsianpu Apr 20, 2017 •

edited

Loading

andresag01 left a comment •

edited

Loading

0xc0170 commented May 24, 2017 •

edited

Loading