Cryptocell 310 support #6794

RonEld · 2018-05-02T15:00:34Z

Description

Add support for Cryptocell 310.
Ported on Nrf52840_DK target.
Adds HW acceleration for the following cryptographic modules:
SHA1, SHA256, AES128_CCM,ECDH, ECDSA

Performance before modification(with ARMCC toolchain):

  SHA-1                    :       1330 KB/s,         46 cycles/byte
  SHA-256                  :       1158 KB/s,         53 cycles/byte
  AES-CCM-128              :        274 KB/s,        228 cycles/byte
  HMAC_DRBG SHA-1 (NOPR)   :         95 KB/s,        667 cycles/byte
  HMAC_DRBG SHA-1 (PR)     :         88 KB/s,        719 cycles/byte
  HMAC_DRBG SHA-256 (NOPR) :        128 KB/s,        491 cycles/byte
  HMAC_DRBG SHA-256 (PR)   :        113 KB/s,        559 cycles/byte
  ECDSA-secp384r1          :     965 ms/sign
  ECDSA-secp256r1          :     636 ms/sign
  ECDSA-secp384r1          :    1855 ms/verify
  ECDSA-secp256r1          :    1246 ms/verify
  ECDHE-secp384r1          :    1770 ms/handshake
  ECDHE-secp256r1          :    1193 ms/handshake
  ECDHE-Curve25519         :    1055 ms/handshake
  ECDH-secp384r1           :     870 ms/handshake
  ECDH-secp256r1           :     582 ms/handshake
  ECDH-Curve25519          :     542 ms/handshake

after acceleration:

  SHA-1                    :      16864 KB/s,          3 cycles/byte
  SHA-256                  :      17385 KB/s,          3 cycles/byte
  AES-CCM-128              :       6886 KB/s,          9 cycles/byte
  HMAC_DRBG SHA-1 (NOPR)   :        286 KB/s,        218 cycles/byte
  HMAC_DRBG SHA-1 (PR)     :        263 KB/s,        238 cycles/byte
  HMAC_DRBG SHA-256 (NOPR) :        420 KB/s,        148 cycles/byte
  HMAC_DRBG SHA-256 (PR)   :        366 KB/s,        170 cycles/byte
  ECDSA-secp384r1          :      46 ms/sign
  ECDSA-secp256r1          :      17 ms/sign
  ECDSA-secp384r1          :      63 ms/verify
  ECDSA-secp256r1          :      22 ms/verify
  ECDHE-secp384r1          :      86 ms/handshake
  ECDHE-secp256r1          :      31 ms/handshake
  ECDHE-Curve25519         :      24 ms/handshake
  ECDH-secp384r1           :      43 ms/handshake
  ECDH-secp256r1           :      16 ms/handshake
  ECDH-Curve25519          :      12 ms/handshake

Tested also with Mbed TLS self tests
Note: CCM self test passes only with Mbed-TLS/mbedtls#1598

This is a PR from the private repository - https://github.com/ARMmbed/mbed-os-cc-porting

Pull request type

[ ] Fix
[ ] Refactor
[] New target
[x] Feature
[ ] Breaking change

ciarmcom · 2018-05-03T07:50:19Z

ARM Internal Ref: IOTSSL-2262

0xc0170 · 2018-05-03T12:58:19Z

Do you have ready documentation for this feature (will you create one to Handbook?) ? To understand this new feature. I assume there will be as well porting guide (how to port this to other targets).

RonEld · 2018-05-03T13:03:18Z

@0xc0170 what sort of documentation do you have in mind?
I do plan on adding a guideline how to port to a new platform which has CC310 in it. But this is currently in discussion whether it should be in the handbook or in the source code.
Other than the guideline, I am not sure what documentation is needed. The CC310 specific documentation are released as part of the CC310 release package from IPG

0xc0170 · 2018-05-03T15:54:25Z

@RonEld Please clean the commit history and commit messages. Looks like a development branch not ready for release.

RonEld · 2018-05-06T07:43:29Z

@0xc0170 How do you suggest I do that? Should I squash all commits into a single one?

RonEld · 2018-05-07T11:02:59Z

@0xc0170 I have re-written history, and combined the commits according to topics.

RonEld · 2018-05-07T11:04:32Z

