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

Port aes cc310 driver #10907

Merged
merged 4 commits into from Aug 23, 2019

Conversation

@RonEld
Copy link
Contributor

commented Jun 27, 2019

Description

Add support for CC310 AES driver,
returning MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED for key size
other than 128 bits, and for AES modes not supported by the driver.
Use MBEDTLS_CTR_DRBG_USE_128_BIT_KEY.
Benchmark information, built with GCC_ARM:

 AES-CBC-128              :      15023 KB/s
  AES-CBC-192              :  Feature unsupported
  AES-CBC-256              :  Feature unsupported
  AES-CTR-128              :      15017 KB/s
  AES-CTR-192              :  Feature unsupported
  AES-CTR-256              :  Feature unsupported
  AES-GCM-128              :        232 KB/s
  AES-GCM-192              :  Feature unsupported
  AES-GCM-256              :  Feature unsupported
  AES-CMAC-128             :        531 KB/s
  AES-CMAC-192             :  Feature unsupported
  AES-CMAC-256             :  Feature unsupported

Tested also with the Mbed TLS On Target Tests on NRF52840_DK

Pull request type

[ ] Fix
[ ] Refactor
[x] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-crypto

Release Notes

This is a target update, adding hw accelerated AES, however, it can also be considered as a breaking change, as it removes support for AES other than 128 bit keys.

@40Grit

This comment has been minimized.

Copy link

commented Jun 27, 2019

Please show effect on resource usage. Flash/ram

@trowbridgec @AGlass0fMilk

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Using the generated map file, I see the following changes:
RAM: no change
ROM: with the Cryptocell AES driver, 2432 bytes smaller than the sw AES implementation

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

benchmark performance with the sw driver:

  AES-CBC-128              :        509 KB/s
  AES-CBC-192              :        439 KB/s
  AES-CBC-256              :        386 KB/s
  AES-CTR-128              :        513 KB/s
  AES-CTR-192              :        442 KB/s
  AES-CTR-256              :        388 KB/s
  AES-GCM-128              :        214 KB/s
  AES-GCM-192              :        200 KB/s
  AES-GCM-256              :        189 KB/s
  AES-CMAC-128             :        440 KB/s
  AES-CMAC-192             :        386 KB/s
  AES-CMAC-256             :        344 KB/s
  AES-CMAC-PRF-128         :        439 KB/s
@40Grit

This comment has been minimized.

Copy link

commented Jun 27, 2019

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jun 27, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@RonEld, thank you for your changes.
@ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-maintainers please review.

Add support for CC310 AES driver,
returning `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED` for key size
other than 128 bits, and for AES modes not supported by the driver.
Use `MBEDTLS_CTR_DRBG_USE_128_BIT_KEY`.
@RonEld RonEld force-pushed the RonEld:cc310_aes_port branch from 1a2f640 to a58457d Jun 27, 2019
@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

I amended the three files with the correct copyright year and updated the Arm brand name

}
#if defined(MBEDTLS_CIPHER_MODE_XTS)

void mbedtls_aes_xts_init( mbedtls_aes_xts_context *ctx ){}

This comment has been minimized.

Copy link
@Patater

Patater Jun 28, 2019

Contributor

If we don't define these, if someone attempts to use them, they'll get build failures. If we implement them doing nothing, that's a small waste of code space, but also could hide the unsupported nature until run-time. Maybe worth adding #error AES XTS is not supported with CC310 AES within this #if defined(MBEDTLS_CIPHER_MODE_XTS), instead of making empty functions.

This comment has been minimized.

Copy link
@RonEld

RonEld Jun 30, 2019

Author Contributor

Personally, I also prefer catching issues as soon as possible, e.g. at build time. However. this is according to our design, and in this specific case it may cause CI issues, when trying to set different configurations on different platforms

This comment has been minimized.

Copy link
@Patater

Patater Jul 1, 2019

Contributor

OK for now in this PR.

That's a problem in our CI, IMO. We shouldn't count it as a failure, nor attempt such an an invalid configuration, when a certain platform's ALT implementation can't do certain block modes.

This comment has been minimized.

Copy link
@RonEld

RonEld Jul 1, 2019

Author Contributor

Yes, this is why I added the FEATURE UNSUPPORTED PR

This comment has been minimized.

Copy link
@Patater

Patater Jul 31, 2019

Contributor

OK


}

