[Silicon Labs] Add TRNG support #4051

stevew817 · 2017-03-28T11:44:41Z

Description

Adding support for the TRNG peripheral present on Series 1 Configuration 2 devices.

Status

READY

Migrations

NO

0xc0170 · 2017-03-28T11:49:26Z

/morph test

mbed-bot · 2017-03-28T16:41:44Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1759

All builds and test passed!

0xc0170 · 2017-03-28T17:50:58Z

How was this tested? Can you share any tests you run?

stevew817 · 2017-03-28T20:21:22Z

@0xc0170 mbedTLS authcrypt example, two resets:
plaintext message: 536f6d65207468696e67732061726520626574746572206c65667420756e7265616400
ciphertext: 4ca1015c1eaea91a446c6746827766dc3ea0b154a82a377528b5f4862b13422cf89f22d02736c789930be51ab6f6acf83069209f6306368365bcce
decrypted: 536f6d65207468696e67732061726520626574746572206c65667420756e7265616400

DONE

plaintext message: 536f6d65207468696e67732061726520626574746572206c65667420756e7265616400
ciphertext: b31cefd3511b903a325e5d0a69b6b5b1c91c3841fb2548a211e1eb2306c05df2e2bdd0360e5519e90f02f5b73d21a36a5f9af3bbc6035e961772dc
decrypted: 536f6d65207468696e67732061726520626574746572206c65667420756e7265616400

DONE

sg- · 2017-03-28T21:17:40Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/trng_api.c

+void trng_init(trng_t *obj)
+{
+  /* Use TRNG0 for all HW entropy collection */
+  obj->instance = TRNG0;


Should be 4 spaces, not 2 https://docs.mbed.com/docs/mbed-os-handbook/en/latest/cont/code_style/

bridadan · 2017-03-29T01:31:25Z

/morph test

mbed-bot · 2017-03-29T08:26:06Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1774

All builds and test passed!

Adding support for the TRNG peripheral present on Series 1 Configuration 2 devices.

stevew817 · 2017-03-30T00:27:18Z

@sg- rebased and merge conflict resolved

0xc0170 · 2017-03-30T08:27:02Z

/morph test

mbed-bot · 2017-03-30T12:27:28Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1793

All builds and test passed!

stevew817 · 2017-04-03T08:40:02Z

@0xc0170 retest uvisor ?

0xc0170 · 2017-04-03T12:03:22Z

retest uvisor

0xc0170 · 2017-04-03T14:25:45Z

@yanesca @sbutcher-arm @RonEld review pls

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

@yanesca @sbutcher-arm @RonEld review pls

update?

RonEld

I have put some comments, waiting for answers before I can approve

RonEld · 2017-04-06T05:49:35Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.c

+ */
+#include "sl_trng.h"
+
+#if defined(TRNG_PRESENT)


why not use the existing DEVICE_TRNG define?

TRNG_PRESENT comes from our own device headers, and is the authority for knowing whether a device has TRNG or not. DEVICE_TRNG is an mbed define to use the mbed TRNG HAL. Considering sl_trng.c is a Silicon Labs file, and the mbed HAL doesn't directly interface with it (that'd be trng_api.c), we are using Silicon Labs defines.

RonEld · 2017-04-06T06:00:00Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.c

+    return 0;
+}
+
+int sl_trng_check_entropy( TRNG_TypeDef *device )


where is this API being called?
The entropy check should be wither done in you trng_get_bytes function, to verify that the collected noise has good entropy, or later from the entropy pool (TBD)

@RonEld This is a self-test function (see my other comment), and is only used during TRNG bringup. The TRNG peripheral has built-in entropy checks (called 'Noise Alarms') which are being checked in the polling function.

Should I get rid of this function for the driver that we ship with mbed, or can I leave it as-is?

see previous comment, but I think this function has less impact than previous test funciton

RonEld · 2017-04-06T06:02:42Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.c

+    return 0;
+}
+
+int sl_trng_check_conditioning( TRNG_TypeDef *device )


