PSA Crypto Service - multipart operation memory fixes #10232

itayzafrir · 2019-03-26T13:50:13Z

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:

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

Also refactor cipher setup function to one common function.

ciarmcom · 2019-03-26T14:00:36Z

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

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

alekshex · 2019-03-27T16:58:11Z

components/TARGET_PSA/services/crypto/COMPONENT_PSA_SRV_IPC/psa_crypto_spm.c

@@ -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) {


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

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"

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.

alekshex · 2019-03-27T17:27:27Z

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

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


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

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

alekshex · 2019-03-27T17:39:51Z

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

@@ -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);


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

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

alekshex · 2019-03-27T17:43:40Z

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

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


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

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 · 2019-03-28T11:32:57Z

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

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

NirSonnenschein

changes look good to me

cmonr · 2019-04-09T18:54:12Z

@avolinski Thoughts?

0xc0170 · 2019-04-10T11:44:43Z

CI started

mbed-ci · 2019-04-10T12:29:17Z

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 · 2019-04-10T12:32:40Z

Restarted jenkins-ci

mbed-ci · 2019-04-10T21:22:02Z

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 · 2019-04-11T08:22:33Z

License server error, restarting exporters

itayzafrir added 4 commits March 26, 2019 15:32

Fix crypto service abort functionality - cipher

1b26e0d

Also refactor cipher setup function to one common function.

Fix crypto service abort functionality - hash

da999c6

Fix crypto service abort functionality - mac

545e669

Fix crypto service abort functionality - generators

8044f6d

ciarmcom requested review from alekshex, NirSonnenschein and a team March 26, 2019 14:00

ciarmcom added the needs: review label Mar 26, 2019

cmonr added the release-version: 5.12.1 label Mar 26, 2019

NirSonnenschein approved these changes Mar 27, 2019

View reviewed changes

NirSonnenschein added needs: work and removed needs: review labels Mar 27, 2019

itayzafrir added 10 commits March 27, 2019 14:29

Do not allocate zero sized buffers - mac

7b35e76

Do not allocate zero sized buffers - hash

979ca1e

Do not allocate zero sized buffers - asymmetric

18feb26

Do not allocate zero sized buffers - aead

19952d9

Do not allocate zero sized buffers - cipher

f3294ef

Do not allocate zero sized buffers - keys

19b8381

Do not allocate zero sized buffers - entropy

abd9c66

Do not allocate zero sized buffers - rng

749b1e3

Do not allocate zero sized buffers - generators

b26890e

Rename internal function destroy_hash_clone to clear_hash_clone

a446ee6

alekshex suggested changes Mar 27, 2019

View reviewed changes

Add message context cleanup helper function

2b81588

NirSonnenschein added needs: review and removed needs: work labels Mar 28, 2019

NirSonnenschein reviewed Mar 31, 2019

View reviewed changes

components/TARGET_PSA/services/crypto/COMPONENT_SPE/psa_crypto_partition.c Show resolved Hide resolved

NirSonnenschein approved these changes Mar 31, 2019

View reviewed changes

cmonr removed the release-version: 5.12.1 label Apr 9, 2019

alekshex approved these changes Apr 10, 2019

View reviewed changes

0xc0170 added needs: CI release-version: 5.12.2 and removed needs: review labels Apr 10, 2019

0xc0170 approved these changes Apr 10, 2019

View reviewed changes

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

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

0xc0170 removed the ready for merge label Apr 11, 2019

PSA Crypto Service - multipart operation memory fixes #10232

PSA Crypto Service - multipart operation memory fixes #10232

Uh oh!

Conversation

itayzafrir commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Mar 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alekshex Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itayzafrir commented Mar 28, 2019

Uh oh!

Uh oh!

NirSonnenschein left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Apr 9, 2019

Uh oh!

0xc0170 commented Apr 10, 2019

Uh oh!

mbed-ci commented Apr 10, 2019

Uh oh!

alekla01 commented Apr 10, 2019

Uh oh!

mbed-ci commented Apr 10, 2019

Uh oh!

0xc0170 commented Apr 11, 2019

Uh oh!

Uh oh!

itayzafrir commented Mar 26, 2019 •

edited

Loading

alekshex Apr 1, 2019 •

edited

Loading