int mbedtls_aes_setkey_dec( mbedtls_aes_context *ctx, const unsigned char *key,

This comment has been minimized.

Copy link
@Patater

Patater Jun 28, 2019

Contributor

This looks identical to mbedtls_aes_setkey_enc() other than one flag. Could we implement these functions by returning the result of a new function return mbedtls_aes_setkey(SASI_AES_DECRYPT)? It should save a bit of flash space, and be more readable.

return( MBEDTLS_ERR_AES_BAD_INPUT_DATA );

return( CC_aes_cipher( ctx, mode, SASI_AES_MODE_ECB, 16, NULL, 0, input, output ) );

This comment has been minimized.

Copy link
@Patater

Patater Jun 28, 2019

Contributor

Style: Sometimes there is a newline here after return before }, and sometimes not. Be consistent at least. We prefer the no newline in this location style.


if ( *nc_off )
{
/* handle corner case where we are resuming a previous encryption,

This comment has been minimized.

Copy link
@Patater

Patater Jun 28, 2019

Contributor

Does CC_aes_cipher not handle this corner case itself?

This comment has been minimized.

Copy link
@RonEld

RonEld Jun 30, 2019

Author Contributor

no. CC CTR implementation only supports where *nc_off is zero

This comment has been minimized.

Copy link
@Patater

Patater Jul 1, 2019

Contributor

What happens if it isn't? Incorrect computation? How do users of the CC API cope with this limitation ordinarily?

This comment has been minimized.

Copy link
@RonEld

RonEld Jul 1, 2019

Author Contributor

yes, incorrect computation. Users of CC, that are interested in *nc_off != 0 will need to do the same thing

if( ( ( length % SASI_AES_BLOCK_SIZE_IN_BYTES ) != 0 ) && ret == 0 )
{
/* in case the length is not aligned, generate stream block for resuming
* increase nonce_block to the correct value*/

This comment has been minimized.

Copy link
@Patater

Patater Jun 28, 2019

Contributor

Could we fail with hardware unsupported or invalid parameter when alignment is not correct?

If not, maybe it's smaller in code size to copy the block to an aligned location, perform the hardware operation, and then copy back.

This comment has been minimized.

Copy link
@RonEld

RonEld Jun 30, 2019

Author Contributor

It's bad wording. It's not a matter of memory alignment. It's a matter of length which is a multiple of AES block size (16). This functionality generates the stream block for resuming on next call( if there is), for generating the first block on next block ( since *nc_off is not zero in this case)

This comment has been minimized.

Copy link
@RonEld

RonEld Jun 30, 2019

Author Contributor

I think that returning unsupported feature in this case will result in a major functionality decrease, on the expense of minor flash set back, IMHO

This comment has been minimized.

Copy link
@Patater

Patater Jul 1, 2019

Contributor

What's the use case exactly? Why this is a major functionality decrease? Don't users usually use AES with a length that's a multiple of the block length?

This comment has been minimized.

Copy link
@RonEld

RonEld Jul 1, 2019

Author Contributor

not for CTR.
CTR is streaming mode, which you can send any length of data, and pause the streaming in the middle, resume the streaming from same location, or any other arbitrary location

This comment has been minimized.

Copy link
@Patater

Patater Jul 31, 2019

Contributor

Seems odd that CC only lets you do CTR in multiples of the block length. Is that really the case? Doesn't that sort of defeat the purpose of streaming?

This comment has been minimized.

Copy link
@gilles-peskine-arm

gilles-peskine-arm Jul 31, 2019

@Patater CTR mode is a stream cipher based on top of a block operation. From the user's perspective, partial inputs can have any byte length (even any bit length in theory). But under the hood, the operation involving the key works on 128 bits at a time (for a cipher with 128-bit blocks). If a call completes with a total input size that isn't a multiple of the block size, something in the stack has to remember the partial state. The hardware can't do this by itself (it would need to store the state of all ongoing calls). Either the hardware stores the partial state in an opaque form that the driver just keeps in memory, or the hardware works on 128 bits at a time and it's up to the driver to compensate. Usually, it's the latter: if the “hardware” is doing it, it means that some software layer deeper down is doing it.

This comment has been minimized.

Copy link
@RonEld

RonEld Jul 31, 2019

Author Contributor

@Patater In addition to what was mentioned by @gilles-peskine-arm CC supports any byte length as input, but it doesn't support byte offset.
As you can see from previous call to CC driver(line 284), the full length ( after reducing nc_off ), no matter what the length is. However, if the length is not a multiple of 16, then we need to calculate stream_block for next API call.

1. Make common function for setting key, which receives
the direction as parameter.
2. Remove rediundant extra lines.
@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

@Patater I addressed your issues.
I modified the code according to the comments I agree with, and waiting for your feedback on other comments.

@bulislaw

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

it can also be considered as a breaking change, as it removes support for AES other than 128 bit keys.

I don't exactly know how it used to work, but I don't want us to break people use cases. I'd much rather add it as an option for now and make it as default for 6.

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

I don't exactly know how it used to work, but I don't want us to break people use cases. I'd much rather add it as an option for now and make it as default for 6.

Previously, AES 192 and AES 256 were supported by SW implementation. With this PR, a request for these key size modes( and some addition aes modes such as XTS and OFB ) will return an unsupported error.
The idea was to make it for the next major release, unless it is required for current minor release, in which case we should make it optional, by removing MBEDTLS_AES_ALT from mbedtls_device.h and users that want this feature will need to add this definition in mbed_app.json

Have the alternative aes undefined by default,
in order not to break backwards compatability.
`MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` remains defined for better usability.
@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

I made the alternative aes optional. I will make a separate PR, updating the readme file

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

@ARMmbed/mbed-os-crypto Please leave your review conclusions, + or -, Can this move forward, or does this require more work?

if( ( ( length % SASI_AES_BLOCK_SIZE_IN_BYTES ) != 0 ) && ret == 0 )
{
/* in case the length is not aligned, generate stream block for resuming
* increase nonce_block to the correct value*/

This comment has been minimized.

Copy link
@Patater

Patater Jul 31, 2019

Contributor

Seems odd that CC only lets you do CTR in multiples of the block length. Is that really the case? Doesn't that sort of defeat the purpose of streaming?

}
#if defined(MBEDTLS_CIPHER_MODE_XTS)

void mbedtls_aes_xts_init( mbedtls_aes_xts_context *ctx ){}

This comment has been minimized.

Copy link
@Patater

Patater Jul 31, 2019

Contributor

OK

static int CC_aes_setkey( mbedtls_aes_context *ctx, const unsigned char *key,
unsigned int keybits, SaSiAesEncryptMode_t cipher_flag )
{

This comment has been minimized.

Copy link
@Patater

Patater Jul 31, 2019

Contributor

Remove extra newline

}

return ( 0 );

This comment has been minimized.

Copy link
@Patater

Patater Jul 31, 2019

Contributor

Remove extra newline


}
int mbedtls_aes_setkey_enc( mbedtls_aes_context *ctx, const unsigned char *key,
unsigned int keybits )

