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

Jump to bottom

Crypto integration with SPM #9409

Closed
wants to merge 5 commits into from
Closed

Crypto integration with SPM #9409

wants to merge 5 commits into from

Conversation

itayzafrir
Copy link
Contributor

@itayzafrir itayzafrir commented Jan 17, 2019 •
edited
Loading

Description

Update to the crypto service due to crypto API changes and an additional new test.
This PR is dependent upon an import of crypto (with the key handles API changes) into mbed os.

PLEASE DO NOT MERGE (or attempt to run CI) until crypto has been re-imported.

This PR is now part of #9529.

Pull request type

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

Reviewers

@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested a review from a team January 17, 2019 14:00
@itayzafrir itayzafrir force-pushed the crypto-integration-with-spm branch 3 times, most recently from a67ab9f to 57bbeb7 Compare January 21, 2019 11:44
@0xc0170 0xc0170 requested a review from a team January 21, 2019 15:44
itayzafrir added 4 commits January 28, 2019 12:53
crypto: Add test of key handles
563701d 
Test key handles by adding a test to TESTS/mbed-crypto/sanity/main.cpp
crypto service: Implement IPC according to Mbed Crypto 1.0.0d1 API
3c10e5d 
1. Removed obsolete crypto APIs from IPC implementation.
2. Updated existing crypto APIs in IPC implementation.
3. Added new crypto APIs to IPC implemntation (except for psa_hash_clone).
crypto service: Implement function psa_hash_clone over IPC
eea6443
crypto: Add test of hash clone
dc54be2 
Test hash clone by adding a test to TESTS/mbed-crypto/sanity/main.cpp
@itayzafrir itayzafrir force-pushed the crypto-integration-with-spm branch from 57bbeb7 to dc54be2 Compare January 28, 2019 11:33
@itayzafrir itayzafrir changed the title WIP Crypto integration with SPM Crypto integration with SPM Jan 28, 2019
alekshex
alekshex reviewed Jan 28, 2019
View reviewed changes
components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c
sizeof(*lifetime))
}
psa_outvec_t out_vec[1] = {
{ key_handle, sizeof(*key_handle) }
Copy link
Contributor

Choose a reason for hiding this comment

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

can it return null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the allocate fails then key_handle remains untouched

alekshex
alekshex reviewed Jan 28, 2019
View reviewed changes
components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c
psa_handle_t handle = PSA_NULL_HANDLE;
psa_invec_t in_vec[1] = {
{ &psa_key_mng_ipc, sizeof(psa_key_mng_ipc_t) }
{ &psa_key_mng_ipc, sizeof(psa_key_mng_ipc) }
Copy link
Contributor

Choose a reason for hiding this comment

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

sizeof(psa_key_mng_ipc)
or
sizeof(psa_key_mng_ipc_t)? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same thing...

alekshex
alekshex reviewed Jan 28, 2019
View reviewed changes
components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c
psa_key_mng_ipc.type = 0;
psa_key_mng_ipc.func = PSA_GET_KEY_LIFETIME;
psa_key_mng_ipc_t psa_key_mng_ipc = { 0, 0, 0, 0 };
psa_key_mng_ipc.handle = *key_handle;
Copy link
Contributor

Choose a reason for hiding this comment

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

so handle expects a content of ptr, not ptr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, this is going IPC from here so it's different memory anyways.

alekshex
alekshex reviewed Jan 28, 2019
View reviewed changes
components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c
@@ -689,22 +696,18 @@ psa_status_t psa_asymmetric_decrypt(psa_key_slot_t key,
/* PSA_KEY_MANAGMENT */
/****************************************************************/

psa_status_t psa_get_key_lifetime(psa_key_slot_t key,
psa_key_lifetime_t *lifetime)
psa_status_t psa_allocate_key(psa_key_handle_t *key_handle)
Copy link
Contributor

@alekshex alekshex Jan 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

psa_get_key_lifetime is no more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gone, removed from the API

alekshex
alekshex reviewed Jan 28, 2019
View reviewed changes
components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c
{ &psa_key_mng_ipc, sizeof(psa_key_mng_ipc) },
{ &id, sizeof(id) }
};
psa_outvec_t out_vec[1] = { {
Copy link
Contributor

@alekshex alekshex Jan 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

style:

psa_outvec_t out_vec[1] = {
{ key_handle, sizeof(*key_handle) }
};

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 style is inconsistent throughout the whole file, see:

mbed-os/components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c

Lines 868 to 872 in dc54be2

psa_outvec_t out_vec[1] = { {
lifetime, (lifetime == NULL ? 0 :
sizeof(*lifetime))
}
};

mbed-os/components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c

Lines 946 to 950 in dc54be2

psa_outvec_t out_vec[1] = { {
policy, (policy == NULL ? 0 :
sizeof(*policy))
}
};

mbed-os/components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c

Lines 1438 to 1440 in dc54be2

out_vec[1] = (psa_outvec_t) {
iv_length, sizeof(*iv_length)
};

alekshex
alekshex reviewed Jan 28, 2019
View reviewed changes
components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c
};

handle = psa_connect(PSA_KEY_MNG_ID, MINOR_VER);
if (handle <= 0) {
Copy link
Contributor

@alekshex alekshex Jan 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

including 0?

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, same as the rest of the file

alekshex
alekshex reviewed Jan 28, 2019
View reviewed changes
components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c
err_call = psa_call(handle, in_vec, 2, out_vec, 1);
psa_close(handle);

if (err_call < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 == PSA_SUCCESS
> 0 == PSA_ERROR_XXX
< 0 == PSA_ERROR_COMMUNICATION_FAILURE

All IPC function calls work this way.

@orenc17 orenc17 mentioned this pull request Jan 29, 2019
Merged
@itayzafrir itayzafrir mentioned this pull request Jan 29, 2019
Closed
@NirSonnenschein
Copy link
Contributor

Hi @mbed-os-crypto ,
could someone please take a look at this PR, it needs to be approved for #9529.

alekshex
alekshex reviewed Jan 30, 2019
View reviewed changes
components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c
static int psa_spm_init_refence_counter = 0;

#ifndef MAX_CONCURRENT_HASH_CLONES
#define MAX_CONCURRENT_HASH_CLONES 2
Copy link
Contributor

Choose a reason for hiding this comment

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

4? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would take a very rare and unlikely scenario, I think it would be a waste of space...

alekshex
alekshex reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@alekshex alekshex left a comment

Choose a reason for hiding this comment

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

please see comments

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c
#ifndef MAX_CONCURRENT_HASH_CLONES
#define MAX_CONCURRENT_HASH_CLONES 2
#endif
static psa_spm_hash_clone_t psa_spm_hash_clones[MAX_CONCURRENT_HASH_CLONES];
Copy link
Contributor

Choose a reason for hiding this comment

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

so there is no multithreading, we don't need to sync over this data structure, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, assuming single threaded

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c
for (*index = 0; *index < MAX_CONCURRENT_HASH_CLONES; (*index)++) {
if (psa_spm_hash_clones[*index].partition_id == partition_id &&
psa_spm_hash_clones[*index].source_operation == source_operation) {
psa_spm_hash_clones[*index].ref_count++;
Copy link
Contributor

Choose a reason for hiding this comment

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

alignment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's ok, will run through astyle to double check

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c
for (*index = 0; *index < MAX_CONCURRENT_HASH_CLONES; (*index)++) {
if (psa_spm_hash_clones[*index].partition_id == 0 &&
psa_spm_hash_clones[*index].source_operation == NULL) {
psa_spm_hash_clones[*index].partition_id = partition_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

alignment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will check with astyle

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c
// ------------------------- Internal Helper Functions -------------------------
static inline psa_status_t reserve_hash_clone(int32_t partition_id, void *source_operation, size_t *index)
{
for (*index = 0; *index < MAX_CONCURRENT_HASH_CLONES; (*index)++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why index is a ptr, why not by value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it's an output parameter

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

for (*index = 0; *index < MAX_CONCURRENT_HASH_CLONES; (*index)++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please do this in single uper for with if, elseif, etc. why scan twice performance wise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scanning for different predicates in these 2 loops, it could be done in one loop but would also add a couple of if so not sure if it would be more efficient, it would make the code harder to understand though.

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c
size_t index = 0;

status = reserve_hash_clone(psa_identity(msg.handle), msg.rhandle, &index);
if (status == PSA_SUCCESS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if not what then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then return PSA_ERROR_BAD_STATE

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

status = get_hash_clone(index, psa_identity(msg.handle), &hash_clone);
if (status == PSA_SUCCESS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if not?
release_hash_clone anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then there will be nothing to release and returns PSA_ERROR_BAD_STATE

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c
psa_key_mng.type,
bits,
parameter, parameter_size);
mbedtls_free(parameter);
break;
}

case PSA_ALLOCATE_KEY: {
status = psa_allocate_key(&psa_key_mng.handle);
if (status == PSA_SUCCESS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if not what then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then whatever the crypto library returns, it's their error code which is returned.

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

status = psa_create_key(psa_key_mng.lifetime, id, &psa_key_mng.handle);
if (status == PSA_SUCCESS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if not ok what then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then whatever the crypto library returns, it's their error code which is returned.

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

status = psa_open_key(psa_key_mng.lifetime, id, &psa_key_mng.handle);
if (status == PSA_SUCCESS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if not ok what then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then whatever the crypto library returns, it's their error code which is returned.

NirSonnenschein
NirSonnenschein approved these changes Jan 31, 2019
View reviewed changes
Copy link
Contributor

@NirSonnenschein NirSonnenschein left a comment

Choose a reason for hiding this comment

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

looks fine except for some style issues which are fixed in the rollup.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 4, 2019

Merged via #9529

@0xc0170 0xc0170 closed this Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alekshex alekshex alekshex approved these changes

@NirSonnenschein NirSonnenschein NirSonnenschein approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@itayzafrir @ciarmcom @NirSonnenschein @0xc0170 @alekshex @cmonr