where is this API being called?
If I understand this API correct, you are now supplying an API for setting a constant output. am I right?
@yanesca I would like to hear your input on this

@RonEld No, this is a self-test function. It should never be used in regular operation, which is why it is never being called from the driver. We use it for internal test purposes, and since this driver was copied verbatim, the test function ended up here.
Should I get rid of it instead?

In my opinion, internal test functions should not be added, in addition that they also might be exploited for other reasons

Okay, I'll remove both test functions.

@RonEld removed now.

RonEld · 2017-04-06T07:03:52Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.c

+#if !defined(SL_TRNG_IGNORE_ALL_ALARMS)
+        /* Check status for current data in FIFO
+         * and handle any error conditions. */
+        ret = sl_trng_check_status( device );


Is the check status an irrecoverable failure? Perhaps wait for an interrupt until check_status will succeed?

Well, on a noise alarm you really should throw away the generated data and reset the TRNG (which is already happening in check_status). Thus, the polling 'failed'. IMO, this should bubble up to the user, who can then decide whether or not to retry.
The other statuses are irrecoverable in the sense that they point to a larger issue, most likely someone tampering with the TRNG ring oscillators.

Could you point me to the location where you poll on fail? It seems to me that on failure, you stop collecting and return an error to the user

Changed now. Upon noise alarm, we throw away the collected entropy and continue collecting. For the other alarms (which are more severe, see my previous comment), we throw away the collected entropy, and return an error.

RonEld · 2017-04-06T07:32:59Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.c

+        if (ret != 0)
+        {
+            output_len = 0;
+            break;


should this be changed to continue? Do you want to continue collecting?

See my other comment.

… alarm.

RonEld

I would remove dead code such as the test vectors that are not used anymore.
Other than that, it's ok by me

andresag01

The code seems OK, but to me it looks like there are some limitations because the implementation of mbedtls_entropy_hardware_poll() continually resets the TRNG device. This PR is trying to get around the problem, but there are other issues, such as not being able to turn off the TRNG device.

andresag01 · 2017-05-24T15:03:25Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/trng_api.c

+void trng_free(trng_t *obj)
+{
+    /* Don't turn off the TRNG to avoid clearing its FIFO */
+    (void) obj;


Does this mean that an application can never really turn off the TRNG?

andresag01 · 2017-05-24T15:15:42Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/trng_api.c

+
+    if(!is_trng_enabled) {
+        sl_trng_init(obj->instance);
+        is_trng_enabled = true;


If I understand correctly, the reasoning behind this is_trng_enabled flag is to avoid restarting the TRNG device every time there is a call to mbedtls_hardware_poll() to improve performance. The main problem is that mbedtls_hardware_poll() is setup to start and shutdown the TRNG on every call, which seems a little inefficient. @gilles-peskine-arm: Any thoughts?

Our Hardware TRNG is setup to fill a 2K FIFO with entropy when it starts, and only start up the ring oscillators again when it needs to replenish entropy. Therefore, actually shutting down the TRNG peripheral is counter-productive, since you'd be generating 2K of entropy (plus peripheral start-up time) on each request for entropy...

yanesca · 2017-05-26T14:00:53Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.c

+            break;
+        }
+
+#if !defined(SL_TRNG_IGNORE_ALL_ALARMS)


Could somebody please help me understand why we need these alarm suppression options? (I mean SL_TRNG_IGNORE_ALL_ALARMS and SL_TRNG_IGNORE_NOISE_ALARMS)

gilles-peskine-arm

I have a few observations which may be due to my lack of familiarity with the hardware.

gilles-peskine-arm · 2017-05-29T09:44:22Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.h

+ * \return
+ *   0 if success. Error code if failure.
+ */
+int sl_trng_set_key( TRNG_TypeDef *ctx, const unsigned char *key );


Where is the conditioning key set? Should it be?

gilles-peskine-arm · 2017-05-29T09:49:12Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.c

+    return 0;
+}
+
+static int sl_trng_check_status( TRNG_TypeDef *device )


This function changes the device state. A name like check_status does not convey this. How about sl_trng_retrieve_alarms?

gilles-peskine-arm · 2017-05-29T09:50:07Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.c

+        return 0;
+    }
+
+    if ( status & TRNG_STATUS_PREIF )


