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

Cryptocell 310 support #6794

Merged
merged 29 commits into from May 24, 2018

Conversation

@RonEld
Contributor

RonEld commented May 2, 2018

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 ARMmbed/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

This comment has been minimized.

Member

ciarmcom commented May 3, 2018

ARM Internal Ref: IOTSSL-2262

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 3, 2018

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 RonEld force-pushed the RonEld:cc310_porting branch from 2420640 to 93c2714 May 3, 2018

@RonEld

This comment has been minimized.

Contributor

RonEld commented May 3, 2018

@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

This comment has been minimized.

Member

0xc0170 commented May 3, 2018

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

@RonEld

This comment has been minimized.

Contributor

RonEld commented May 6, 2018

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

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

@RonEld

This comment has been minimized.

Contributor

RonEld commented May 7, 2018

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

@RonEld

This comment has been minimized.

Contributor

RonEld commented May 7, 2018

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

@0xc0170

The git history looks 👍

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

This comment has been minimized.

@0xc0170

0xc0170 May 7, 2018

Member

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

This comment has been minimized.

@RonEld

RonEld May 7, 2018

Contributor

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

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

@0xc0170

LGTM, just one question about cryptocell versioning via labels.

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

This comment has been minimized.

@0xc0170

0xc0170 May 7, 2018

Member

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

This comment has been minimized.

@RonEld

RonEld May 7, 2018

Contributor

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)

This comment has been minimized.

@RonEld

RonEld May 7, 2018

Contributor

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

This comment has been minimized.

@0xc0170

0xc0170 May 7, 2018

Member

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

This comment has been minimized.

@RonEld

RonEld May 7, 2018

Contributor

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.

This comment has been minimized.

@0xc0170

0xc0170 May 8, 2018

Member

@sg- Please review (device_has and label addition)

This comment has been minimized.

@bulislaw

bulislaw May 16, 2018

Member

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

This comment has been minimized.

@cmonr

cmonr May 16, 2018

Contributor

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.

This comment has been minimized.

@RonEld

RonEld May 17, 2018

Contributor

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

This comment has been minimized.

Contributor

RonEld commented May 7, 2018

@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

@0xc0170 0xc0170 requested a review from AnotherButler May 7, 2018

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

* `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 comment has been minimized.

@AnotherButler

AnotherButler May 7, 2018

Contributor

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

@AnotherButler

Please address the comment on line 17.

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

@RonEld

This comment has been minimized.

Contributor

RonEld commented May 8, 2018

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

@0xc0170 0xc0170 requested a review from sg- May 8, 2018

* `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.

This comment has been minimized.

@Patater

Patater May 8, 2018

Contributor

"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.

This comment has been minimized.

@RonEld

RonEld May 8, 2018

Contributor

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

This comment has been minimized.

@Patater

Patater May 8, 2018

Contributor

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.

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

This comment has been minimized.

@hanno-arm

hanno-arm May 8, 2018

Contributor

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

This comment has been minimized.

@hanno-arm

hanno-arm May 8, 2018

Contributor

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.

This comment has been minimized.

@RonEld

RonEld May 8, 2018

Contributor

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

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

@RonEld

This comment has been minimized.

Contributor

RonEld commented May 22, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 22, 2018

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

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 22, 2018

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.

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

This comment has been minimized.

Contributor

RonEld commented May 23, 2018

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

Current status:

  1. Last minor change awaiting review from @Patater and @yanesca
  2. Readme possible awaiting review from Tech writer for the last modifications
  3. waiting review from @bulislaw
@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 23, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 23, 2018

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

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 23, 2018

Shuffling around some jobs.

/morph export-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 23, 2018

/morph test

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 24, 2018

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

/morph test

@mbed-ci

This comment has been minimized.

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

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

13 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 847 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9997 cycles (+554 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label May 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment