Nuvoton: Re-implement TRNG HAL with TRNG H/W #11650

ccli8 · 2019-10-08T09:42:46Z

Description

This PR is conclusion of #11176 (comment) and tries to re-implement TRNG HAL with TRNG H/W on targets which support it.

Support targets:

NU_PFM_M2351_*
NUMAKER_IOT_M263A

@cyliangtw

Related PR

#11176 (comment)

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@kjbracey-arm @yanesca

ciarmcom · 2019-10-08T11:00:35Z

@ccli8, thank you for your changes.
@yanesca @kjbracey-arm @Ronny-Liu @ARMmbed/mbed-os-maintainers please review.

bulislaw

@Patater please review

kjbracey · 2019-10-15T08:34:16Z

targets/TARGET_NUVOTON/TARGET_M2351/trng_api.cpp

+                   "TRNG initialization counter would overflow");
+    }
+    core_util_atomic_incr_u16(&trng_init_counter, 1);
+    if (trng_init_counter == 1) {


Re-reading the counter isn't atomic - you need to look at the return value of the increment function.

Suggested change

if (trng_init_counter == 1) {

if (core_util_atomic_incr_u16(&trng_init_counter, 1) == 1) {

Although here you're inside a critical section anyway, meaning you don't need the atomic increment at all, you can just do ++trng_init_counter.

kjbracey · 2019-10-15T09:02:09Z

targets/TARGET_NUVOTON/TARGET_M2351/device/StdDriver/m2351_crypto.c

+    switch(id)
+    {
+        case CURVE_P_192:
+            p32[  0] = 0x00000000;


Is this really a code-size win over just memcpying from the const ECC_CURVE table?

Even if it is, I suspect you could get an equivalent saving by just changing the char Ea[144] etc members to const char *Ea, which removes the need to store zero-padding.

(Beyond that, why store in hex rather than binary anyway?)

The above code is for XOM (execute-only ROM). On mbed-os, it is disabled (XOM_SUPPORT == 0).

Beyond that, why store in hex rather than binary anyway?

The curve definition source is in hex form. No further conversion.

The above code is for XOM (execute-only ROM).

Okay, not relevant here, but a bit puzzled. Maybe there's something I'm missing.

If you're having problems with not being able to access const data in execute-only ROM, there's something wrong with your memory map and/or linker/compiler setup. Execute-only ROM shouldn't be breaking basic concepts like being able to access const data objects.

If really necessary in some environment, the build should be placing read-only data regions in RAM via the linker map.

Edit: oh, is that the point? You're having to resort to putting const data objects in RAM, which makes the other form work, but use a lot of RAM? So this form forces ROM use via code instead, even if clunky?

@kjbracey-arm Why const data not place in normal ROM? This is security consideration. The major purpose of XOM is non-readable. If XOM code references const data in another ROM, compromising the ROM means compromising the XOM indirectly.

Anyway, XOM is not used on mbed-os, so just ignore it.

ccli8 · 2019-10-16T02:06:54Z

Make modifications:

Do rebase
For M2351, update default secure image/gateway library due to change of TRNG security attribute
Minor fix with trng_init_counter

yanesca · 2019-10-16T08:24:25Z

I have searched for some documentation and these were the best I could find:
http://www.nuvoton.com/resource-files/DS_M2351_Series_EN_Rev1.00.pdf
http://www.nuvoton.com/resource-files/UM_NuMaker-M263KI_EN_Rev1.00.pdf

These have very limited information on the TRNGs in these targets. Where can I find more information on these TRNGs?

ccli8 · 2019-10-16T08:51:23Z

These have very limited information on the TRNGs in these targets. Where can I find more information on these TRNGs?

@yanesca Sorry about that. Detailed spec is not open. However, you can find register definition in:

mbed-os/targets/TARGET_NUVOTON/TARGET_M2351/device/Reg/trng_reg.h

Line 12 in 1960671

    
           /*---------------------- True Random Number Generator -------------------------*/

mbed-os/targets/TARGET_NUVOTON/TARGET_M261/device/Reg/trng_reg.h

Line 45 in 1960671

typedef struct

yanesca

LGTM

0xc0170

One question otherwise looks fine to me

0xc0170 · 2019-10-23T14:16:20Z

targets/TARGET_NUVOTON/TARGET_M261/trng_api.cpp

+static uint16_t trng_init_counter = 0U;
+
+/* Mutex for synchronizing access to TRNG H/W */
+static SingletonPtr<PlatformMutex> trng_mutex;


is this needed? Because the upper layer takes care of this - HAL does not by default.

I assume this TRNG is just one peripheral so this is not taking care of sync in HAL layer when there are multiple users of this peripheral, there's just one trng hal? Or this is connected to secure/non secure thus this lock is needed.

@0xc0170 Yes. It is unnecessary. mbedtls_mutex has done. Removed it.

Re-implement __PC() by replacing BSP assembly with toolchain built-in.

Related targets: - NU_PFM_M2351_* - NUMAKER_IOT_M263A

ccli8 · 2019-10-24T02:41:35Z

Make modifications:

Do rebase
Remove unnecessary mutex. mbedtls_mutex has done for it.

Targets supporting TRNG H/W: - NU_PFM_M2351_* - NUMAKER_IOT_M263A

Update for change of TRNG security attribute

0xc0170 · 2019-10-24T07:17:03Z

CI started

mbed-ci · 2019-10-24T08:28:26Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

ciarmcom requested review from kjbracey, Ronny-Liu, yanesca and a team October 8, 2019 11:00

ciarmcom added the needs: review label Oct 8, 2019

bulislaw reviewed Oct 9, 2019

View reviewed changes

kjbracey reviewed Oct 15, 2019

View reviewed changes

ccli8 force-pushed the nuvoton_trng-hal-with-trng-hw branch from 6800f29 to cd63c09 Compare October 16, 2019 01:55

yanesca approved these changes Oct 16, 2019

View reviewed changes

This was referenced Oct 21, 2019

M2351: Support TFM level 1 #10959

Merged

Support entropy source for targets without TRNG H/W #11680

Closed

0xc0170 approved these changes Oct 23, 2019

View reviewed changes

ccli8 added 4 commits October 24, 2019 09:36

M2351: Update BSP/crypto driver

c326e07

M2351: Change TRNG security attribute to secure

3548d38

NUVOTON: Re-implement __PC() with toolchain built-in

d993c5a

Re-implement __PC() by replacing BSP assembly with toolchain built-in.

NUVOTON: Fix BSP/MKROM header

3f9ba9e

Related targets: - NU_PFM_M2351_* - NUMAKER_IOT_M263A

ccli8 force-pushed the nuvoton_trng-hal-with-trng-hw branch from cd63c09 to 0bb8034 Compare October 24, 2019 02:36

ccli8 added 2 commits October 24, 2019 10:55

NUVOTON: Re-implement TRNG HAL with TRNG H/W

4cd0332

Targets supporting TRNG H/W: - NU_PFM_M2351_* - NUMAKER_IOT_M263A

M2351: Update default secure image/gateway library

8161386

Update for change of TRNG security attribute

ccli8 force-pushed the nuvoton_trng-hal-with-trng-hw branch from 0bb8034 to 8161386 Compare October 24, 2019 03:44

0xc0170 added needs: CI and removed needs: review labels Oct 24, 2019

0xc0170 added the ready for merge label Oct 24, 2019

0xc0170 added the release-version: 5.14.2 label Oct 24, 2019

0xc0170 merged commit 19fc2b1 into ARMmbed:master Oct 24, 2019

0xc0170 removed needs: CI ready for merge labels Oct 24, 2019

ccli8 deleted the nuvoton_trng-hal-with-trng-hw branch October 24, 2019 08:34

ccli8 mentioned this pull request Oct 24, 2019

M2351: Pre-build secure image/lib to favor pelion application #11594

Merged

	if (trng_init_counter == 1) {
	if (core_util_atomic_incr_u16(&trng_init_counter, 1) == 1) {

Nuvoton: Re-implement TRNG HAL with TRNG H/W #11650

Nuvoton: Re-implement TRNG HAL with TRNG H/W #11650

Uh oh!

Conversation

ccli8 commented Oct 8, 2019

Description

Related PR

Pull request type

Reviewers

Uh oh!

ciarmcom commented Oct 8, 2019

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

kjbracey Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

ccli8 Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

kjbracey Oct 15, 2019 • 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!

ccli8 Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

kjbracey Oct 16, 2019 • 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!

ccli8 Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

ccli8 commented Oct 16, 2019

Uh oh!

yanesca commented Oct 16, 2019

Uh oh!

ccli8 commented Oct 16, 2019

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

ccli8 Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

ccli8 commented Oct 24, 2019

Uh oh!

0xc0170 commented Oct 24, 2019

Uh oh!

mbed-ci commented Oct 24, 2019

Test run: SUCCESS

Uh oh!

Uh oh!

kjbracey Oct 15, 2019 •

edited

Loading

kjbracey Oct 16, 2019 •

edited

Loading