Nuvoton: TRNG_Get support 32 bytes unalignment #5454

cyliangtw · 2017-11-08T06:40:19Z

Description

This PR refines code with TRNG_Get, to let TRNG_Get() support 32 bytes unalignment on NUC472/M487 platforms.

Related PRs

#5434
#5449

Test

Passed Mbed Cloud-Client-restricted(R1.2.4-LA) OTA test.

@0xc0170
@Ronny-Liu
@ccli8
@samchuarm

0xc0170 · 2017-11-08T07:56:38Z

cc @hanno-arm @Patater - please can you review? (I cant asign you as reviewer here, not certain why it does not work for me now)

hanno-becker · 2017-11-08T08:15:22Z

targets/TARGET_NUVOTON/TARGET_M480/trng_api.c

+        if( length > *output_length ) {
+            trng_get(tmpBuff);
+            memcpy(output, &tmpBuff, (length - *output_length));
+            *output_length = length;


The two code-paths handling the case of remaining length < 32 can be unified here. Also, while it's a pre-existing defect, we should safely zeroize tmpBuff before returning.

@hanno-arm , thanks of your comment, added new commit for code unify & zeroize.

hanno-becker · 2017-11-08T08:17:20Z

@0xc0170 Sure. Could you also add the Mbed TLS label for ease of reference?

ciarmcom · 2017-11-08T12:50:37Z

ARM Internal Ref: IOTSSL-1875

hanno-becker · 2017-11-08T14:33:43Z

targets/TARGET_NUVOTON/TARGET_M480/trng_api.c

+        output += 32;
+    }     
+    if( length > *output_length ) {
+        trng_zeroize(tmpBuff, sizeof(tmpBuff));


@cyliangtw Thanks for the quick reworking. The zeroization should be done after the bytes have been fetched from the TRNG.

@hanno-arm , after fetch from TRNG and copy to output, tmpBuffer is no longer used in this function. Why still need pay CPU resource to do zeroization, do you concern to disclose partial of random number in stack memory ?

@cyliangtw Yes, exactly. The zeroization at the end (which is the only one that's needed here) is to prevent leakage of the random data on the stack - imagine the OS handing the same stack memory to some other process later. That's why one generally wipes sensitive data from the stack before returning.

hanno-becker · 2017-11-10T06:55:27Z

targets/TARGET_NUVOTON/TARGET_NUC472/trng_api.c

        trng_get(tmpBuff);
-        memcpy(output, &tmpBuff, length);
+        memcpy(output, &tmpBuff, (length - *output_length));


One cannot reason about the correctness of this because *output_length might have changed since the check length > *output_length -- it's not under our control (it is likely that the compiler will not read it twice as it's not declared volatile, but it could). My suggestion would be to use an additional temporary to keep track of the number of bytes written so far, and to set *output_length to that temporary in the end.

@hanno-arm Agree with your opinion to avoid the risk of *output_length's change.
Commit 2ee058b

hanno-becker

Thanks for the quick reworkings, the PR looks good to me!

@cyliangtw Two minor nigglings that you can but need not obey:

You inconsistently use tmpBuff vs. &tmpBuff, which when converted to void* doesn't make a difference, but still it would be nicer to have it uniform. I'd prefer tmpBuff everywhere.
The function is slightly easier to read if length gets decreased during the main loop, and that loop goes as long as length >= 32. This way, it's immediately clear that the last branch will at most write 32 bytes and hence cannot overflow the temporary buffer. (Again, the present version is fine wrt. functionality and security from my point of view, so it's just a suggestion).

0xc0170 · 2017-11-10T11:05:49Z

/morph build

mbed-ci · 2017-11-10T12:27:22Z

Build : SUCCESS

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

Triggering tests

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

Patater · 2017-11-10T13:30:59Z

targets/TARGET_NUVOTON/TARGET_NUC472/trng_api.c

@@ -82,21 +87,21 @@ void trng_free(trng_t *obj)
 int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length)


trng_get_bytes looks pretty similar between TARGET_NUC472 and TARGET_M480. Would it be prudent to move it to a location shared by both TARGET_NUC472 and TARGET_M480?

@Patater We will consider to be shared as while one more platform utilize the same HW IP.

Patater · 2017-11-10T13:32:09Z

targets/TARGET_NUVOTON/TARGET_M480/trng_api.c

