Skip to content

Conversation

gilles-peskine-arm
Copy link
Collaborator

I went through FIXME/TODO comments in the code and added the corresponding issue numbers:

I also modified a comment relating to https://github.com/ARMmbed/mbed-crypto/issues/86 (not related to secure elements) but did not add an issue number because this is not exactly a bug and may even be resolved differently.

While doing this, I found two bugs in psa_destroy_key which I fixed and added a test for (in one case, I actually fixed a test bug that was hidden by the implementation bug).

I'm making this PR to psa-api-1.0-beta to be on top of the latest SE work. Once this PR is merged, we should do another merge from psa-api-1.0-beta following #212.

When a key slot is wiped, a copy of the key material may remain in
operations. This is undesirable, but does not violate the safety of
the code. Tracked in https://github.com/ARMmbed/mbed-crypto/issues/86
Adopt a simple method for tracking whether there was a failure: each
fallible operation sets overall_status, unless overall_status is
already non-successful. Thus in case of multiple failures, the
function always reports whatever failed first. This may not always be
the right thing, but it's simple.

This revealed a bug whereby if the only failure was the call to
psa_destroy_se_key(), i.e. if the driver reported a failure or if the
driver lacked support for destroying keys, psa_destroy_key() would
ignore that failure.

For a key in a secure element, if creating a transaction file fails,
don't touch storage, but close the key in memory. This may not be
right, but it's no wronger than it was before. Tracked in
ARMmbed#215
Drivers that allow destroying a key must have a destroy method. This
test bug was previously not caught because of an implementation bug
that lost the error triggered by the missing destroy method.
@gilles-peskine-arm gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. api-spec Issue or PR about the PSA specifications labels Aug 13, 2019
Copy link
Collaborator

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

I have a couple of questions, but no change requests. Looks good to me.

@k-stachowiak k-stachowiak self-requested a review August 14, 2019 10:52
Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm added bug Something isn't working enhancement New feature or request and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Aug 14, 2019
@gilles-peskine-arm gilles-peskine-arm merged commit bbdf310 into ARMmbed:psa-api-1.0-beta Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-spec Issue or PR about the PSA specifications bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants