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

Initialize platform in trng test #9493

Merged
merged 3 commits into from Feb 19, 2019

Conversation

Projects
None yet
@RonEld
Copy link
Contributor

commented Jan 24, 2019

Description

Add calls to mbedtls_platform_setup() and
mbedtls_platform_terminate() to the trng greentea test, to
initialize the hardware acceleration engines, in some platforms.
However, this involves hal test including Mbed TLS API, which I am not sure is architecturally correct. On the other hand, mbedtls_platform_setup() is intended for initializing the HW acceleration engines, where needed.

This should fix IOTPART-6810

@ashok-rao Could you please check that this resolves the issue you encountered on LAIRD BL654, and if it does, please return the usage of Cryptocell in that library?

Pull request type

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

Reviewers

@ARMmbed/mbed-os-crypto

@RonEld RonEld force-pushed the RonEld:add_platform_initialization_in_trng_test branch Jan 24, 2019

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jan 24, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

@dannybenor

This comment has been minimized.

Copy link

commented Jan 24, 2019

@Patater are there any other initialization functions required? @bulislaw Should this and other init functions called at some global part of mbedos?

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

Note:

  • psa_crypto_init() is already called inside inject_entropy_for_psa(), in line 85, so no other psa crypto initialization function required.
@dannybenor

This comment has been minimized.

Copy link

commented Jan 27, 2019

@RonEld this function should not be called in the test, but in mbed OS. We need the test to make sure mbed OS will work. Please move the call to a platform specific initialization code

@dannybenor
Copy link

left a comment

The call to mbedtls_platform_setup should be part of mbed OS and not the test

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

@dannybenor Although I agree with you, this is the design of the function, and of Mbed OS design.
It was original in the Mbed OS boot, but after long discussions, it was decided it should be called by the applications requesting Mbed TLS \ crypto operations.
@bulislaw please confirm that this is still the case

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

More information in IOTSSL-1576

@dannybenor

This comment has been minimized.

Copy link

commented Jan 27, 2019

@bulislaw need your input here. We have functionality in mbedOS that uses crypto. Adding a new target should not brake it. Adding the initialization to the test is not helping. If the function mbedtls_platform_terminate() is required for some targets, code should not work for any target if the function is not called.

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

In current design, modules in Mbed OS, that use Mbed TLS \ crypto, should call mbedtls_platform_setup() and mbedtls_platform_terminate(), like BLE does.
We have added a reference counter in the platform context, that will not actually terminate the platform hw acceleration, if still used.
See #7099

@dannybenor

This comment has been minimized.

Copy link

commented Jan 28, 2019

@liatvindzberg @erezlandau FYI
@RonEld @sbutcher-arm why the function mbedtls_platform_setup() is not called inside psa_crypto_init()?
The implementation of devicekey and securestore passed a lot of review of the crypto team and nobody pointed that we need to call the function mbedtls_platform_setup(). That means we are setting a big trap for any developer.
The parameter mbedtls_platform_context looks like something not required if no other TLS function uses it.
https://context.reverso.net/translation/hebrew-english/%D7%91%D7%90%D7%A8%D7%96%D7%99%D7%9D+%D7%A0%D7%A4%D7%9C%D7%94+%D7%A9%D7%9C%D7%94%D7%91%D7%AA

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

why the function mbedtls_platform_setup() is not called inside psa_crypto_init()?

This is in discussion in ARMmbed/mbed-crypto#24 . IMHO, once PSA will be fully integrated within Mbed OS, mbedtls_platfor_setup() will be deprecated, or at least not handle the crypto operations. This function precedes PSA crypto.

The implementation of devicekey and securestore passed a lot of review of the crypto team and nobody pointed that we need to call the function mbedtls_platform_setup().

For PSA point of view, mbedtls_platform_setup() should not be called, and as I mentioned in previous answer, it should probably not be used once all crypto operations be called in SA crypto. Until that happens, mbedtls_platform_setup() and mbedtls_platform_terminate() should be called as well, and I think it's a mistake \ overlook that you weren't been pointed out on that.

@0xc0170 0xc0170 requested a review from bulislaw Jan 28, 2019

@bulislaw

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

That's a breaking change isn't it? As in current use of TLS/crypto won't work without this change? Generally speaking, my personal opinion is, that we shouldn't initialise modules globally unless they are used by the OS itself. In this case user is well aware that he/she will need TLS/crypto so they should initialise it. That being said, it's a breaking change, is there a way for us to hide it in existing calls so that we are backward compatible? Could public API initialise the modules if they weren't initialised before?

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

I wouldn't say it's a breaking change. but a bug fix.
Without this fix, platforms that implemented their alternative platform initialization function( at the moment only NRF52840 AFAIK) will simply fail on this test. Other platforms will use the default implementation of mbedtls_platform_setup() which is empty in Mbed TLS.
Ever since this API was introduced, in mbedtls-2.7 and then mbed-os-5.8, this function was recomended to be used whenever a cryptographic operation is used. See https://os.mbed.com/docs/mbed-os/v5.11/porting/hardware-accelerated-crypto.html#mbed-tls-platform-context and IOTSSL-1576

That being said, it's a breaking change, is there a way for us to hide it in existing calls so that we are backward compatible? Could public API initialise the modules if they weren't initialised before?

As mentioned, I don't think it's a breaking change, but a bug fix, and we are backwards compatible. The NRF52480 didn't work before on this test, since the Cryptocell library porting was introduced in #6794
and now it does( It was a breaking change in that PR though). As for initialize the modules if they weren't initialized before, well, since #7099, there is a refernce counter in the platform context, so it is posisble, however, this requires all the Mbed TLS APIto call this function, which means changing Mbed TLS for this transition to mbed crypto stage, which I think is redundant. note that in ARMmbed/mbed-crypto#24 we are discussing how to integrate this function with psa_crypto_init(), but until this is done, and psa crypto is fully incorporated in Mbed OS, I think this change is needed.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

This "simple" fix seems to be difficult to understand the implications. @RonEld Where is this captured (any docs) that these 2 functions need to be invoked when crypto used ? How we can proceed further?

@ARMmbed/mbed-os-crypto Please review

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@yanesca

yanesca approved these changes Feb 7, 2019

Copy link
Contributor

left a comment

The implications of this fix:

  • The test will pass for every properly functioning and integrated TRNG
  • The test won't catch that some part of the OS uses the TRNG but does not call mbedtls_platform_setup() before.
  • We can't see yet how this affects future plans (see ARMmbed/mbed-crypto#24)

If we are unhappy with the fix we have two other options:

  1. Leave it as it is and have correct implementations fail the test suite.
  2. Move mbedtls_platform_setup() back to Mbed OS boot.

This fix is a clear improvement to option 1 and I can see why we don't want option 2.

That being said this test does not catch that a part of the OS uses TRNG but does not call mbedtls_platform_setup(). This failure can potentially be silent and still catastrophic for security. Optimally we should identify every part of the OS that uses TRNG and add a test for that module that makes sure that the platform initialisation has been done where it is needed.

I am approving since this fix is a clear improvement, but it would be best to cover the gaps in the test suite. If that is out of scope for this PR, then we should raise an issue about it. (My approval is conditional on fixing the gap or raising an issue for it.)

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Thanks @yanesca ! @dannybenor Do you agree with the above statement about options and approval for this PR?

@dannybenor

This comment has been minimized.

Copy link

commented Feb 7, 2019

O do not agree with adding the call to the test.
We need to find a better way to integrate into mbed os

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

From what you are saying, you also do not agree of having psa_crypto_init in the test, as it is basically intended to do the same thing, once crypto will be fully integrated

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

Note: Having the initialization done at boot time, causing the hw acceleration engine to always be "on", might have side effects of high power consumption. see #9372 for example.

@sbutcher-arm

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

@RonEld @sbutcher-arm why the function mbedtls_platform_setup() is not called inside psa_crypto_init()?
The implementation of devicekey and securestore passed a lot of review of the crypto team and nobody pointed that we need to call the function mbedtls_platform_setup(). That means we are setting a big trap for any developer.

At the moment we're moving the crypto APIs from Mbed TLS to PSA. We haven't completed that work yet, and it's still in transition. When we have completed that work, I'd expect users of only Crypto APIs to not need to call mbedtls_platform_setup(). That's not true yet, but it should become true when we've finished the work.

cc: @Patater

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@RonEld Can you talk to @dannybenor to resolve this please? We shall find a path forward or close this PR

@dannybenor

This comment has been minimized.

Copy link

commented Feb 11, 2019

@RonEld This PR can be enhanced to make sure all uses of crypto in mbed OS will work, not only this test.
Will be good also to make sure it is clear in the documentation

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Unittest failures are related, please review

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Feb 15, 2019

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

I have fixed failures in unittests.

Once CI completes successfully, I will squash the commits to dcc194f

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Feb 18, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

CI started

RonEld added some commits Jan 24, 2019

Initialize platform in trng test
Add calls to `mbedtls_platform_setup()` and
`mbedtls_platform_terminate()` to the trng greentea test, to
initialize the hardware acceleration engines, in some platforms.
Add mbedtls platform setup and teardown to modules
Add calls to `mbedtls_platform_setup()`
and `mbedtls_platform_teardown()` to all modules and tests using Mbed TLS.
Use a mutex in platform setup \ teardown functions
Use a singleton Mutex in platforms_alt functions, to be shared
with the trng function, to save RAM. Rename `platform_alt.c`
to `platform_alt.cpp` as the mutex is in a `singletonPtr`
template class.
@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

@0xc0170 Do you want me to squash the CI fixes, or is there a need for reviewing them before?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

As soon as CI finishes, the log could be cleaned. Let us know once done

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 18, 2019

Test run: SUCCESS

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

@RonEld This shall be ready for integration, do you want to squash and we rerun ci?

@RonEld RonEld force-pushed the RonEld:add_platform_initialization_in_trng_test branch to 77f9faf Feb 18, 2019

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Feb 18, 2019

@RonEld

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

@0xc0170 done

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Feb 18, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 18, 2019

Test run: FAILED

Summary: 1 of 12 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

I restarted CI to get new results

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Feb 19, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

@RonEld all green, reviewed, this shall be now ready for integration

@cmonr cmonr merged commit feae56e into ARMmbed:master Feb 19, 2019

27 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-ARMC6 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 Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10094 cycles (-387 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.