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.

Sign up for GitHub

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

Fix PSA crypto partition and tests #8935

Merged
merged 5 commits into from
Dec 6, 2018
Merged

Fix PSA crypto partition and tests #8935

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c3bd6f1
Fix PSA crypto partiotion and tests
Dec 2, 2018
d614fd8
The inject test can run also on single core CPU's such as K64F
Dec 4, 2018
fa5c96c
Fix includes
Dec 4, 2018
86c7a59
Basic test can run on single v7's
Dec 4, 2018
51a462a
Use entropy injection on any NVseed platform or PSA dual-core/v8
Dec 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix PSA crypto partiotion and tests 
* Styling
* Correct error codes on failing connection
* Add panics where needed
* correct skip defines
* Fix psa_spm_init_refence_counter bug
  • Loading branch information
Oren Cohen authored and Alexander Zilberkant committed Dec 4, 2018
commit c3bd6f12d2eb02bd8d1b5beba5439dd6f3bdeae3
18 changes: 9 additions & 9 deletions TESTS/psa/crypto_init/COMPONENT_SPE/psa_setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,21 @@
#include "spm_internal.h"
#include "handles_manager.h"
#include "cmsis.h"
#include "psa_psa_f_partition.h"
#include "psa_crypto_srv_partition.h"
#include "psa_its_partition.h"

extern const uint32_t psa_f_external_sids[4];
extern const uint32_t crypto_srv_external_sids[4];

spm_partition_t g_partitions[2] = {
{
.partition_id = PSA_F_ID,
.partition_id = CRYPTO_SRV_ID,
.thread_id = 0,
.flags_rot_srv = PSA_F_WAIT_ANY_SID_MSK,
.flags_rot_srv = CRYPTO_SRV_WAIT_ANY_SID_MSK,
.flags_interrupts = 0,
.rot_services = NULL,
.rot_services_count = PSA_F_ROT_SRV_COUNT,
.extern_sids = psa_f_external_sids,
.extern_sids_count = PSA_F_EXT_ROT_SRV_COUNT,
.rot_services_count = CRYPTO_SRV_ROT_SRV_COUNT,
.extern_sids = crypto_srv_external_sids,
.extern_sids_count = CRYPTO_SRV_EXT_ROT_SRV_COUNT,
.irq_mapper = NULL,
},
{
Expand All @@ -65,7 +65,7 @@ const mem_region_t *mem_regions = NULL;
const uint32_t mem_region_count = 0;

// forward declaration of partition initializers
void psa_f_init(spm_partition_t *partition);
void crypto_srv_init(spm_partition_t *partition);
void its_init(spm_partition_t *partition);

uint32_t init_partitions(spm_partition_t **partitions)
Expand All @@ -74,7 +74,7 @@ uint32_t init_partitions(spm_partition_t **partitions)
SPM_PANIC("partitions is NULL!\n");
}

psa_f_init(&(g_partitions[0]));
crypto_srv_init(&(g_partitions[0]));
its_init(&(g_partitions[1]));

*partitions = g_partitions;
Expand Down
34 changes: 19 additions & 15 deletions TESTS/psa/crypto_init/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :) .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#error [NOT_SUPPORTED] Mbed SPM Crypto is OFF - skipping.
#endif // TARGET_PSA

