CM3DS Maintenance Pull Request: TRNG support (3/4) #6169

hug-dev · 2018-02-22T15:38:57Z

This pull request brings TRNG support to the CM3DS target.
It is the pull request 3 among the 4 concerning CM3DS Maintenance. Please check #6119 for the full scope.

Other pull requests regarding this target:

CM3DS Maintenance Pull Request: Bug fixes (1/4) CM3DS Maintenance Pull Request: Bug fixes (1/4) #6167
CM3DS Maintenance Pull Request: Memory changes (2/4) CM3DS Maintenance Pull Request: Memory changes (2/4) #6168
CM3DS Maintenance Pull Request: TRNG support (3/4) CM3DS Maintenance Pull Request: TRNG support (3/4) #6169
CM3DS Maintenance Pull Request: Driver updates (4/4) CM3DS Maintenance Pull Request: Driver updates (4/4) #6170

ciarmcom · 2018-02-23T16:50:35Z

ARM Internal Ref: IOTSSL-2109

bulislaw

@0xc0170 @cmonr @adbridge @sg- This commit includes new external OS (apache2) project with all the bells and whistles. Any thoughts?

cmonr · 2018-02-27T06:47:12Z

Seems fine to me as long as the source project is left asis, similar to how we include external libraries as well. Apache 2.0 is fine.

For reference: https://os.mbed.com/docs/latest/reference/guidelines.html#license

Or are you referring to the files in targets/TARGET_ARM_SSG/TARGET_CM3DS_MPS2/device/drivers/TZ-TRNG/host/* ?

0xc0170 · 2018-02-27T08:56:39Z

Or are you referring to the files in targets/TARGET_ARM_SSG/TARGET_CM3DS_MPS2/device/drivers/TZ-TRNG/host/* ?

I believe so, that is 3rd library in this pull request. Looks good to me as it is

@hug-dev Please can you rebase?

hug-dev · 2018-02-27T11:14:58Z

Seems fine to me as long as the source project is left asis, similar to how we include external libraries as well. Apache 2.0 is fine.

Yes, I made sure to leave the project as it is, I had to make a pull request (ARM-software/TZ-TRNG#8) to the project to make it work with mbed-os.
version.txt file contains the commit SHA of the project and the (tiny) differences with it.

@hug-dev Please can you rebase?

Yes sure, I am going to do it.

hug-dev · 2018-02-27T15:00:38Z

Just rebased to latest master.

hanno-becker · 2018-03-01T10:57:53Z

.../TARGET_ARM_SSG/TARGET_CM3DS_MPS2/device/drivers/TZ-TRNG/host/src/tztrng_lib/tztrng_driver.c

+    }
+
+    /* zeroe the rnd buff  */
+    tztrng_memset((uint8_t *)rndWorkBuff, 0, CC_RND_WORK_BUFFER_SIZE_WORDS * sizeof(uint32_t));


Consider using sizeof( rndWorkBuff ) instead here.

hanno-becker · 2018-03-01T11:07:10Z

.../TARGET_ARM_SSG/TARGET_CM3DS_MPS2/device/drivers/TZ-TRNG/host/src/tztrng_lib/tztrng_driver.c

+        }
+
+        /* try to reduce the memcpy */
+        if (requireBytes < (int)sourceOutSizeBytes)


Strictly speaking, this is a dangerous cast - if LLF_RND_GetTrngSource is compromised and sets sourceOutSizeBytes to a value above MAX_INT, then (int) sourceOutSizeBytes may be negative, and the resulting else branch performs - which uses sourceOutSizeBytes as a uint32_t - performs a fauly memcpy.

hug-dev · 2018-03-01T12:00:21Z

Thanks @hanno-arm for your review. I agree with both of them.