Unfortunately, during this rewrite, @k-stachowiak name was removed on some of the contributions

0xc0170

The git history looks 👍

0xc0170 · 2018-05-07T11:27:36Z

platform/mbed_sdk_boot.c

@@ -18,7 +18,9 @@
 #include <stdlib.h>
 #include <stdint.h>
 #include "cmsis.h"
-
+#if DEVICE_CRYPTOCELL


This is SDK boot . The mbed OS 5 boot is in rtos folder (unified), see mbed_boot.c file

When and how this library should be initialized (why it was added to this boot sequence) ? Why trng_init does not initalizes this (is there better place to do init than adding this here) ?

you are correct. This part was already removed as part of 98a9e91db34c8ebad5ad3276097c56ea7d416017

I probably returned it with my history changes. I'll remove it again

0xc0170

LGTM, just one question about cryptocell versioning via labels.

0xc0170 · 2018-05-07T11:50:50Z

targets/targets.json

@@ -3658,6 +3658,9 @@
    "NRF52840_DK": {
        "supported_form_factors": ["ARDUINO"],
        "inherits": ["MCU_NRF52840"],
+        "macros_add": ["MBEDTLS_CONFIG_HW_SUPPORT"],
+        "device_has_add": ["CRYPTOCELL"],


This is HAL implementation for cryptocell, that we provide. And then what is the extra label for ? To specify the version?

I added the DEVICE_CRYPTOCELL label for the sake of using the cryptocell trng.
For example, in the platform's trng_api.c, it is mutual to all of NRF5x boards, but on the NRF52840_DK, there is also cryptocell TRNG.
I am thinking that perhaps in the future, to use this label also for the MPS2 porting, done in #6169 but this is only if the API will be common for both targets, which is currently not.

The label for DEVICE_CRYPTOCELL is more intended to the partner's code, for them to specify Cryptocell specific function( in case it is common for several boards)

If your question is about the TARGET_CRYPTOCELL310, this is specifically the CC310 product. Future Cryptocell products will have a different target

DEVICE_ is used for HAL, device capabilities, like trng. Shouldn't this just be part of it? If cryptocell is enabled, it is used instead ? Does this new cryptocell provide own API or it's just what trng should use? Or does it need better name CRYPTOCELL_TRNG ?

I am confused what cryptocell is , why is it being defined as DEVICE_CRYPTOCELL and needs extra label of its own to specify a version

Cryptocell is Arm's HW accelerator.
perhaps DEVICE_CRYPTOCELL is not the right MACRO, but I used device_has, same way the MICROVISOR was used.

The extra label, is the actual target version, that partner should add\enable.
The DEVICE_CRYPTOCELL is in compilation time, to determine in the partner's common code, for some functionality that should be compiled in \ out of the partner's specific code contains Cryptocell HW. I wouldn't limit it only for TRNG, but this is probably the most common case.
The Cryptocell trng implements same API for trng as any other trng driver, so I needed some special compilation flag to determine the specific board has cryptocell, to avoid multiple definition of same function.

Perhaps the Readme I just added could explain better.

@sg- Please review (device_has and label addition)

Is cryptocell part of the SoC or is it something separate? That you connect to the board/design?

Cryptocell is an ARM security module that is part of the SoC.
The device brief mentions it several times: https://infocenter.nordicsemi.com/pdf/nRF52840_PB_v2.0.pdf

@0xc0170 I think the problem with CRYPTOCELL_TRNG is that CRYPTOCELL appears to be more generic than simply applying to the TRNG.

Cryptocell is Arm's HW crypto accelerator. It is more than just TRNG, this is why I defined it as DEVICE_CRYPTOCELL, to add flexibility for the partner to decide whether to use Cryptocell or their code, if there are duplicates

RonEld · 2018-05-07T13:10:13Z

@0xc0170 I eventually uploaded the porting guidelines to this PR. I am not sure if the location is OK.

@AnotherButler Please review the Readme file

AnotherButler · 2018-05-07T20:14:46Z

features/mbedtls/targets/TARGET_CRYPTOCELL310/Readme.md

+ 	* `MBEDTLS_CONFIG_HW_SUPPORT` to `macros_add` key.
+ 	* `CRYPTOCELL` to `device_has_add` key.
+ 	* `CRYPTOCELL310` to `extra_labels_add` key.
+1. In `objects.h`, include `objects_cryptocell.h`. (You can condition it with `#if DEVICE_CRYPTOCELL` in case you have another `trng` engine for a differnt board, and `objects.h` is common file for your boards, in this case your common `trng_api.c` file should be compiled only if `#if !defined(DEVICE_CRYPTOCELL)`).


This line confuses me. Could you please rewrite the second and third sentences for clarity?

AnotherButler

Please address the comment on line 17.

RonEld · 2018-05-08T06:50:09Z

@AnotherButler I have added usage of the labels in targets.json and rephrased the sentence accordingly, as you requested

Patater · 2018-05-08T10:23:11Z

features/mbedtls/targets/TARGET_CRYPTOCELL310/Readme.md

+ 	* `MBEDTLS_CONFIG_HW_SUPPORT` to `macros_add` key. This is used to suggest there is HW accelerated cryptography engine that will replace the default Mbed TLS implementation.
+ 	* `CRYPTOCELL` to `device_has_add` key. This should be used in your common code, that you need to remove from compilation in case CC exists in your board. Use `#if !defined(DEVICE_CRYPTOCELL)` or  `#if DEVICE_CRYPTOCELL`.
+ 	* `CRYPTOCELL310` to `extra_labels_add` key. This is used for the build system to look for the CC 310 code and binaries.
+1. In `objects.h`, include `objects_cryptocell.h`. You can use the `DEBICE_CRYPTOCELL` pre-compilation check as defined above.
 1. In `features/mbedtls/targets/TARGET_CRYPTOCELL310/TARGET_<target name>`, add your platform-specific libraries for all toolchains in `TOOLCHAIN_ARM`, `TOOLCHAIN_GCC_ARM` and `TOOLCHAIN_IAR` respectively.
 1. Add your CC setup code:
 	* Implement `cc_platform_setup()` and `cc_platform_terminate()` to enable CC on your platform, in case you have such limitations. You can implement these functions as empty functions.


"in case you have such limitations"

Could you be more explicit in this document about which limitations you have in mind? It's not obvious which limitations are being referred to.

perhaps limitations is not the proper wording. It was meant for board specific setup, prior to CC setup. I'll rephrase

OK, this is explained well enough now. Thanks.

AnotherButler

@RonEld I've completed a final copy edit of this. Please review my changes to make sure I didn't accidentally change the meaning of anything.

hanno-becker · 2018-05-08T14:21:16Z

features/mbedtls/targets/TARGET_CRYPTOCELL310/cc_rand.h

+}mbedtls_rand_func_container;
+
+
+uint32_t mbedtls_to_cc_rand_func( void *mbedtls_rnd_ctx, uint16_t outSizeBytes, uint8_t *out_ptr )