-    *output_length = 0;
-    if (length < 32) {
-        unsigned char tmpBuff[32];
+    unsigned char tmpBuff[32];


Consider making a define for 32 to use for the size of tmpBuff.

@Patater Update the code in new commit d8a9e35

Patater · 2017-11-10T13:32:10Z

targets/TARGET_NUVOTON/TARGET_M480/trng_api.c

+    unsigned char tmpBuff[32];
+    size_t cur_length = 0;
+
+    for (unsigned i = 0; i < (length/32); i++) {


Consider using sizeof(tmpBuff) instead of 32.

Consider also using spaces around binary operators like /, for consistency with the style of other code in this file.

@Patater Update the code in new commit d8a9e35

mbed-ci · 2017-11-10T15:15:43Z

Test : SUCCESS

Build number : 309
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5454/309

mbed-ci · 2017-11-10T15:43:38Z

Exporter Build : SUCCESS

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

theotherjimmy · 2017-11-13T16:35:02Z

@Patater Were your questions resolved by the latest commit?

cyliangtw · 2017-11-17T02:00:28Z

@Patater The last commit could resolve your concerns except shared location. This PR is relative to mbed-cloud-client-example-restricted PR for Nuvoton platforms, is there any other concern in your review ?

@samchuarm

Patater

LGTM

theotherjimmy · 2017-11-20T16:39:55Z

/morph build

mbed-ci · 2017-11-20T17:18:29Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2017-11-20T18:28:07Z

Exporter Build : SUCCESS

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

mbed-ci · 2017-11-21T01:19:57Z

Test : SUCCESS

Build number : 372
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5454/372

0xc0170 · 2017-11-21T15:00:13Z

This is now waiting for travis only, to be resolved

cyliangtw · 2017-11-22T12:31:25Z

@0xc0170 To close and reopen this PR to trigger build in Travis

0xc0170 · 2017-11-22T12:35:08Z

@cyliangtw It's working now 🍾

mbed-ci · 2017-11-22T13:18:40Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2017-11-22T13:59:00Z

Exporter Build : SUCCESS

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

mbed-ci · 2017-11-22T14:21:19Z

Test : SUCCESS

Build number : 389
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5454/389

mbed-ci · 2017-11-22T14:52:53Z

Exporter Build : SUCCESS

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

[M487/NUC472] TRN_Get support 32 bytes unalignment

4118afa

0xc0170 added the needs: review label Nov 8, 2017

hanno-becker reviewed Nov 8, 2017

View reviewed changes

0xc0170 added component: security tracking labels Nov 8, 2017

ARMmbed deleted a comment from adbridge Nov 8, 2017

[M487/NUC472] Unified code-path for remaining bytes of TRNG_Get

76c2c19

ciarmcom added the mirrored label Nov 8, 2017

hanno-becker reviewed Nov 8, 2017

View reviewed changes

[M487/NUC472] zeroize random data on the stack memory

e252b10

hanno-becker reviewed Nov 10, 2017

View reviewed changes

[M487/NUC472] Refine for correctness control

2ee058b

hanno-becker approved these changes Nov 10, 2017

View reviewed changes

Patater reviewed Nov 10, 2017

View reviewed changes

[M487/NUC472] Refine trng_get_bytes for consistency and readability

d8a9e35

Patater approved these changes Nov 17, 2017

View reviewed changes

theotherjimmy added needs: CI and removed needs: review labels Nov 20, 2017

cyliangtw closed this Nov 22, 2017

sg- removed the needs: CI label Nov 22, 2017

cyliangtw reopened this Nov 22, 2017

0xc0170 added the needs: CI label Nov 22, 2017

theotherjimmy added ready for merge release-version: 5.6.6 and removed needs: CI labels Nov 22, 2017

theotherjimmy merged commit ab1b3ae into ARMmbed:master Nov 22, 2017

theotherjimmy removed the ready for merge label Nov 22, 2017

ccli8 mentioned this pull request Nov 23, 2017

M487: Fix premature lp_ticker interrupt #5552

Closed

ciarmcom added the closed_in_jira label Dec 1, 2017

cyliangtw deleted the trng_get_unalignment branch February 27, 2018 03:03

		@@ -82,21 +87,21 @@ void trng_free(trng_t *obj)
		int trng_get_bytes(trng_t obj, uint8_t output, size_t length, size_t *output_length)

Nuvoton: TRNG_Get support 32 bytes unalignment #5454

Nuvoton: TRNG_Get support 32 bytes unalignment #5454

Uh oh!

Conversation

cyliangtw commented Nov 8, 2017

Description

Related PRs

Test

Uh oh!

0xc0170 commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker commented Nov 8, 2017

Uh oh!

ciarmcom commented Nov 8, 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!

hanno-becker Nov 10, 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!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Nov 10, 2017

Uh oh!

mbed-ci commented Nov 10, 2017

Triggering tests

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!

mbed-ci commented Nov 10, 2017

Uh oh!

mbed-ci commented Nov 10, 2017

Uh oh!

theotherjimmy commented Nov 13, 2017

Uh oh!

cyliangtw commented Nov 17, 2017

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Nov 20, 2017

Uh oh!

mbed-ci commented Nov 20, 2017

Triggering tests

Uh oh!

mbed-ci commented Nov 20, 2017

Uh oh!

mbed-ci commented Nov 21, 2017

Uh oh!

0xc0170 commented Nov 21, 2017

Uh oh!

cyliangtw commented Nov 22, 2017

Uh oh!

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

Uh oh!

Uh oh!

hanno-becker Nov 10, 2017 •

edited

Loading

0xc0170 commented Nov 22, 2017 •

edited

Loading