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 CC 310 sha 512 driver #10913

Merged
merged 3 commits into from Aug 1, 2019

Conversation

@RonEld
Copy link
Contributor

commented Jun 27, 2019

Description

Port the cc310 SHA512 driver, even though it is sw implementation.
Because the linker could not remove the cc310 sha512 implementation,
there was duplicate implementation of SHA512, without enabling
the sha512 alternative implementation.

There is a minor performance degradation (284 KB/s instead of 322 KB/s), however improvement in memory usage.
According to the generated map file, using GCC_ARM:
ROM: 81880 bytes smaller
RAM: 48 bytes smaller
Downside s the SHA384 is not supported with this driver introduction anymore.

Pull request type

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

Reviewers

@ARMmbed/mbed-os-crypto

Release Notes

SHA384 is not supported, returning MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED

Port the cc310 SHA512 driver, even though it is sw implementation.
Because the linker could not remove the cc310 sha512 implementation,
there was duplicate implementation of SHA512, without enabling
the sha512 alternative implementation.
@loverdeg-ep

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

81880 bytes less in flash isn't a mistype?

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

It looked to me strange as well, however, looking at the generated map file with GCC)ARM toolchain I see ROM value:
With Mbed TLS sha512: 251084 bytes ( Mbed TLS alone is 113320)
With CC 310 SHA 512 impl.: 169204 ( Mbed TLS alone 63324)

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

@loverdeg-ep

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

If this works out, you have made our summer more interesting. Thanks, keep the optimizations coming.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

If this works out, you have made our summer more interesting. Thanks, keep the optimizations coming.

🌞 🍹

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

looking at the generated map file, I think the numbers I found are incorrect and not realted to this change, unfortunately. There was probably some environment issues causeing the differnce.
I made a new map file and in this file, ROM size of sha512.o when using Mbed TLS sha 512 is 4440 bytes, while when using cc310 it is 62 bytes, so the decrease is not as big as 82 KB.
The size of ROM in latest measurement is 172428 bytes ( and not 251084 as I got in previous measurement :( ), so the ROM improvement is probably only 3224 bytes, and no change in RAM

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@ARMmbed/mbed-os-crypto Please review

@gilles-peskine-arm

This comment has been minimized.

Copy link

commented Jul 1, 2019

the SHA384 is not supported with this driver introduction anymore

Is this acceptable? Note that SHA-384 is used in the real world, especially for HTTPS on the web. I think most clients and servers also accept SHA-256 but not SHA-512. The reason they use SHA-384 in preference to SHA-256 is that it's faster on 64-bit machines, but they normally also support SHA-256 to interoperate with 32-bit endpoints or to save on the transmitted data size. Even so, losing SHA-384 can affect applications and should at least be clearly documented.

@Patater

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

the SHA384 is not supported with this driver introduction anymore

Is this acceptable? Note that SHA-384 is used in the real world, especially for HTTPS on the web. I think most clients and servers also accept SHA-256 but not SHA-512. The reason they use SHA-384 in preference to SHA-256 is that it's faster on 64-bit machines, but they normally also support SHA-256 to interoperate with 32-bit endpoints or to save on the transmitted data size. Even so, losing SHA-384 can affect applications and should at least be clearly documented.

It's a limitation of opting in to hardware acceleration on this platform, which the system integrator would need to accept as valid for their expected use cases. We are making it opt-in, right? FEATURE_CRYPTOCELL310 is an opt-in thing?

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

We are making it opt-in, right? FEATURE_CRYPTOCELL310 is an opt-in thing?

It is defined by default in targets.json for MCU_NRF52840 target.
I guess it can be opted out in mbed_app.json, but not sure on that, but this will remove all hw accelerated algorithms.
However, one can just undefine MBEDTLS_SHA512_ALT in the user config file if they must have SHA384

@bulislaw

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

I'd rather avoid braking changes and do it as an option. We can break at next major version.

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

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_SHA512_ALT from mbedtls_device.h and users that want this feature will need to add this definition in mbed_app.json

Have the alternative sha512 undefined by default,
in order not to break backwards compatability.
@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

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

@artokin

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

@ARMmbed/mbed-os-crypto , please add your review comments!

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 19, 2019

Test run: SUCCESS

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

int mbedtls_internal_sha512_process( mbedtls_sha512_context *ctx,
const unsigned char data[128] )
{
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );

This comment has been minimized.

Copy link
@Patater

Patater Jul 19, 2019

Contributor

Will this break applications that were using software SHA-512 previously? Should we return non-error here?

This comment has been minimized.

Copy link
@RonEld

RonEld Jul 21, 2019

Author Contributor

no.
This function is called only by our internal test.
Please look at #8728 and discuss @yanesca for more information

void mbedtls_sha512_init( mbedtls_sha512_context *ctx )
{
memset( ctx, 0, sizeof( mbedtls_sha512_context ) );

This comment has been minimized.

Copy link
@Patater

Patater Jul 19, 2019

Contributor

Remove extra newline here.

return ( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );
}
#endif //MBEDTLS_SHA512_ALT

This comment has been minimized.

Copy link
@Patater

Patater Jul 19, 2019

Contributor

Remove extra newline here

Remove redundant extra lines.
@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@Patater Thank you for your comments! I have addressed them.

Copy link
Contributor

left a comment

LGTM

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@ARMmbed/mbed-os-maintainers Can we start the CI?

@evedon

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 27, 2019

Test run: SUCCESS

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

@SeppoTakalo SeppoTakalo merged commit 54d7d7e into ARMmbed:master Aug 1, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
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(+0 bytes) RAM(+0 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 8608 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 8448B.
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
You can’t perform that action at this time.