This is defined in a header, yet not declared static. Is that deliberate?

As far as I see, this file is nowhere used. It seems to be a duplicate of code from cc_internal.h and cc_internal.c.

@hanno-arm yes, it should have been removed, and probably returned on my rebase. I'll remove it

RonEld · 2018-05-22T10:59:36Z

@Patater yes, it should be without the /. I amended the commit.
Unfortunately, your (and @yanesca ) approval was dismissed

0xc0170 · 2018-05-22T12:28:41Z

@Patater @yanesca @bulislaw Please approve. This feature looks to me complete

marcuschangarm · 2018-05-22T19:16:13Z

I have change to the TARGET_MCU_NRF52840 target, although the only target I found could be built is the DK target.

Thank you! The DK is the only one at the moment, but any future boards should be inheriting from MCU_NRF52840 so now we are future ready.

Assure that `size_t` isn't larger than 32 bit, with preprocessor check. Using `#if SIZE_MAX > UINT_MAX`.

RonEld · 2018-05-23T05:49:48Z

I addressed comment from @andresag01 regarding assuring the size_t is not 64 bit.

Current status:

Last minor change awaiting review from @Patater and @yanesca
Readme possible awaiting review from Tech writer for the last modifications
waiting review from @bulislaw

0xc0170 · 2018-05-23T09:00:18Z

/morph build

mbed-ci · 2018-05-23T11:34:14Z

Build : SUCCESS

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

Triggering tests

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

cmonr · 2018-05-23T19:39:31Z

Shuffling around some jobs.

