Fix PSA crypto partition and tests #8935

orenc17 · 2018-12-02T13:21:40Z

Description

Styling
Correct error codes on failing connection
Add panics where needed
correct skip defines
Fix psa_spm_init_refence_counter bug

Required by #8745

Pull request type

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

0xc0170 · 2018-12-03T11:09:18Z

Travis failure will be fixed on master soon (we will request rebase once done). See #8945

0xc0170 · 2018-12-03T14:05:25Z

Travis failure will be fixed on master soon (we will request rebase once done). See #8945

The fix is on master, if you rebase, it should allow travis to run

cmonr · 2018-12-03T16:46:49Z

@orenc17 Please take a look at the astyle test.

orenc17 · 2018-12-03T16:58:43Z

@cmonr Fixed astyle

cmonr · 2018-12-03T17:43:16Z

CI started

alzix · 2018-12-03T21:30:34Z

TESTS/psa/entropy_inject/main.cpp

@@ -83,31 +84,46 @@ utest::v1::status_t greentea_test_setup(const size_t number_of_cases)
 #ifndef NO_GREENTEA
    GREENTEA_SETUP(60, "default_auto");
 #endif
+
+    /* fill seed in some data */
+    for (size_t i = 0; i < MBEDTLS_ENTROPY_MAX_SEED_SIZE + 2; ++i) {


Suggested change

for (size_t i = 0; i < MBEDTLS_ENTROPY_MAX_SEED_SIZE + 2; ++i) {

for (size_t i = 0; i < sizeof(seed); ++i) {

alzix · 2018-12-03T21:33:31Z

TESTS/psa/entropy_inject/main.cpp

@@ -16,7 +16,7 @@
 * limitations under the License.
 */

-#if ((!defined(TARGET_PSA) || (!defined(COMPONENT_PSA_SRV_IPC)) && !defined(MBEDTLS_ENTROPY_NV_SEED)))
+#if (!defined(TARGET_PSA) || ((!defined(COMPONENT_PSA_SRV_IPC)) && !defined(MBEDTLS_ENTROPY_NV_SEED)))


please double check this expression - looks suspicious to me
too many parenthesis here

alzix · 2018-12-03T21:35:41Z

components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c

@@ -1067,7 +1067,7 @@ psa_status_t mbedtls_psa_inject_entropy(const unsigned char *seed,

    handle = psa_connect(PSA_ENTROPY_ID, MINOR_VER);
    if (handle <= 0) {
-        return (PSA_ERROR_COMMUNICATION_FAILURE);
+        return (PSA_ERROR_NOT_SUPPORTED);


psa_connect may fail due to resource exhaustion - does it really means that mbedtls_psa_inject_entropy is not supported?

Fixed, moved the not_supported logic to the call()

alzix · 2018-12-03T21:40:06Z

components/TARGET_PSA/services/crypto/COMPONENT_SPE/crypto_partition.c

@@ -0,0 +1,1376 @@
+// ---------------------------------- Includes ---------------------------------


i cannot review the changes in this file. It was renamed and modified and now appears as a new file.
can we split rename and modify commits so the changes will be reviewable?

alzix · 2018-12-03T21:43:06Z

CC @Patater please review

mbed-ci · 2018-12-04T02:00:23Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 1
Build artifacts
Build logs

alzix · 2018-12-04T11:29:50Z

rebased on master and renamed crypto partition from CRYPTO to CRYPTO_SRV for avoiding file names collision between auto-generated files and partition implementation file. This change will help us to review the proposed changes.

Patater · 2018-12-04T11:47:04Z

TESTS/psa/crypto_init/main.cpp

@@ -102,5 +98,13 @@ Specification specification(greentea_test_setup, cases, greentea_test_teardown_h

 int main()
 {
+    uint8_t seed[MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE] = {0};
+    /* inject some a seed for test*/


This is only needed if the seed hasn't been injected at the factory yet. Factory tool should do this, not our tests, right?

Our tests should be able to assume the device was manufactured correctly.

but then the tests won't work. since there is no prior factory injection for tests

tests are expected to work during OOB - so we must inject entropy in tests.
as a side effects tests executed on a device may destroy/corrupt factory values - but this is true for all the tests. KVStore tests will have same effect.

The board should have entropy injected before it gets inserted into the box at the factory. Then, OOB should work.

i tend to think of it as our internal tests. so we need to simulate the injection

For testing entropy injection itself, yes. For testing everything else, no. Tests should be able to assume a properly manufactured board.

Anyway, nothing to do with this PR. We'll sort this out later.

Patater · 2018-12-04T11:48:21Z

TESTS/psa/entropy_inject/main.cpp

@@ -16,7 +16,7 @@
 * limitations under the License.
 */

-#if ((!defined(TARGET_PSA) || (!defined(COMPONENT_PSA_SRV_IPC)) && !defined(MBEDTLS_ENTROPY_NV_SEED)))
+#if (!defined(TARGET_PSA) || !defined(COMPONENT_PSA_SRV_IPC))


Where do we want the entropy injection tests to run? I'm not sure these defines are right if we want it to run on both "K64F when MBEDTLS_ENTROPY_NV_SEED is configured" and on a "PSoC 6's NSPE when the PSoC 6's SPE has MBEDTLS_ENTROPY_NV_SEED configured".

@Patater ideally the test should run on dual-MCU(PSoC6) or v8-m systems
this is being taken care of by COMPONENT_PSA_SRV_IPC define which the K64F will never have

We probably want this test to keep working on K64F or other single v7-M targets.

indeed inject needs to run also on k64f

Patater · 2018-12-04T11:48:49Z

TESTS/psa/prot_internal_storage/COMPONENT_SPE/psa_setup.c

@@ -46,14 +46,14 @@ spm_partition_t g_partitions[3] = {
        .irq_mapper = NULL,
    },
    {
-        .partition_id = PSA_F_ID,
+        .partition_id = CRYPTO_SRV_ID,


Do we want to change PSA_F_ID to CRYPTO_ID everywhere, even in non-crypto tests? Maybe the storage tests should use a storage partition and not a crypto partition?

Do the storage tests need Mbed Crypto? Do they need a Crypto partition?

this is an auto-generated file which represents all the partitons defined in the system
specific for this test crypto and storage are the base partitions while its_reset is the additional user partition

and again this file will be compiled only for the secure side on dual-MCU(PSoC6) or v8-m systems only

OK, so Crypto partition should always be available from the SPE.

alekshex · 2018-12-04T14:02:06Z

TESTS/psa/entropy_inject/main.cpp

@@ -41,6 +41,7 @@
 using namespace utest::v1;

 uint8_t seed[MBEDTLS_ENTROPY_MAX_SEED_SIZE + 2] = {0};
+bool skip_tests = false;


static maybe

Patater

So long as the tests work for K64F and PSoC 6, I'm happy with these changes.

Patater · 2018-12-04T14:18:50Z

TESTS/psa/crypto_init/main.cpp

@@ -16,7 +16,7 @@
 * limitations under the License.
 */

-#if ((!defined(TARGET_PSA)) || (!defined(MBEDTLS_PSA_CRYPTO_C)) || (!defined(MBEDTLS_PSA_CRYPTO_SPM )))
+#if ((!defined(TARGET_PSA)) || (!defined(COMPONENT_PSA_SRV_IPC)) || (!defined(MBEDTLS_PSA_CRYPTO_C)))


These ifdefs prevent K64F from running these tests, don't they?

repetitive crypto init call will have a different return code on multi core comparing to a single core.
this test as it is written now can only pass on multi core - thus the ifdefs are correct.
you are welcome to update the test and enable it for single core targets in a separate PR.
This PR is a blocker for #8745 thus i suggest to avoid unnecessary last minute enchantments :) .

Alright. I had assumed when we added the tests for K64F that they were passing there. Seems the tests are in worse condition that expected, or the tests used to work and after this PR they break on K64F. So long as this PR isn't the cause of the tests not working on K64F, I'm fine with disabling the tests on K64F like this.

OK, looked into the old code more. This didn't work on K64F before. So, no regression.

alekshex · 2018-12-04T14:55:44Z

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c


-#if ( defined(MBEDTLS_ENTROPY_NV_SEED) && defined(MBEDTLS_PSA_HAS_ITS_IO) )
+    psa_get(PSA_ENTROPY_INJECT, &msg);


* Styling * Correct error codes on failing connection * Add panics where needed * correct skip defines * Fix psa_spm_init_refence_counter bug

alzix · 2018-12-04T15:10:48Z

i saw travis failures - rebased on master

0xc0170 · 2018-12-05T13:01:08Z

@orenc17 Please take a look at the TESTS-PSA-CRYPTO_INIT compilations.

I can see the branch was forced push. Did this address failures ? Is this ready for CI now?

orenc17 · 2018-12-05T13:39:00Z

@cmonr @0xc0170 i'm sorry, i thought i pushed that commit last night

cmonr · 2018-12-05T15:51:14Z

CI started

mbed-ci · 2018-12-05T16:40:29Z

Test run: FAILED

Summary: 3 of 7 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-IAR

cmonr · 2018-12-05T16:53:00Z

Will restart CI when able. There appears to be a parse error with the CI scripts.

alekla01 · 2018-12-05T17:45:25Z

CI restarted

orenc17 · 2018-12-05T18:10:32Z

@cmonr @alekla01 please restart the CI again to check the new commit please

cmonr · 2018-12-05T18:23:15Z

@orenc17 Will so as soon as Travis CI has completed its checks.

cmonr · 2018-12-05T18:45:42Z

CI started

mbed-ci · 2018-12-05T20:31:06Z

Test run: SUCCESS

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

mbed-ci · 2018-12-05T22:47:13Z

Test run: SUCCESS

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

cmonr

LGTM. A question about closing parenthesis, and should be good to merge after.

cmonr · 2018-12-06T01:49:42Z

TESTS/mbed-crypto/sanity/main.cpp

@@ -47,7 +47,7 @@ void inject_entropy()
    }
    mbedtls_psa_inject_entropy(seed, MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE);
 }
-#endif
+#endif // defined(MBEDTLS_ENTROPY_NV_SEED) || defined(COMPONENT_PSA_SRV_IPC)


+1 for commenting on closing #endif statements.

cmonr · 2018-12-06T01:53:18Z

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c

        case PSA_IPC_CALL: {
+#if (defined(MBEDTLS_ENTROPY_NV_SEED) && defined(MBEDTLS_PSA_HAS_ITS_IO))


Two )?

(In the end of the line)

@cmonr i hope the following graphic helps

#if (defined(MBEDTLS_ENTROPY_NV_SEED) && defined(MBEDTLS_PSA_HAS_ITS_IO)) | | | | || | ------------------------- ------------------------| | | ---------------------------------------------------------------------

That it does. My bad.

( 👍 Nice graphic! )

0xc0170 added the needs: review label Dec 3, 2018

0xc0170 requested a review from a team December 3, 2018 11:09

cmonr added needs: work and removed needs: review labels Dec 3, 2018

cmonr added needs: CI and removed needs: work labels Dec 3, 2018

alzix suggested changes Dec 3, 2018

View reviewed changes

cmonr added needs: work and removed needs: CI labels Dec 4, 2018

Patater reviewed Dec 4, 2018

View reviewed changes

alekshex reviewed Dec 4, 2018

View reviewed changes

Patater reviewed Dec 4, 2018

View reviewed changes

alekshex reviewed Dec 4, 2018

View reviewed changes

alekshex approved these changes Dec 4, 2018

View reviewed changes

Oren Cohen and others added 2 commits December 4, 2018 17:07

Fix PSA crypto partiotion and tests

c3bd6f1

* Styling * Correct error codes on failing connection * Add panics where needed * correct skip defines * Fix psa_spm_init_refence_counter bug

The inject test can run also on single core CPU's such as K64F

d614fd8

alzix approved these changes Dec 4, 2018

View reviewed changes

alzix mentioned this pull request Dec 4, 2018

Add new target future sequana PSA #8745

Merged

adbridge added release-version: 5.11.0-rc2 needs: CI and removed needs: work labels Dec 4, 2018

cmonr added needs: work and removed needs: CI labels Dec 4, 2018

adbridge added release-version: 5.11.0-rc3 and removed release-version: 5.11.0-rc2 labels Dec 5, 2018

Basic test can run on single v7's

86c7a59

0xc0170 added needs: CI and removed needs: work labels Dec 5, 2018

cmonr added needs: review needs: CI and removed needs: CI needs: review labels Dec 5, 2018

Use entropy injection on any NVseed platform or PSA dual-core/v8

51a462a

cmonr added ready for merge and removed needs: CI labels Dec 6, 2018

cmonr reviewed Dec 6, 2018

View reviewed changes

cmonr changed the title ~~Fix PSA crypto partiotion and tests~~ Fix PSA crypto partition and tests Dec 6, 2018

0xc0170 approved these changes Dec 6, 2018

View reviewed changes

0xc0170 merged commit 8301325 into ARMmbed:master Dec 6, 2018

0xc0170 removed the ready for merge label Dec 6, 2018

	for (size_t i = 0; i < MBEDTLS_ENTROPY_MAX_SEED_SIZE + 2; ++i) {
	for (size_t i = 0; i < sizeof(seed); ++i) {

		@@ -0,0 +1,1376 @@
		// ---------------------------------- Includes ---------------------------------


		#if ( defined(MBEDTLS_ENTROPY_NV_SEED) && defined(MBEDTLS_PSA_HAS_ITS_IO) )
		psa_get(PSA_ENTROPY_INJECT, &msg);

		case PSA_IPC_CALL: {
		#if (defined(MBEDTLS_ENTROPY_NV_SEED) && defined(MBEDTLS_PSA_HAS_ITS_IO))

Fix PSA crypto partition and tests #8935

Fix PSA crypto partition and tests #8935

Conversation

orenc17 commented Dec 2, 2018 • edited by adbridge Loading

Description

Pull request type

0xc0170 commented Dec 3, 2018

0xc0170 commented Dec 3, 2018

cmonr commented Dec 3, 2018

orenc17 commented Dec 3, 2018

cmonr commented Dec 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alzix commented Dec 3, 2018

mbed-ci commented Dec 4, 2018

alzix commented Dec 4, 2018

Patater Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patater left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alzix commented Dec 4, 2018

0xc0170 commented Dec 5, 2018

orenc17 commented Dec 5, 2018

cmonr commented Dec 5, 2018

mbed-ci commented Dec 5, 2018

cmonr commented Dec 5, 2018

alekla01 commented Dec 5, 2018

orenc17 commented Dec 5, 2018

cmonr commented Dec 5, 2018

cmonr commented Dec 5, 2018

mbed-ci commented Dec 5, 2018

mbed-ci commented Dec 5, 2018

cmonr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenc17 commented Dec 2, 2018 •

edited by adbridge

Loading

Patater Dec 4, 2018 •

edited

Loading