Refactor Crypto Service Proxy #10036

itayzafrir · 2019-03-11T14:39:18Z

Description

Refactor common functionality in crypto service proxy (client side) to prevent code duplication and save some flash space.

Tested locally:
Cypress - crypto sanity + crypto access control + crypto compliance - PASSED
NXP - crypto sanity + crypto access control + crypto compliance - PASSED

Pull request type

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

Reviewers

@avolinski @NirSonnenschein

Release Notes

ciarmcom · 2019-03-11T16:00:49Z

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

mikisch81 · 2019-03-11T16:13:21Z

How much space did it save?
Can you post the result of the secure build before and after?

itayzafrir · 2019-03-11T16:58:55Z

I'll post the size diff tomorrow, but this PR affects the non-secure side only. I'm planning on refactoring the secure side as well when I get a chance...

orenc17

Looks great, main focus is consistency
probably needed to be documented that the ipc_call returns BAD_STATE on a bad handle
But again looks great

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

itayzafrir · 2019-03-12T11:35:48Z

@orenc17 thanks for the review. I've added some more commits, please take a look.

Currently running tests on NXP & Cypress: crypto-sanity, crypto-access-control, crypto-compliance
All tests passed on NXP, Cypress is still running, I'll attach a log once tests are done.

@0xc0170 can you please remove the needs work label and add the needs review?

itayzafrir · 2019-03-12T12:12:26Z

All tests passed locally on both Cypress & NXP:

crypto sanity
crypto access control
crypto compliance

test_results.txt

- Use designated initializers for IPC structs - Unify variables declaration and initialization

orenc17

LGTM

0xc0170 · 2019-03-13T10:52:51Z

5.12.1 release target?

itayzafrir · 2019-03-13T11:13:58Z

5.12.1 release target?

This isn't anything urgent, it can wait.

0xc0170

One small change. Otherwise looks fine

0xc0170 · 2019-03-18T08:13:12Z

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

 {
-    psa_status_t status;
-    psa_crypto_ipc_t psa_crypto_ipc = { 0, 0, 0 };
+    //TODO: add retry mechanism to make sure resources were deallocated.


We should not leave TODO in the code, create a tracking issue rather

I removed the comment as it's not relevant anymore.

itayzafrir · 2019-03-18T08:31:54Z

@avolinski please review

0xc0170 · 2019-03-18T10:41:32Z

While waiting for final approval, CI started

alekshex · 2019-03-18T11:38:44Z

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


-    status = psa_call(handle, NULL, 0, NULL, 0);
-    psa_close(handle);
+    psa_status_t status = psa_call(*handle, in_vec, in_vec_size, out_vec, out_vec_size);


if call failed, do we close anyway?

yes, if close == true

mbed-ci · 2019-03-18T12:54:30Z

Test run: SUCCESS

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

itayzafrir · 2019-03-18T19:48:24Z

Please hold the merge until @avolinski completes his review.

itayzafrir · 2019-03-21T08:28:25Z

All reviews are done, can we get this merged please

Refactor crypto service proxy

7a15865

ciarmcom requested review from alekshex, NirSonnenschein and a team March 11, 2019 16:00

ciarmcom added the needs: review label Mar 11, 2019

orenc17 suggested changes Mar 11, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Mar 12, 2019

itayzafrir added 4 commits March 12, 2019 10:51

Use psa prefix for crypto.h include

caafc34

Use PSA_NULL_HANDLE when testing for invalid handles

bf05154

Set handle to PSA_NULL_HANDLE when closing IPC connection

5c48d58

Unify sections titles/comments & fix typo in comment

9bc8bcf

itayzafrir force-pushed the crypto-ipc-refactor branch 2 times, most recently from 2c87258 to 5bd8591 Compare March 12, 2019 12:29

Unify code style

c97b90a

- Use designated initializers for IPC structs - Unify variables declaration and initialization

itayzafrir force-pushed the crypto-ipc-refactor branch from 5bd8591 to c97b90a Compare March 12, 2019 12:31

orenc17 approved these changes Mar 12, 2019

View reviewed changes

0xc0170 added needs: review and removed needs: work labels Mar 13, 2019

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

NirSonnenschein approved these changes Mar 17, 2019

View reviewed changes

0xc0170 requested changes Mar 18, 2019

View reviewed changes

Remove uneeded TODO comment from the code

95b0a18

0xc0170 approved these changes Mar 18, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Mar 18, 2019

alekshex reviewed Mar 18, 2019

View reviewed changes

cmonr added ready for merge and removed needs: CI labels Mar 18, 2019

cmonr added needs: review and removed ready for merge labels Mar 18, 2019

cmonr requested a review from alekshex March 18, 2019 19:49

alekshex approved these changes Mar 20, 2019

View reviewed changes

NirSonnenschein added ready for merge and removed needs: review labels Mar 24, 2019

NirSonnenschein merged commit ca62922 into ARMmbed:master Mar 26, 2019

NirSonnenschein removed the ready for merge label Mar 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Crypto Service Proxy #10036

Refactor Crypto Service Proxy #10036

itayzafrir commented Mar 11, 2019 •

edited

Loading

ciarmcom commented Mar 11, 2019

mikisch81 commented Mar 11, 2019

itayzafrir commented Mar 11, 2019

orenc17 left a comment

itayzafrir commented Mar 12, 2019 •

edited

Loading

itayzafrir commented Mar 12, 2019

orenc17 left a comment

0xc0170 commented Mar 13, 2019

itayzafrir commented Mar 13, 2019

0xc0170 left a comment

0xc0170 Mar 18, 2019

itayzafrir Mar 18, 2019

itayzafrir commented Mar 18, 2019

0xc0170 commented Mar 18, 2019

alekshex Mar 18, 2019

itayzafrir Mar 18, 2019

mbed-ci commented Mar 18, 2019

itayzafrir commented Mar 18, 2019

itayzafrir commented Mar 21, 2019

Refactor Crypto Service Proxy #10036

Refactor Crypto Service Proxy #10036

Conversation

itayzafrir commented Mar 11, 2019 • edited Loading

Description

Pull request type

Reviewers

Release Notes

ciarmcom commented Mar 11, 2019

mikisch81 commented Mar 11, 2019

itayzafrir commented Mar 11, 2019

orenc17 left a comment

Choose a reason for hiding this comment

itayzafrir commented Mar 12, 2019 • edited Loading

itayzafrir commented Mar 12, 2019

All tests passed locally on both Cypress & NXP:

orenc17 left a comment

Choose a reason for hiding this comment

0xc0170 commented Mar 13, 2019

itayzafrir commented Mar 13, 2019

0xc0170 left a comment

Choose a reason for hiding this comment

0xc0170 Mar 18, 2019

Choose a reason for hiding this comment

itayzafrir Mar 18, 2019

Choose a reason for hiding this comment

itayzafrir commented Mar 18, 2019

0xc0170 commented Mar 18, 2019

alekshex Mar 18, 2019

Choose a reason for hiding this comment

itayzafrir Mar 18, 2019

Choose a reason for hiding this comment

mbed-ci commented Mar 18, 2019

Test run: SUCCESS

itayzafrir commented Mar 18, 2019

itayzafrir commented Mar 21, 2019

itayzafrir commented Mar 11, 2019 •

edited

Loading

itayzafrir commented Mar 12, 2019 •

edited

Loading