Expand Down Expand Up @@ -50,22 +50,20 @@ utest::v1::status_t greentea_test_setup(const size_t number_of_cases)
static void check_multi_crypto_init_deinit()
{
uint8_t output[TEST_RANDOM_SIZE] = {0};
uint8_t seed[MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE] = {0};
/* inject some a seed for test*/
for (int i; i < MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE; ++i) {
seed[i] = i;
}
/* don't really care if this succeed this is just to make crypto init pass*/
mbedtls_psa_inject_entropy(seed, MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE);

psa_status_t status = psa_crypto_init();
TEST_ASSERT_EQUAL(PSA_SUCCESS, status);

status = psa_crypto_init();
TEST_ASSERT_EQUAL(PSA_SUCCESS, status);

status = psa_generate_random(output, sizeof(output));
TEST_ASSERT_NOT_EQUAL(PSA_ERROR_BAD_STATE, status);

mbedtls_psa_crypto_free();
status = psa_generate_random(output, sizeof(output));
TEST_ASSERT_NOT_EQUAL(PSA_ERROR_BAD_STATE, status);

mbedtls_psa_crypto_free();
status = psa_generate_random(output, sizeof(output));
TEST_ASSERT_EQUAL(PSA_ERROR_BAD_STATE, status);
Expand All @@ -75,19 +73,17 @@ static void check_crypto_init_deinit()
{
psa_status_t status;
uint8_t output[TEST_RANDOM_SIZE] = {0};
uint8_t seed[MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE] = {0};
/* inject some a seed for test*/
for (int i; i < MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE; ++i) {
seed[i] = i;
}
/* don't really care if this succeed this is just to make crypto init pass*/
mbedtls_psa_inject_entropy(seed, MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE);

// Should fail as init is required first
status = psa_generate_random(output, sizeof(output));
TEST_ASSERT_EQUAL(PSA_ERROR_BAD_STATE, status);

status = psa_crypto_init();
TEST_ASSERT_EQUAL(PSA_SUCCESS, status);

status = psa_generate_random(output, sizeof(output));
TEST_ASSERT_NOT_EQUAL(PSA_ERROR_BAD_STATE, status);

mbedtls_psa_crypto_free();
status = psa_generate_random(output, sizeof(output));
TEST_ASSERT_EQUAL(PSA_ERROR_BAD_STATE, status);
Expand All @@ -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*/
Copy link
Contributor

@Patater Patater Dec 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (int i = 0; i < MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE; ++i) {
seed[i] = i;
}

/* don't really care if this succeed this is just to make crypto init pass*/
mbedtls_psa_inject_entropy(seed, MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE);
return !Harness::run(specification);
}
18 changes: 9 additions & 9 deletions TESTS/psa/entropy_inject/COMPONENT_SPE/psa_setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@
#include "handles_manager.h"
#include "cmsis.h"
#include "psa_test_its_reset_partition.h"
#include "psa_psa_f_partition.h"
#include "psa_crypto_srv_partition.h"
#include "psa_its_partition.h"

extern const uint32_t psa_f_external_sids[4];
extern const uint32_t crypto_srv_external_sids[4];

spm_partition_t g_partitions[3] = {
{
Expand All @@ -46,14 +46,14 @@ spm_partition_t g_partitions[3] = {
.irq_mapper = NULL,
},
{
.partition_id = PSA_F_ID,
.partition_id = CRYPTO_SRV_ID,
.thread_id = 0,
.flags_rot_srv = PSA_F_WAIT_ANY_SID_MSK,
.flags_rot_srv = CRYPTO_SRV_WAIT_ANY_SID_MSK,
.flags_interrupts = 0,
.rot_services = NULL,
.rot_services_count = PSA_F_ROT_SRV_COUNT,
.extern_sids = psa_f_external_sids,
.extern_sids_count = PSA_F_EXT_ROT_SRV_COUNT,
.rot_services_count = CRYPTO_SRV_ROT_SRV_COUNT,
.extern_sids = crypto_srv_external_sids,
.extern_sids_count = CRYPTO_SRV_EXT_ROT_SRV_COUNT,
.irq_mapper = NULL,
},
{
Expand All @@ -78,7 +78,7 @@ const uint32_t mem_region_count = 0;

// forward declaration of partition initializers
void test_its_reset_init(spm_partition_t *partition);
void psa_f_init(spm_partition_t *partition);
void crypto_srv_init(spm_partition_t *partition);
void its_init(spm_partition_t *partition);

uint32_t init_partitions(spm_partition_t **partitions)
Expand All @@ -88,7 +88,7 @@ uint32_t init_partitions(spm_partition_t **partitions)
}

test_its_reset_init(&(g_partitions[0]));
psa_f_init(&(g_partitions[1]));
crypto_srv_init(&(g_partitions[1]));
its_init(&(g_partitions[2]));

*partitions = g_partitions;
Expand Down
22 changes: 17 additions & 5 deletions TESTS/psa/entropy_inject/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed inject needs to run also on k64f

#error [NOT_SUPPORTED] PSA entropy injection tests can run only on PSA-enabled targets.
#endif // TARGET_PSA

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static maybe


void validate_entropy_seed_injection(int seed_length_a,
int expected_status_a,
Expand Down Expand Up @@ -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 < sizeof(seed); ++i) {
seed[i] = i;
}

if (mbedtls_psa_inject_entropy(seed, MBEDTLS_ENTROPY_MAX_SEED_SIZE) == PSA_ERROR_NOT_SUPPORTED) {
skip_tests = true;
}

return greentea_test_setup_handler(number_of_cases);
}

static void injection_small_good()
{
TEST_SKIP_UNLESS(!skip_tests);
validate_entropy_seed_injection(MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE, PSA_SUCCESS, MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE, PSA_ERROR_NOT_PERMITTED);
}

static void injection_big_good()
{
TEST_SKIP_UNLESS(!skip_tests);
validate_entropy_seed_injection(MBEDTLS_ENTROPY_MAX_SEED_SIZE, PSA_SUCCESS, MBEDTLS_ENTROPY_MAX_SEED_SIZE, PSA_ERROR_NOT_PERMITTED);
}

static void injection_too_small()
{
TEST_SKIP_UNLESS(!skip_tests);
validate_entropy_seed_injection((MBEDTLS_ENTROPY_MIN_PLATFORM - 1), PSA_ERROR_INVALID_ARGUMENT, MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE, PSA_SUCCESS);
}

static void injection_too_big()
{
TEST_SKIP_UNLESS(!skip_tests);
validate_entropy_seed_injection((MBEDTLS_ENTROPY_MAX_SEED_SIZE + 1), PSA_ERROR_INVALID_ARGUMENT, MBEDTLS_ENTROPY_MAX_SEED_SIZE, PSA_SUCCESS);
}

static void injection_and_init_deinit()
{
TEST_SKIP_UNLESS(!skip_tests);
run_entropy_inject_with_crypto_init();
}

Expand All @@ -129,10 +145,6 @@ utest::v1::status_t case_setup_handler(const Case *const source, const size_t in
psa_status_t status;
status = test_psa_its_reset();
TEST_ASSERT_EQUAL(PSA_ITS_SUCCESS, status);
/* fill seed in some data */
for (size_t i = 0; i < MBEDTLS_ENTROPY_MAX_SEED_SIZE + 2; ++i) {
seed[i] = i;
}
return greentea_case_setup_handler(source, index_of_case);
}

Expand Down
18 changes: 9 additions & 9 deletions TESTS/psa/prot_internal_storage/COMPONENT_SPE/psa_setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@
#include "handles_manager.h"
#include "cmsis.h"
#include "psa_test_its_reset_partition.h"
#include "psa_psa_f_partition.h"
#include "psa_crypto_srv_partition.h"
#include "psa_its_partition.h"

extern const uint32_t psa_f_external_sids[4];
extern const uint32_t crypto_srv_external_sids[4];

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.thread_id = 0,
.flags_rot_srv = PSA_F_WAIT_ANY_SID_MSK,
.flags_rot_srv = CRYPTO_SRV_WAIT_ANY_SID_MSK,
.flags_interrupts = 0,
.rot_services = NULL,
.rot_services_count = PSA_F_ROT_SRV_COUNT,
.extern_sids = psa_f_external_sids,
.extern_sids_count = PSA_F_EXT_ROT_SRV_COUNT,
.rot_services_count = CRYPTO_SRV_ROT_SRV_COUNT,
.extern_sids = crypto_srv_external_sids,
.extern_sids_count = CRYPTO_SRV_EXT_ROT_SRV_COUNT,
.irq_mapper = NULL,
},
{
Expand All @@ -78,7 +78,7 @@ const uint32_t mem_region_count = 0;

// forward declaration of partition initializers
void test_its_reset_init(spm_partition_t *partition);
void psa_f_init(spm_partition_t *partition);
void crypto_srv_init(spm_partition_t *partition);
void its_init(spm_partition_t *partition);

uint32_t init_partitions(spm_partition_t **partitions)
Expand All @@ -88,7 +88,7 @@ uint32_t init_partitions(spm_partition_t **partitions)
}

test_its_reset_init(&(g_partitions[0]));
psa_f_init(&(g_partitions[1]));
crypto_srv_init(&(g_partitions[1]));
its_init(&(g_partitions[2]));

*partitions = g_partitions;
Expand Down
18 changes: 9 additions & 9 deletions TESTS/psa/spm_client/COMPONENT_SPE/psa_setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@
#include "handles_manager.h"
#include "cmsis.h"
#include "psa_client_tests_part1_partition.h"
#include "psa_psa_f_partition.h"
#include "psa_crypto_srv_partition.h"
#include "psa_its_partition.h"

extern const uint32_t psa_f_external_sids[4];
extern const uint32_t crypto_srv_external_sids[4];

spm_partition_t g_partitions[3] = {
{
Expand All @@ -46,14 +46,14 @@ spm_partition_t g_partitions[3] = {
.irq_mapper = NULL,
},
{
.partition_id = PSA_F_ID,
.partition_id = CRYPTO_SRV_ID,
.thread_id = 0,
.flags_rot_srv = PSA_F_WAIT_ANY_SID_MSK,
.flags_rot_srv = CRYPTO_SRV_WAIT_ANY_SID_MSK,
.flags_interrupts = 0,
.rot_services = NULL,
.rot_services_count = PSA_F_ROT_SRV_COUNT,
.extern_sids = psa_f_external_sids,
.extern_sids_count = PSA_F_EXT_ROT_SRV_COUNT,
.rot_services_count = CRYPTO_SRV_ROT_SRV_COUNT,
.extern_sids = crypto_srv_external_sids,
.extern_sids_count = CRYPTO_SRV_EXT_ROT_SRV_COUNT,
.irq_mapper = NULL,
},
{
Expand All @@ -78,7 +78,7 @@ const uint32_t mem_region_count = 0;

// forward declaration of partition initializers
void client_tests_part1_init(spm_partition_t *partition);
void psa_f_init(spm_partition_t *partition);
void crypto_srv_init(spm_partition_t *partition);
void its_init(spm_partition_t *partition);

uint32_t init_partitions(spm_partition_t **partitions)
Expand All @@ -88,7 +88,7 @@ uint32_t init_partitions(spm_partition_t **partitions)
}

client_tests_part1_init(&(g_partitions[0]));
psa_f_init(&(g_partitions[1]));
crypto_srv_init(&(g_partitions[1]));
its_init(&(g_partitions[2]));

*partitions = g_partitions;
Expand Down
Loading