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

Add TRNG for NRF52832 #6116

Merged
merged 1 commit into from Feb 26, 2018

Conversation

Projects
None yet
10 participants
@marcuschangarm
Contributor

marcuschangarm commented Feb 16, 2018

Use SoftDevice API to get random numbers when present and active,
otherwise read random numbers directly from TRNG peripheral.

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Feb 16, 2018

ARM Internal Ref: IOTSSL-2100

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:fix_trng_nrf52 branch 3 times, most recently from 5347b45 to 56e7227 Feb 16, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 21, 2018

Can you please resolve the conflicts?

@yogpan01

This comment has been minimized.

Contributor

yogpan01 commented Feb 21, 2018

@marcuschangarm This PR may require rebase.

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Feb 21, 2018

@marcuschangarm I can confirm that this works on my NRF52_DK after rebasing it with current master (to get the SWO support) and resolving the conflict on targets.json.

*@}
**/
#ifndef SUPPRESS_INLINE_IMPLEMENTATION

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

What is the reason for this macro? Where are the functions defined if it's set?

{
*((volatile uint32_t *)((uint8_t *)NRF_RNG + rng_event)) = NRF_RNG_EVENT_CLEAR;
#if __CORTEX_M == 0x04
volatile uint32_t dummy = *((volatile uint32_t *)((uint8_t *)NRF_RNG + rng_event));

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

What's the significance of this dummy read?

void trng_free(trng_t *obj)
{
(void) obj;
nrf_drv_rng_uninit();

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

Why has this been removed even in case SD is enabled?

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 21, 2018

Contributor

There is no good way (at least in this version of the SDK) to keep track of SoftDevice state. Imagine a case where trng_init is called before the SoftDevice is enabled and trng_free is called after, then we would have an inconsistent pairing of init/uninit calls.

In this version the TRNG is kept running after the first call. The power consumption for that is orders of magnitude lower than the SoftDevice in sleep mode.

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

@marcuschangarm Ok, but with the present code, issuing multiple trng_init / trng_free calls leads to failure, which is not consistent with the TRNG API. What about calling nrf_drg_rng_uninit if only if trng_init == false -- i.e. if the TRNG has been initialized when SoftDevice was enabled -- and resetting it to trng_init = true? This way, multiple trng_init / trng_free calls would be fine, as long as it doesn't happen that the SoftDevice gets disabled in the middle of a trng_init / trng_free pair.

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 21, 2018

Contributor

issuing multiple trng_init / trng_free calls leads to failure

I don't understand, can you please elaborate? trng_init doesn't do anything after the first successful call and trng_free is empty.

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

@marcuschangarm There was a misunderstanding on my side, I agree that multiple trng_init calls with SoftDevice enabled are fine, with all but the first one being no-ops.

Regarding the case you described -- SoftDevice being enabled after trng_init was called: Can we use trng_init == false (and perhaps give it a more descriptive name, pointing out that it's about TRNG initialization in the presence of SoftDevice) instead of NRF_HAL_SD_IS_ENABLED() in trng_get_bytes to avoid using SoftDevice functionality without being sure that it is properly initialized?

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 21, 2018

Contributor

Can we use trng_init == false (and perhaps give it a more descriptive name, pointing out that it's about TRNG initialization in the presence of SoftDevice) instead of NRF_HAL_SD_IS_ENABLED() in trng_get_bytes to avoid using SoftDevice functionality without being sure that it is properly initialized?

The SoftDevice does its own TRNG initialization. So we might end up in a situation where we use the low-level TRNG API and steal random numbers from the SoftDevice.

How about moving nrf_drv_rng_init to trng_get_bytes instead?

Edit: I went ahead and moved the initialization call inside get bytes. This is symmetric to the non-driver, raw register get-bytes code path.

int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length)
{
    (void) obj;

    /* Use SDK RNG driver if SoftDevice is enabled. */
    if (NRF_HAL_SD_IS_ENABLED() ) {
        static bool nordic_driver_init = true;

        if (nordic_driver_init) {
            nordic_driver_init = false;
            nrf_drv_rng_init(NULL);
        }
#include "nrf_sdm.h"
#include "nrf_soc.h"
#else
#include "app_fifo.h"

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

Where can I find this file? It doesn't seem to be contained in the code base.

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

@marcuschangarm Is it intended that this file is missing?

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 21, 2018

Contributor

Ah, I see what you are saying. SOFTDEVICE_PRESENT must always be defined in the current setup. Removing it will break the code in many places. The ability to remove the SoftDevice will be introduced in the SDK upgrade we are working on.

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

@marcuschangarm Ok, thanks for clarifying this!

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

@marcuschangarm I would prefer not to have large parts of this PR depending on a configuration which is known to be broken, though. Wouldn't it make more sense to error out in a single place if SOFTDEVICE_PRESENT is not defined, and adding support for ! SOFTDEVICE_PRESENT in a single go in your SDK upgrade?

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 21, 2018

Contributor

Unfortunately, all files in the Nordic SDK have this built in bug! 😒

We are completely re-implementing most of the mbed HAL for the new SDK and reorganizing the directory structure so that we can both change SoftDevices on the fly and even compile without them. This PR is just to get the Cloud client going until we have the new SDK in place.

uint8_t softdevice_is_enabled;
result = sd_softdevice_is_enabled(&softdevice_is_enabled);
if (softdevice_is_enabled)

This comment has been minimized.

@hanno-arm

hanno-arm Feb 21, 2018

Contributor

It seems to be safer to only take this branch if sd_softdevice_is_enabled has been successful and softdevice_is_enabled is set.

This comment has been minimized.

@0xc0170

0xc0170 Feb 22, 2018

Member

Note this is 3rd party SDK thus should be sent upstream if needed

@hanno-arm

Requested clarification on some parts of the code.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Feb 21, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Feb 21, 2018

@hanno-arm Thank you for the review!

Everything in the TARGET_SDK11 directory is copied directly from Nordic's SDK and traditionally we don't make modifications unless we are fixing bugs.

Also, this is a temporary implementation until the new SDK upgrade arrives.

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:fix_trng_nrf52 branch 2 times, most recently from 331c0ef to cd9c587 Feb 21, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Feb 21, 2018

@yogpan01 @0xc0170 @TeroJaasko

Rebased!

Add TRNG for NRF52832
Use SoftDevice API to get random numbers when present and active,
otherwise read random numbers directly from TRNG peripheral.

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:fix_trng_nrf52 branch from cd9c587 to 9439c19 Feb 21, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 22, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 22, 2018

Build : SUCCESS

Build number : 1212
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6116/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Feb 22, 2018

@marcuschangarm the latest version still works with Cloud client.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 22, 2018

The review should be only for file (targets/TARGET_NORDIC/TARGET_NRF5/trng_api.c), the rest of files in this PR are coming from 3rd party library (still can be reviewed but should not be changed if it is not a blocker).

/** Deinitialize the TRNG peripheral
*
* @param obj The TRNG object
*/
void trng_free(trng_t *obj)
{
(void) obj;

This comment has been minimized.

@hanno-arm

hanno-arm Feb 22, 2018

Contributor

@marcuschangarm Apologies if this is repeating something we already touched yesterday, but the following is still not clear to me: Why aren't we initializing the TRNG based on the SoftDevice state at the time of the trng_init call, saving this initialization state somewhere within the trng_t struct or in a global variable, and handling trng_get_bytes and trng_init based on it (in particular, not checking whether SoftDevice has been enabled in the meantime). This way, we could correctly setup the SoftDevice / low-level TRNG drivers in trng_init via nrf_drv_rng_init(NULL) resp. nrf_rng_task_trigger(NRF_RNG_TASK_START);, and free them via nrf_drv_rng_uninit resp. nrf_rng_task_trigger(NRF_RNG_TASK_STOP); in trng_free. Is this unacceptable because we cannot guarantee that SoftDevice is enabled before trng_init is called, and we definitely want to use SoftDevice is possible?

The SoftDevice does its own TRNG initialization. So we might end up in a situation where we use the low-level TRNG API and steal random numbers from the SoftDevice.

I didn't yet understand how that could happen in the above approach.

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 22, 2018

Contributor

The user application can enable and disable the SoftDevice at any time. Usually this would be done to save power. On the other hand, the TRNG might be called from different parts of the application at times that are not in lock step with the SoftDevice. So the code must be able to handle init and free calls that are not in sync with the SoftDevice state.

The SoftDevice does its own TRNG initialization. So we might end up in a situation where we use the low-level TRNG API and steal random numbers from the SoftDevice.

I believe the SoftDevice is copying the TRNG byte to a buffer on each interrupt. If we read that byte using the low level API before the SoftDevice gets to read it, weird things might happen.

This comment has been minimized.

@hanno-arm

hanno-arm Feb 22, 2018

Contributor

@marcuschangarm

I believe the SoftDevice is copying the TRNG byte to a buffer on each interrupt. If we read that byte using the low level API before the SoftDevice gets to read it, weird things might happen.

Ah, ok, so this might happen even if the SoftDevice's TRNG driver API is not used?

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 22, 2018

Contributor

so this might happen even if the SoftDevice's TRNG driver API is not used?

Sorry, I don't understand the question.

This comment has been minimized.

@hanno-arm

hanno-arm Feb 22, 2018

Contributor

I understood you as saying that when SoftDevice is active, the TRNG byte is automatically read by the SoftDevice driver code when ready, even if there is no code explicitly requesting random data through nrf_drv_rng_rand - is that right? So if trng_init would be called before SoftDevice is initialized, then my suggestion for trng_get_bytes would lead to random data being read from the low-level driver that has potentially already been automatically processed by the SoftDevice driver code, even though trng_api.c didn't request that.

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 22, 2018

Contributor

when SoftDevice is active, the TRNG byte is automatically read by the SoftDevice driver code when ready, even if there is no code explicitly requesting random data through nrf_drv_rng_rand - is that right?

Yes.

So if trng_init would be called before SoftDevice is initialized, then my suggestion for trng_get_bytes would lead to random data being read from the low-level driver that has potentially already been automatically processed by the SoftDevice driver code, even though trng_api.c didn't request that.

Yes, that is my concern.

This comment has been minimized.

@hanno-arm

hanno-arm Feb 22, 2018

Contributor

@marcuschangarm Thanks for confirming, then I think your solution is the best one. 👍

@mbed-ci

This comment has been minimized.

@hanno-arm

I don't like that this PR introduces code under ! SOFTDEVICE_PRESENT which is known to be broken.

Leaving that side, I am ok with how trng_api.c multiplexes between the NRF driver implementation in case SoftDevice is present + active, and a hand-crafted direct driver implementation.
As mentioned in the comments, though, I would prefer to do initialization and cleanup of the native driver resp. the SoftDevice driver in trng_init and trng_free, respectively, which is what these functions are intended for.

Given the importance of the PR and the fact that it looks fine from the perspective of functionality and security, the above concerns are no blockers, and I'm ok approving after @yanesca has given his opinion.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Feb 22, 2018

@hanno-arm I completely understand where you are coming from! It's an imperfect solution to a bad situation. I would also be uncomfortable adding files that I can't test because the SOFTDEVICE_PRESENT flag isn't working.

For reference, this is how the new implementation in SDK 14.2 looks like: https://github.com/ARMmbed/mbed-os/blob/feature-nrf528xx/targets/TARGET_NORDIC/TARGET_NRF5x/trng_api.c#L49-L79

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Feb 22, 2018

/morph test

@yanesca

I have the same reservations as @hanno-arm, and although I am approving it on account of the deadline, I would like to emphasise that this approval is for the part of the code that does not use the SDK.

@hanno-arm

Approving, reinforcing @yanesca's remarks.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 22, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: work labels Feb 22, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 22, 2018

Build : SUCCESS

Build number : 1223
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6116/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Feb 23, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 23, 2018

Waiting for the second Test: SUCCESS to come in. Was pointed out that I jumped the gun in issuing the morph build command, when the morph test queue was really long.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Feb 26, 2018

Waiting for the second Test: SUCCESS to come in. Was pointed out that I jumped the gun in issuing the morph build command, when the morph test queue was really long.

we had aborted first morph test #6116 (comment) since a new build #6116 (comment) was triggered.

@cmonr cmonr merged commit c9cefcc into ARMmbed:master Feb 26, 2018

18 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mbed-ci-generic Build finished.
Details
travis-ci/docs/ Local docs testing has passed
Details
travis-ci/events/ Local events testing has passed
Details
travis-ci/littlefs/ Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL/ Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM/ Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC/ Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON/ Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP/ Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-SILICON_LABS/ Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM/ Local mbed2-STM testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 26, 2018

@marcuschangarm marcuschangarm deleted the marcuschangarm:fix_trng_nrf52 branch Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment