Skip to content

Multithreaded access to PSA slots - stateful approach #5673

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

Conversation

AndrzejKurek
Copy link
Contributor

@AndrzejKurek AndrzejKurek commented Mar 30, 2022

Protect access to key slot data & metadata by a global mutex.
Introduce a state machine to handle psa key slot states in a more robust way.
Split key destruction into two parts to be able to delay the destruction once no readers are active.

State graph:

                │
           ┌────▼─────┐  ┌──────────┐
      ┌────┤  EMPTY   ◄──┤DESTROYING│
      │    └─┬────┬──▲┘  └──▲──▲────┘
┌─────▼────┐ │ ┌──▼──┴────┐ │  │
│ CREATING │ │ │  WIPING  ├─┘  │
└──┬─┬─────┘ │ └──────▲───┘    │
   │ │  ┌────▼─────┐  ├────────┘
   │ └──►  UNUSED  ├──┤
   │    └──▲────┬──┘  │
   │    ┌──┴────▼──┐  │
   │  ┌─┤ READING  ├──┤
   │  │ └─▲────────┘  │
   │  └───┘           │
   └──────────────────┘ 

State transitions from:
Empty state:
-> Wiping: cleaning slots in psa_wipe_all_key_slots: psa_crypto_init.
-> Unused: importing an existing key that does not require creation: psa_load_persistent_key_into_slot, psa_load_builtin_key_into_slot.
-> Creating: psa_import_key, psa_copy_key, psa_key_derivation_output_key, psa_generate_key.

Creating state:
-> Wiping: psa_fail_key_creation.
-> Destroying: UNUSED; TODO - could/should it happen?
-> Unused: psa_finish_key_creation from psa_import_key, psa_copy_key, psa_key_derivation_output_key, psa_generate_key.

Unused state:
-> Reading: Any operation that needs to read the key material. Copying, exporting, signing, verifying, enc/dec... via psa_get_and_lock_key_slot_with_policy.
-> Wiping: psa_purge_key, psa_close_key, but also psa_get_empty_key_slot if there's an unused persistent key.
-> Destroying: psa_destroy_key.

Reading state:
-> Reading: another reader added via psa_get_and_lock_key_slot_with_policy.
-> Unused: psa_unlock_key_slot by the last reader.
-> Wiping: psa_purge_key, psa_close_key;
-> Destroying: psa_destroy_key.

Wiping state:
-> Destroying: psa_destroy_key.
-> Empty: psa_wipe_key from psa_close_key, psa_purge_key, but also delayed wiping when the last reader is unlocked in psa_unlock_key_slot. Failures in psa_get_and_lock_key_slot, psa_get_empty_key_slot.

Destroying state:
-> Empty: psa_finish_key_destruction (also calls psa_wipe_key), psa_destroy_key if there are no readers, and if there was - unlocking the last one via psa_unlock_key_slot.

Open questions:

  • If a destruction is scheduled (but there are some active readers), should a second destruction call return a new error, for example PSA_ERROR_PENDING?
  • Should calling psa_open_key on a key slot that has pending destruction/wiping fail?
  • Is it possible to go from PSA_STATE_CREATING to PSA_STATE_DESTROYING?
  • Should psa_wipe_key_slot not work if the key slots are in use? Currently, calling psa_wipe_all_key_slots will wipe all key slots once it can acquire a mutex, regardless of slot states. psa_wipe_key_slot is surrounded by protective logic everywhere apart from psa_wipe_all_key_slots.
  • Should the caller be notified, that, when the key slot was unlocked, an additional delayed wiping/destruction was performed? PSA_SUCCESS_KEY_DESTROYED? PSA_SUCCESS_KEY_WIPED?
  • Should psa_get_and_lock_key_slot be renamed? In fact this function gets the key slot and tries to transition it to a desired state.

Double, simultaneous key creation test cannot be added without multithreading in tests.

This PR supercedes #5084 and #5226.
Partially fixes #3263.

Status update 2023-03-29

  • informally, broadly agreed on design, but have not done formal design review (significant task)
  • needs significant work to add tests (probably follow-up PR to add thorough multi-threaded tests)
  • interest from multiple partners and internal team

@AndrzejKurek
Copy link
Contributor Author

AndrzejKurek commented Mar 31, 2022

Force pushed to trigger the CI.

