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.

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

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

Merged
merged 6 commits into from Oct 24, 2019

Conversation

@ccli8
Copy link
Contributor

ccli8 commented Oct 8, 2019

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 ciarmcom requested review from kjbracey-arm, Ronny-Liu, yanesca and ARMmbed/mbed-os-maintainers Oct 8, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 8, 2019

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

Copy link
Member

bulislaw left a comment

@Patater please review

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 15, 2019

Contributor

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.

This comment has been minimized.

Copy link
@ccli8

ccli8 Oct 16, 2019

Author Contributor

Fixed

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 15, 2019

Contributor

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?)

This comment has been minimized.

Copy link
@ccli8

ccli8 Oct 16, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 16, 2019

Contributor

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?

This comment has been minimized.

Copy link
@ccli8

ccli8 Oct 16, 2019

Author Contributor

@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 ccli8 force-pushed the OpenNuvoton:nuvoton_trng-hal-with-trng-hw branch from 6800f29 to cd63c09 Oct 16, 2019
@ccli8

This comment has been minimized.

Copy link
Contributor Author

ccli8 commented Oct 16, 2019

Make modifications:

  1. Do rebase
  2. For M2351, update default secure image/gateway library due to change of TRNG security attribute
  3. Minor fix with trng_init_counter
@yanesca

This comment has been minimized.

Copy link
Contributor

yanesca commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

ccli8 commented Oct 16, 2019

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:

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

Copy link
Contributor

yanesca left a comment

LGTM

Copy link
Member

0xc0170 left a comment

One question otherwise looks fine to me

static uint16_t trng_init_counter = 0U;

/* Mutex for synchronizing access to TRNG H/W */
static SingletonPtr<PlatformMutex> trng_mutex;

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Oct 23, 2019

Member

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.

This comment has been minimized.

Copy link
@ccli8

ccli8 Oct 24, 2019

Author Contributor

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

ccli8 added 4 commits Oct 1, 2019
Related targets:
-   NU_PFM_M2351_*
-   NUMAKER_IOT_M263A
Re-implement __PC() by replacing BSP assembly with toolchain built-in.
@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_trng-hal-with-trng-hw branch from cd63c09 to 0bb8034 Oct 24, 2019
@ccli8

This comment has been minimized.

Copy link
Contributor Author

ccli8 commented Oct 24, 2019

Make modifications:

  1. Do rebase
  2. Remove unnecessary mutex. mbedtls_mutex has done for it.
ccli8 added 2 commits Oct 1, 2019
Targets supporting TRNG H/W:

-   NU_PFM_M2351_*
-   NUMAKER_IOT_M263A
Update for change of TRNG security attribute
@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_trng-hal-with-trng-hw branch from 0bb8034 to 8161386 Oct 24, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 24, 2019

CI started

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 24, 2019
@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 24, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 19fc2b1 into ARMmbed:master Oct 24, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8669 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@ccli8 ccli8 deleted the OpenNuvoton:nuvoton_trng-hal-with-trng-hw branch Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.