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.

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

PSA Crypto Service - multipart operation memory fixes #10232

Merged

Conversation

Projects
None yet
8 participants
@itayzafrir
Copy link
Contributor

commented Mar 26, 2019

Description

Prevent potential memory leaks on the SPM side and also free dynamically allocated memory as soon as possible.

Summary of changes in this PR:

  1. Make the service behavior consistent with the library - e.g. in the library if psa_hash_update fails then the operation is aborted and the caller doesn't have to call psa_hash_abort (as abort is called by the library). The service behavior was not consistent with the library and could lead to memory leaks on the server side.
  2. Free dynamically allocated memory on the SPM side as soon as possible - whenever possible, don't rely on a call to psa_close to free dynamically allocated contexts but free them ASAP.
  3. Don't allow various (client) setup operations (such as psa_hash_setup) if the context is active (part of an active session), this could lead to memory leaks on the SPM side.
  4. Don't allocate zero sized buffers, pass the input/output buffers to the library as is and let the library decide how to handle them.
  5. Rename internal function destroy_hash_clone to clear_hash_clone.

Pull request type

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

Reviewers

@avolinski @NirSonnenschein

Release Notes

@ciarmcom ciarmcom requested review from avolinski, NirSonnenschein and ARMmbed/mbed-os-maintainers Mar 26, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

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

@@ -133,6 +137,9 @@ static psa_status_t psa_mac_setup(psa_mac_operation_t *operation,
return (status);
}
status = ipc_call(&operation->handle, &in_vec, 1, NULL, 0, false);
if (status != PSA_SUCCESS) {

This comment has been minimized.

Copy link
@avolinski

avolinski Mar 27, 2019

Contributor

ipc_call is a wrapper by itself.
i suggest for the same idea of code being clear and refactoring.
to put the
if (status != PSA_SUCCESS) {
ipc_close(&operation->handle);

inside ipc_call in case of failure.
with only one important note, if and only if we always want to close connections on call failure

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Mar 28, 2019

Author Contributor

This is not always true, e.g. generators functions like psa_get_generator_capacity etc. don't close in case of error, so leaving it as is.

This comment has been minimized.

Copy link
@avolinski

avolinski Apr 1, 2019

Contributor

why they don't close?
maybe we want to consider having a flag passed whether to close or not.
and still hide the close inside? there are too many of the same "close"

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Apr 1, 2019

Author Contributor

I don't think having another flag makes much of a difference regarding code size or readability, if you think it's important I'll make another change.

psa_crypto.handle,
psa_crypto.alg);
if (status != PSA_SUCCESS) {
mbedtls_free(msg.rhandle);

This comment has been minimized.

Copy link
@avolinski

avolinski Mar 27, 2019

Contributor

is it ok to free handle (mbedtls_free) for both scenarios:
sign setup fails
handle is not permitted?

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Mar 28, 2019

Author Contributor

yes it is, the operation failed for whatever reason and the context is discarded.

@@ -328,22 +330,27 @@ static void psa_mac_operation(void)

uint8_t *mac = mbedtls_calloc(1, mac_length);
if (mac == NULL) {
psa_mac_abort(msg.rhandle);

This comment has been minimized.

Copy link
@avolinski

avolinski Mar 27, 2019

Contributor

won't it cause recursion?
calling psa_mac_abort will call psa_mac_abort which will call this function with case of abort, which calls psa_mac_abort in case of abort

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Mar 28, 2019

Author Contributor

No, this is service code calling library code, the library never calls the service.

mbedtls_free(salt);
mbedtls_free(label);
if (status != PSA_SUCCESS) {

This comment has been minimized.

Copy link
@avolinski

avolinski Mar 27, 2019

Contributor

this block returns dozens of times.
please wrap in in static cleanup named function.
i would also suggest considering instead of repeating it multiple times through various cases. considering some bailout flag and logic and doing cleanup related to this flag once before return or so.
code becomes now a bit spaghetti like and is being a bit blown in size

This comment has been minimized.

Copy link
@itayzafrir

itayzafrir Mar 28, 2019

Author Contributor

I'll provide a static cleanup function. I don't like the error flag approach in this case because it will add even more complexity to these nested switch cases. We'll refactor the partition also once we get a chance.

@itayzafrir

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@NirSonnenschein @avolinski I've added some more commits addressing the issues you raised, please re-review.

@NirSonnenschein
Copy link
Contributor

left a comment

changes look good to me

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@avolinski Thoughts?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 10, 2019

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
@alekla01

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Restarted jenkins-ci

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 10, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

License server error, restarting exporters

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Apr 11, 2019

@0xc0170 0xc0170 merged commit c6b9173 into ARMmbed:master Apr 11, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-36 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10280 cycles (+1108 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.