@AndrzejKurek AndrzejKurek force-pushed the multithreading-2022-global branch from 492ba03 to f1e5159 Compare April 5, 2022 12:46
@AndrzejKurek AndrzejKurek added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed DO-NOT-MERGE labels Apr 5, 2022
@AndrzejKurek AndrzejKurek force-pushed the multithreading-2022-global branch from f1e5159 to e2ca5b1 Compare April 5, 2022 18:01
@AndrzejKurek AndrzejKurek removed the needs-ci Needs to pass CI tests label Apr 8, 2022
@daverodgman daverodgman added component-psa PSA keystore/dispatch layer (storage, drivers, …) priority-medium Medium priority - this can be reviewed as time permits and removed mbed TLS team labels May 13, 2022
Andrzej Kurek added 11 commits August 17, 2022 07:57
Create a state machine as a first step of improving the code state towards
concurrency. This way multiple threads will be able to read the key slot
at once as long as the critical data is protected by mutexes (to be done).

Split key destruction code into half, first one handling state changes,
and the second one doing all the work. This way the destruction itself
can be delayed until there are no readers of the slot.

Majority of the state changes are handled by psa_get_and_lock_key_slot,
which now receives an intent from callers, to perform key locking and
a state transition as one, atomic operation.

Introduce a new error indicating that a destruction or a purge operation
is postponed until the slot is no longer used.
Signed-off-by: Andrzej Kurek <[email protected]>
Signed-off-by: Andrzej Kurek <[email protected]>
Any modification of key slot data, or state has to be done under
a lock. Reading however can be performed by multiple callers,
as long as the slot remains in one of the valid states.
Signed-off-by: Andrzej Kurek <[email protected]>
Use test hooks to validate states before locking the key slot.
Signed-off-by: Andrzej Kurek <[email protected]>
Add a missing include in psa_crypto.c
Signed-off-by: Andrzej Kurek <[email protected]>
Andrzej Kurek and others added 14 commits August 17, 2022 07:57
Signed-off-by: Andrzej Kurek <[email protected]>
Destroying is considered a more destructive operation
and internally wipes the slot too. Reducing it to WIPING
should not be possible.
Signed-off-by: Andrzej Kurek <[email protected]>
Add tests for double closing/purging/destroying a key plus
permutations of these operations.
Signed-off-by: Andrzej Kurek <[email protected]>
This simplifies code a little, and also lets psa_get_and_lock_key_slot
identify calls from psa_open_key that do not intend to change
the key slot state.
Signed-off-by: Andrzej Kurek <[email protected]>
Looks like a mutex isn't enough?

Signed-off-by: Andrzej Kurek <[email protected]>
Signed-off-by: Andrzej Kurek <[email protected]>
There are two somewhat distinct aspects here: if it compiled, it still
compiles; and if it worked functionally, it still works. They're related in
that if application code currently compiles but cannot possibly work, we
could reasonably make it not compile anymore.

Signed-off-by: Andrzej Kurek <[email protected]>
@AndrzejKurek AndrzejKurek force-pushed the multithreading-2022-global branch from e2ca5b1 to b00f2b4 Compare August 17, 2022 12:05
@AndrzejKurek
Copy link
Contributor Author

Rebased on top of current development without any conflicts.

@carlaageamundsen
Copy link

Hi
What is the plan for this PR?

@AndrzejKurek
Copy link
Contributor Author

Hi What is the plan for this PR?

A design review was agreed upon in the nearest future, followed up by an assesment of the volume of work needed to finish this task. Any relevant information will be posted in this PR in the process to keep you updated.

@tom-daubney-arm tom-daubney-arm added needs-work historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jul 14, 2023
@tom-daubney-arm
Copy link
Contributor

We are now converting older PRs to draft PRs where the following conditions are met: They have not been updated in the last 3 months, and they need more than non-trivial work to complete.

@tom-daubney-arm tom-daubney-arm marked this pull request as draft July 14, 2023 10:33
@paul-elliott-arm paul-elliott-arm self-requested a review July 21, 2023 16:07
@ronald-cron-arm ronald-cron-arm removed their request for review August 28, 2023 09:30
@yanesca
Copy link
Contributor

yanesca commented Nov 28, 2023

@yanesca yanesca closed this Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-psa PSA keystore/dispatch layer (storage, drivers, …) historical-reviewed Reviewed & agreed to keep legacy PR/issue needs-work priority-medium Medium priority - this can be reviewed as time permits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The PSA code is not thread-safe
6 participants