Initialize platform in trng test #9493

RonEld · 2019-01-24T16:38:04Z

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

ciarmcom · 2019-01-24T18:00:34Z

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

dannybenor · 2019-01-24T18:38:15Z

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

RonEld · 2019-01-27T09:40:39Z

Note:

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

dannybenor · 2019-01-27T09:55:52Z

@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

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

RonEld · 2019-01-27T10:03:15Z

@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 · 2019-01-27T10:07:30Z

More information in IOTSSL-1576

dannybenor · 2019-01-27T12:18:49Z

@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 · 2019-01-27T12:45:08Z

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 · 2019-01-28T08:01:32Z

@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 · 2019-01-28T08:12:15Z

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.

bulislaw · 2019-02-05T16:51:56Z

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 · 2019-02-06T07:30:14Z

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 · 2019-02-07T09:10:51Z

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 · 2019-02-07T09:15:53Z

@0xc0170 As mentioned in previous comment, it is described in https://os.mbed.com/docs/mbed-os/v5.11/porting/hardware-accelerated-crypto.html#mbed-tls-platform-context

RonEld · 2019-02-07T09:17:42Z

Also in the function description: https://github.com/ARMmbed/mbed-os/blob/master/features/mbedtls/inc/mbedtls/platform.h#L335

yanesca

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 mbedtls_platform_setup and psa_crypto_init mbed-crypto#24)

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

Leave it as it is and have correct implementations fail the test suite.
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 · 2019-02-07T11:39:49Z

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

dannybenor · 2019-02-07T14:54:14Z

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

RonEld · 2019-02-07T16:00:53Z

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 · 2019-02-10T11:26:55Z

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.

simonbutcher · 2019-02-11T14:18:24Z

@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 · 2019-02-11T14:39:11Z

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

dannybenor · 2019-02-11T14:51:08Z

@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

mbed-ci · 2019-02-15T10:34:17Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

0xc0170 · 2019-02-15T10:50:14Z

Unittest failures are related, please review

RonEld · 2019-02-18T09:02:35Z

I have fixed failures in unittests.

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

0xc0170 · 2019-02-18T09:19:55Z

CI started

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 calls to `mbedtls_platform_setup()` and `mbedtls_platform_teardown()` to all modules and tests using Mbed TLS.

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 · 2019-02-18T10:10:31Z

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

0xc0170 · 2019-02-18T10:39:18Z

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

mbed-ci · 2019-02-18T11:36:44Z

Test run: SUCCESS

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

0xc0170 · 2019-02-18T11:44:12Z

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

RonEld · 2019-02-18T11:46:36Z

@0xc0170 done

0xc0170 · 2019-02-18T12:16:39Z

CI restarted

mbed-ci · 2019-02-18T15:12:20Z

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 · 2019-02-19T09:49:11Z

I restarted CI to get new results

0xc0170 · 2019-02-19T11:27:27Z

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

RonEld force-pushed the add_platform_initialization_in_trng_test branch from 20ad736 to 7dfe3e4 Compare January 24, 2019 16:52

ciarmcom requested review from a team January 24, 2019 18:00

ciarmcom added the needs: review label Jan 24, 2019

dannybenor suggested changes Jan 27, 2019

View reviewed changes

0xc0170 requested a review from bulislaw January 28, 2019 09:15

yanesca approved these changes Feb 7, 2019

View reviewed changes

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

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

Ron Eldor added 3 commits February 18, 2019 11:43

Initialize platform in trng test

22bf92b

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

c94b586

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

77f9faf

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 force-pushed the add_platform_initialization_in_trng_test branch from a70a563 to 77f9faf Compare February 18, 2019 11:45

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

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

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

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

cmonr removed the ready for merge label Feb 19, 2019

RonEld mentioned this pull request Mar 3, 2019

SecureStore and Cell with nRF52840 #9880

Closed

Initialize platform in trng test #9493

Initialize platform in trng test #9493

Uh oh!

Conversation

RonEld commented Jan 24, 2019

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Jan 24, 2019

Uh oh!

dannybenor commented Jan 24, 2019

Uh oh!

RonEld commented Jan 27, 2019

Uh oh!

dannybenor commented Jan 27, 2019

Uh oh!

dannybenor left a comment

Choose a reason for hiding this comment

Uh oh!

RonEld commented Jan 27, 2019

Uh oh!

RonEld commented Jan 27, 2019

Uh oh!

dannybenor commented Jan 27, 2019

Uh oh!

RonEld commented Jan 27, 2019

Uh oh!

dannybenor commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonEld commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bulislaw commented Feb 5, 2019

Uh oh!

RonEld commented Feb 6, 2019

Uh oh!

0xc0170 commented Feb 7, 2019

Uh oh!

RonEld commented Feb 7, 2019

Uh oh!

RonEld commented Feb 7, 2019

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 7, 2019

Uh oh!

dannybenor commented Feb 7, 2019

Uh oh!

RonEld commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonEld commented Feb 10, 2019

Uh oh!

simonbutcher commented Feb 11, 2019

Uh oh!

0xc0170 commented Feb 11, 2019

Uh oh!

dannybenor commented Feb 11, 2019

Uh oh!

mbed-ci commented Feb 15, 2019

Test run: FAILED

Uh oh!

0xc0170 commented Feb 15, 2019

Uh oh!

RonEld commented Feb 18, 2019

Uh oh!

0xc0170 commented Feb 18, 2019

Uh oh!

RonEld commented Feb 18, 2019

Uh oh!

0xc0170 commented Feb 18, 2019

Uh oh!

mbed-ci commented Feb 18, 2019

Test run: SUCCESS

Uh oh!

0xc0170 commented Feb 18, 2019

Uh oh!

dannybenor commented Jan 28, 2019 •

edited

Loading

RonEld commented Jan 28, 2019 •

edited

Loading

RonEld commented Feb 7, 2019 •

edited

Loading