If the preliminary noise alarm is raised, other alarms are ignored. Is this desirable? Or is the point moot because the alarms are mutually exclusive?

gilles-peskine-arm · 2017-05-29T09:52:37Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/trng/sl_trng.c

+             (ret == SL_TRNG_ERR_NOISE_ALARM) )
+        {
+            ret = 0;
+            continue;


Like yanesca I wonder why there's a compilation option to ignore alarms. Assuming that the mode SL_TRNG_IGNORE_NOISE_ALARMS is useful, I don't understand the logic here. If there is a physical condition that causes the alarm to keep being raised, wouldn't this cause a tight loop?

0xc0170 requested review from yanesca and simonbutcher March 28, 2017 11:46

0xc0170 added the ready for merge label Mar 28, 2017

sg- added needs: CI and removed ready for merge labels Mar 28, 2017

sg- suggested changes Mar 28, 2017

View reviewed changes

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

sg- approved these changes Mar 28, 2017

View reviewed changes

bridadan added needs: CI needs: review and removed needs: work labels Mar 29, 2017

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

stevew817 added 3 commits March 30, 2017 02:25

[Silicon Labs] Add TRNG support

285f82f

Adding support for the TRNG peripheral present on Series 1 Configuration 2 devices.

Remove leftover comment about mbedTLS plugin

88c996e

Update to 4 spaces/tab

f8be297

stevew817 force-pushed the integration/trng branch from d48e6ee to f8be297 Compare March 30, 2017 00:26

0xc0170 approved these changes Mar 30, 2017

View reviewed changes

0xc0170 added the ready for merge label Apr 3, 2017

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

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

RonEld suggested changes Apr 6, 2017

View reviewed changes

RonEld reviewed Apr 6, 2017

View reviewed changes

stevew817 added 2 commits April 6, 2017 10:02

Remove internal test functions

d2020f5

Add behavior to throw away collected entropy on occurrence of a noise…

4675d05

… alarm.

RonEld approved these changes Apr 6, 2017

View reviewed changes

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

sg- merged commit 1b2aa9d into ARMmbed:mbed-os-workshop-17q2 Apr 6, 2017

sg- removed the ready for merge label Apr 6, 2017

andresag01 reviewed May 24, 2017

View reviewed changes

yanesca reviewed May 26, 2017

View reviewed changes

gilles-peskine-arm reviewed May 29, 2017

View reviewed changes

c1728p9 mentioned this pull request Jun 7, 2017

Silicon Labs: Add TRNG support #4497

Merged

[Silicon Labs] Add TRNG support #4051

[Silicon Labs] Add TRNG support #4051

Uh oh!

Conversation

stevew817 commented Mar 28, 2017

Description

Status

Migrations

Uh oh!

0xc0170 commented Mar 28, 2017

Uh oh!

mbed-bot commented Mar 28, 2017

Output

Uh oh!

0xc0170 commented Mar 28, 2017

Uh oh!

stevew817 commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridadan commented Mar 29, 2017

Uh oh!

mbed-bot commented Mar 29, 2017

Output

Uh oh!

stevew817 commented Mar 30, 2017

Uh oh!

0xc0170 commented Mar 30, 2017

Uh oh!

mbed-bot commented Mar 30, 2017

Output

Uh oh!

stevew817 commented Apr 3, 2017

Uh oh!

0xc0170 commented Apr 3, 2017

Uh oh!

0xc0170 commented Apr 3, 2017

Uh oh!

0xc0170 commented Apr 5, 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!

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!

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!

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!

RonEld Apr 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevew817 commented Mar 28, 2017 •

edited

Loading

RonEld Apr 6, 2017 •

edited

Loading