This comment has been minimized.

Copy link
@Patater

Patater Jul 31, 2019

Contributor

Align arguments

int mbedtls_aes_crypt_ecb( mbedtls_aes_context *ctx,
int mode,
const unsigned char input[16],
unsigned char output[16] )

This comment has been minimized.

Copy link
@Patater

Patater Jul 31, 2019

Contributor

Align arguments

@@ -22,6 +22,8 @@
#define __MBEDTLS_DEVICE__

#define MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT
//#define MBEDTLS_AES_ALT
#define MBEDTLS_CTR_DRBG_USE_128_BIT_KEY

This comment has been minimized.

Copy link
@Patater

Patater Jul 31, 2019

Contributor

I don't think we want to degrade the security of devices by default. Consider wrapping this setting in a ifdef, with a comment explaining why it's needed.

#if defined(MBEDTLS_AES_ALT)
#define MBEDTLS_CTR_DRBG_USE_128_BIT_KEY
#endif

This comment has been minimized.

Copy link
@gilles-peskine-arm

gilles-peskine-arm Jul 31, 2019

AES-128 is acceptable as default security as long as there are less than about one quintillion devices (128-bit security, divide by two for the birthday bound) under ideal conditions (if there are side channels, it can be another matter, but more bits often doesn't help anyway). AES-256 gives a better security margin, but there's no reason to require it.

This comment has been minimized.

Copy link
@RonEld

RonEld Jul 31, 2019

Author Contributor

I agree, we don't want to degrade security, and perhaps as long as AES_ALT is not defined we shouldn't , however I decide to add it in order to have as less configurability as possible, for readability reasons. In addition, as mentioned by Gilles, the degrade doesn't have much effect.

1. Remove redundant extra lines.
2. Have the function parameters aligned.
3. Remove redundant white spaces.
@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

@Patater @gilles-peskine-arm I have fixes some style issues.
Please review.
I haven't changed according to #10907 (comment) as I don't think it's needed.

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

This comment has been minimized.

Copy link

commented Aug 20, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

CI restarted

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Restarted CI (the last run did not complete neither reported here).

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 23, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 23, 2019
@0xc0170 0xc0170 merged commit b9a6251 into ARMmbed:master Aug 23, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/jenkins/pr-merge Manually fixed status
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(+8 bytes) RAM(-84 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 9544 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 8464B.
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.