/morph export-build

cmonr · 2018-05-23T19:42:18Z

/morph test

mbed-ci · 2018-05-24T00:08:31Z

Test : FAILURE

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

mbed-ci · 2018-05-24T00:48:18Z

Exporter Build : SUCCESS

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

cmonr · 2018-05-24T02:11:14Z

Log saved for further debugging. Restarting test since failure seems timing/CI load related.

/morph test

mbed-ci · 2018-05-24T09:09:05Z

Test : SUCCESS

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

0xc0170 added needs: review tracking component: security labels May 3, 2018

0xc0170 requested review from mazimkhan and hanno-becker May 3, 2018 07:10

ciarmcom added the mirrored label May 3, 2018

RonEld force-pushed the cc310_porting branch from 2420640 to 93c2714 Compare May 3, 2018 12:59

RonEld force-pushed the cc310_porting branch from 93c2714 to 230eea4 Compare May 7, 2018 11:00

0xc0170 requested changes May 7, 2018

View reviewed changes

0xc0170 previously approved these changes May 7, 2018

View reviewed changes

RonEld dismissed 0xc0170’s stale review via 094107c May 7, 2018 13:08

0xc0170 requested a review from AnotherButler May 7, 2018 13:10

RonEld force-pushed the cc310_porting branch from 094107c to 724ad26 Compare May 7, 2018 15:38

AnotherButler reviewed May 7, 2018

View reviewed changes

AnotherButler suggested changes May 7, 2018

View reviewed changes

cmonr added needs: work and removed needs: review labels May 7, 2018

0xc0170 requested a review from sg- May 8, 2018 09:18

Patater reviewed May 8, 2018

View reviewed changes

AnotherButler previously approved these changes May 8, 2018

View reviewed changes

hanno-becker reviewed May 8, 2018

View reviewed changes

RonEld dismissed Patater’s stale review via 4fd8688 May 22, 2018 10:58

RonEld force-pushed the cc310_porting branch from e0534a3 to 4fd8688 Compare May 22, 2018 10:58

yanesca previously approved these changes May 22, 2018

View reviewed changes

Check the size_t isn't larger than 32 bit

77e87d3

Assure that `size_t` isn't larger than 32 bit, with preprocessor check. Using `#if SIZE_MAX > UINT_MAX`.

RonEld dismissed yanesca’s stale review via 77e87d3 May 23, 2018 05:44

0xc0170 approved these changes May 23, 2018

View reviewed changes

Patater approved these changes May 23, 2018

View reviewed changes

yanesca approved these changes May 23, 2018

View reviewed changes

bulislaw approved these changes May 23, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels May 23, 2018

0xc0170 added ready for merge and removed needs: CI labels May 24, 2018

cmonr merged commit 2f86c15 into ARMmbed:master May 24, 2018

cmonr removed the ready for merge label May 24, 2018

ciarmcom added the closed_in_jira label May 24, 2018

cmonr mentioned this pull request May 31, 2018

Remove redundant Cryptocell libraries #7074

Merged

RonEld mentioned this pull request Feb 6, 2019

Initialize platform in trng test #9493

Merged

		}mbedtls_rand_func_container;


		uint32_t mbedtls_to_cc_rand_func( void mbedtls_rnd_ctx, uint16_t outSizeBytes, uint8_t out_ptr )

Cryptocell 310 support #6794

Cryptocell 310 support #6794

Uh oh!

Conversation

RonEld commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

ciarmcom commented May 3, 2018

Uh oh!

0xc0170 commented May 3, 2018

Uh oh!

RonEld commented May 3, 2018

Uh oh!

0xc0170 commented May 3, 2018

Uh oh!

RonEld commented May 6, 2018

Uh oh!

RonEld commented May 7, 2018

Uh oh!

RonEld commented May 7, 2018

Uh oh!

0xc0170 left a comment

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!

0xc0170 left a comment

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!

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!

RonEld commented May 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnotherButler left a comment

Choose a reason for hiding this comment

Uh oh!

RonEld commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater May 8, 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!

AnotherButler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker May 8, 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!

RonEld commented May 22, 2018

RonEld commented May 2, 2018 •

edited

Loading

Patater May 8, 2018 •

edited

Loading

hanno-becker May 8, 2018 •

edited

Loading