As it has been said before, the TZ-TRNG project introduced with this commit is an external open-source project (https://github.com/ARM-software/TZ-TRNG) which I do not have ownership with. For maintenance reasons, it would be better if there are no differences between the version of that project here and the original one.
For that matter, I can make a pull-request with your suggested changes to original project and, if they get merged, I can then import here the new version.

What do you think about that?

0xc0170 · 2018-03-08T14:32:33Z

For that matter, I can make a pull-request with your suggested changes to original project and, if they get merged, I can then import here the new version.

Sounds good 👍

@hanno-arm @mazimkhan Anything blocking that should take priority to go upstream?

mazimkhan · 2018-03-12T09:58:15Z

.../TARGET_ARM_SSG/TARGET_CM3DS_MPS2/device/drivers/TZ-TRNG/host/src/tztrng_lib/tztrng_driver.c

+        if (requireBytes < (int)sourceOutSizeBytes)
+            tztrng_memcpy(outAddr, (uint8_t *)rndSrc_ptr, requireBytes);
+        else
+            tztrng_memcpy(outAddr, (uint8_t *)rndSrc_ptr, sourceOutSizeBytes);


rndSrc_ptr should also be scrubbed otherwise we are leaking entropy on stack.

If rndSrc_ptr is from rndWorkBuff then please clear rndWorkBuff.

I am not sure to understand well: do you want me to write requireBytes or sourceOutSizeBytes zeroes at rndStr_ptr address location after the memcpy call? Also, should I do it for every iteration of the while loop or just once at the end?

Sorry for my question, I understood what you meant. I will send my pull-request soon.

No worries. Sorry for late reply. Yes I meant:

write requireBytes or sourceOutSizeBytes zeroes at rndStr_ptr address location after the memcpy call

The (int) cast of sourceOutSizeBytes could result in a negative value if this variable is bigger than INT_MAX which can lead to a faulty memcpy. Two other changes have been done on the same function: * use sizeof on the array to have its size in bytes directly * clear the rndWorkBuff array to not leak entropy on the stack The change here proposed are a result of the review of the TZ-TRNG driver integrated into CM3DS port on mbed OS. It has been reviewed in the pull request ARMmbed/mbed-os#6169 Change-Id: Ifdfa8ac854365989723239d382f12f42648429f1 Signed-off-by: Hugues de Valon <[email protected]>

The (int) cast of sourceOutSizeBytes could result in a negative value if this variable is bigger than INT_MAX which can lead to a faulty memcpy. Two other changes have been done on the same function: * use sizeof on the array to have its size in bytes directly * clear the rndWorkBuff array to not leak entropy on the stack The change here proposed are a result of the review of the TZ-TRNG driver integrated into CM3DS port on mbed OS. It has been reviewed in the pull request ARMmbed/mbed-os#6169 Signed-off-by: Hugues de Valon <[email protected]>

hug-dev · 2018-03-14T14:14:45Z

The pull request ARM-software/TZ-TRNG#9 has been added!

cmonr · 2018-03-21T03:16:37Z

@hanno-arm @mazimkhan @0xc0170 Are y'all please with the updated commit?

0xc0170 · 2018-03-21T15:52:56Z

targets/TARGET_ARM_SSG/TARGET_CM3DS_MPS2/trng_api.c

+ * @param output_length The length of generated data
+ * @return 0 success, -1 fail
+ */
+int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length)


Why are we including here doxygen (they are from header file copy paste?) ? If yes, we shall remove them. If there is another info that should be added here, could do via own implementation comments.

They are indeed copied from the header file. I am going to remove them.

This patchs adds TRNG support using the upstreamed, open-source, TZ-TRNG driver. It also implements the HAL for TRNG and add it in features. The mbed-os.py script deletes files that are unused by mbed-os. Change-Id: Idf8eefd809f00d40e0ad3cf7657f2a8c2eca3505 Signed-off-by: Hugues de Valon <[email protected]>

hug-dev · 2018-04-06T10:35:38Z

I have just pushed an updated commit. It contains all the modifications previously done and, instead of adding a .mbedignore file adds a mbed-os.py script that deletes unused files. This same script was used before pushing and can be use in the future when new versions of the TRNG driver are out.

hanno-becker

The TRNG code in trng_api.c looks fine to me. As I understand @0xc0170, a review of the underlying CC driver code is not needed here.

0xc0170 · 2018-04-10T09:34:07Z

/morph build

mbed-ci · 2018-04-10T10:38:19Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-04-10T13:15:17Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-04-10T13:33:11Z

Test : FAILURE

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

hug-dev · 2018-04-10T13:45:58Z

I may be wrong but it seems that tests that failed are not related with this PR (as the files changed may not even be compiled in the tests). It is maybe that I am using an old Mbed OS version, if that is the case I can rebase this pull request.

0xc0170 · 2018-04-10T14:23:00Z

/morph test

mbed-ci · 2018-04-11T07:36:34Z

Test : FAILURE

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

ashok-rao · 2018-04-11T09:12:05Z

Looks like failure on a different un-related platform..

0xc0170 · 2018-04-11T09:33:22Z

That is correct, the target needs to be looked at , not related. I've just commented in another 3 PR with the same problem

0xc0170 · 2018-04-11T12:35:06Z

/morph test

mbed-ci · 2018-04-12T11:52:14Z

Test : SUCCESS

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

This commit updates the way this driver is integrated in Mbed OS. It adds a Python script, mbed-os.py that deletes some files to use the driver in 800-90B TRNG mode. It also updates the README.md with the new instructions. The pull request ARMmbed/mbed-os#6169 was merged in Mbed OS using this new way. Signed-off-by: Hugues de Valon <[email protected]>

This was referenced Feb 22, 2018

CM3DS Maintenance Pull Request #6119

Closed

CM3DS Maintenance Pull Request: Driver updates (4/4) #6170

Merged

CM3DS Maintenance Pull Request: Memory changes (2/4) #6168

Merged

CM3DS Maintenance Pull Request: Bug fixes (1/4) #6167

Merged

cmonr requested review from bulislaw and jamesbeyond February 22, 2018 19:40

cmonr added the needs: review label Feb 22, 2018

0xc0170 added tracking component: security labels Feb 23, 2018

0xc0170 requested review from mazimkhan and hanno-becker February 23, 2018 14:22

ciarmcom added the mirrored label Feb 23, 2018

bulislaw reviewed Feb 26, 2018

View reviewed changes

hug-dev force-pushed the cm3ds-trng branch from 0a19176 to d1cfebb Compare February 27, 2018 14:59

hanno-becker reviewed Mar 1, 2018

View reviewed changes

mazimkhan reviewed Mar 12, 2018

View reviewed changes

hug-dev mentioned this pull request Mar 14, 2018

Replace type to avoid corner case ARM-software/TZ-TRNG#9

Merged

0xc0170 reviewed Mar 21, 2018

View reviewed changes

hug-dev force-pushed the cm3ds-trng branch from d1cfebb to acb5318 Compare April 6, 2018 10:32

hanno-becker approved these changes Apr 10, 2018

View reviewed changes

0xc0170 approved these changes Apr 10, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Apr 10, 2018

0xc0170 added ready for merge and removed needs: CI labels Apr 12, 2018

cmonr merged commit 641e814 into ARMmbed:master Apr 12, 2018

cmonr added release-version: 5.9.0-rc1 release-version: 5.8.3 and removed ready for merge release-version: 5.9.0-rc1 labels Apr 12, 2018

ciarmcom added the closed_in_jira label Apr 12, 2018

hug-dev mentioned this pull request Apr 20, 2018

CM3DS Maintenance Pull Request: Cleaning #6696

Merged

RonEld mentioned this pull request May 7, 2018

Cryptocell 310 support #6794

Merged

hug-dev mentioned this pull request May 10, 2018

Update Mbed OS integration ARM-software/TZ-TRNG#10

Closed

CM3DS Maintenance Pull Request: TRNG support (3/4) #6169

CM3DS Maintenance Pull Request: TRNG support (3/4) #6169

Uh oh!

Conversation

hug-dev commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ciarmcom commented Feb 23, 2018

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Feb 27, 2018

Uh oh!

0xc0170 commented Feb 27, 2018

Uh oh!

hug-dev commented Feb 27, 2018

Uh oh!

hug-dev commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hug-dev commented Mar 1, 2018

Uh oh!

0xc0170 commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hug-dev Mar 12, 2018 • 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!

hug-dev commented Mar 14, 2018

Uh oh!

cmonr commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hug-dev commented Apr 6, 2018

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 10, 2018

Uh oh!

mbed-ci commented Apr 10, 2018

Triggering tests

Uh oh!

mbed-ci commented Apr 10, 2018

Uh oh!

mbed-ci commented Apr 10, 2018

Uh oh!

hug-dev commented Apr 10, 2018

Uh oh!

0xc0170 commented Apr 10, 2018

Uh oh!

mbed-ci commented Apr 11, 2018

Uh oh!

ashok-rao commented Apr 11, 2018

Uh oh!

0xc0170 commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 11, 2018

Uh oh!

mbed-ci commented Apr 12, 2018

Uh oh!

Uh oh!

hug-dev commented Feb 22, 2018 •

edited

Loading

hug-dev Mar 12, 2018 •

edited

Loading

0xc0170 commented Apr 11, 2018 •

edited

Loading