Skip to content

PSA: Fix error codes masking in psa_attestation_inject_key() #9996

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
Merged
merged 2 commits into from
Mar 10, 2019
Merged

PSA: Fix error codes masking in psa_attestation_inject_key() #9996

merged 2 commits into from
Mar 10, 2019

Conversation

orenc17
Copy link
Contributor

@orenc17 orenc17 commented Mar 7, 2019

Description

psa_attestation_inject_key() IPC implementation was masking negative error codes as PSA_ERROR_COMMUNICATION_FAILURE instead of returning the actual error code.

Clear the ITS storage in test case setup handler to avoid a situation where a call for psa_attestation_inject_key() would fail for key already exists error.

Did not check regression yet.

Pull request type

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

Reviewers

@ARMmbed/mbed-os-psa @moranpeker @netanelgonen @itayzafrir @avolinski

Release Notes

netanelgonen
netanelgonen reviewed Mar 7, 2019
View reviewed changes
TESTS/psa/attestation/main.cpp Outdated
@@ -128,11 +129,13 @@ utest::v1::status_t case_teardown_handler(const Case *const source, const size_t

utest::v1::status_t case_setup_handler(const Case *const source, const size_t index_of_case)
{
psa_status_t status = mbed_psa_reboot_and_request_new_security_state(PSA_LIFECYCLE_ASSEMBLY_AND_TEST);
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to choose the correct way to clean this test,
in tear down the test uses 'psa_destroy_key(handle);' and in init the test uses mbed_psa_reboot_and_request_new_security_state.

it seems the correct way is or both use 'mbed_psa_reboot_and_request_new_security_state' or both uses 'psa_destroy_key'

in addition, does the 'mbed_psa_reboot_and_request_new_security_state' will work if using protected storage and not secure internal storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@netanelgonen
what does PS has to do with attestation? please elaborate

Copy link
Contributor

Choose a reason for hiding this comment

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

@netanelgonen, despite its name, the mbed_psa_reboot_and_request_new_security_state()API will not reboot the system when moving from PSA_LIFECYCLE_ASSEMBLY_AND_TEST to PSA_LIFECYCLE_ASSEMBLY_AND_TEST. The only effect it will have is to clean PSA RoT state. (currently just wiping out ITS).

Copy link
Contributor

Choose a reason for hiding this comment

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

@orenc17 @alzix, please choose 'mbed_psa_reboot_and_request_new_security_state' or both uses 'psa_destroy_key'

@ciarmcom
Copy link
Member

ciarmcom commented Mar 7, 2019

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

@ghost ghost added the PM_ACCEPTED label Mar 7, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 8, 2019

Set to 5.12.0-rc2

@orenc17 orenc17 changed the title PSA: Fix error codes making in psa_attestation_inject_key() PSA: Fix error codes masking in psa_attestation_inject_key() Mar 8, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 8, 2019

This needs reviews ! I'll run CI meanwhile

@mbed-ci
Copy link

mbed-ci commented Mar 8, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

alzix
alzix approved these changes Mar 9, 2019
View reviewed changes
Copy link
Contributor

@alzix alzix left a comment

Choose a reason for hiding this comment

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

LGTM

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 9, 2019

Did not check regression yet.

Any findings?

@orenc17
Copy link
Contributor Author

orenc17 commented Mar 10, 2019

Tested on K64F, CY8CKIT_062_WIFI_BT_PSA, FUTURE_SEQUANA_PSA

@mbed-ci
Copy link

mbed-ci commented Mar 10, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mbed-ci
Copy link

mbed-ci commented Mar 10, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 3
Build artifacts

@NirSonnenschein NirSonnenschein merged commit de1f086 into ARMmbed:master Mar 10, 2019
@alzix alzix mentioned this pull request Mar 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alzix alzix alzix approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@mikisch81 mikisch81 mikisch81 approved these changes

@NirSonnenschein NirSonnenschein NirSonnenschein approved these changes

@moranpeker moranpeker moranpeker approved these changes

@alekshex alekshex Awaiting requested review from alekshex

@itayzafrir itayzafrir Awaiting requested review from itayzafrir

@netanelgonen netanelgonen Awaiting requested review from netanelgonen

Assignees
No one assigned
Labels
release-version: 5.12.0-rc2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@orenc17 @ciarmcom @0xc0170 @mbed-ci @alzix @mikisch81 @NirSonnenschein @netanelgonen @moranpeker