From 014bd8a8af63390c8b345c320c4048055594a13d Mon Sep 17 00:00:00 2001 From: Samuel Bailey Date: Thu, 25 Jun 2020 09:02:21 +0100 Subject: [PATCH] Changed local_ids for Atomic counter and removed key_slot_semaphore. Signed-off-by: Samuel Bailey --- src/providers/mbed_provider/asym_sign.rs | 2 - src/providers/mbed_provider/key_management.rs | 76 +++++++------------ src/providers/mbed_provider/mod.rs | 32 ++++---- 3 files changed, 40 insertions(+), 70 deletions(-) diff --git a/src/providers/mbed_provider/asym_sign.rs b/src/providers/mbed_provider/asym_sign.rs index b4a1e04a..1e7e373a 100644 --- a/src/providers/mbed_provider/asym_sign.rs +++ b/src/providers/mbed_provider/asym_sign.rs @@ -16,7 +16,6 @@ impl MbedProvider { op: psa_sign_hash::Operation, ) -> Result { info!("Mbed Provider - Asym Sign"); - let _semaphore_guard = self.key_slot_semaphore.access(); let key_name = op.key_name; let hash = op.hash; let alg = op.alg; @@ -53,7 +52,6 @@ impl MbedProvider { op: psa_verify_hash::Operation, ) -> Result { info!("Mbed Provider - Asym Verify"); - let _semaphore_guard = self.key_slot_semaphore.access(); let key_name = op.key_name; let hash = op.hash; let alg = op.alg; diff --git a/src/providers/mbed_provider/key_management.rs b/src/providers/mbed_provider/key_management.rs index 58be8bd3..828fe136 100644 --- a/src/providers/mbed_provider/key_management.rs +++ b/src/providers/mbed_provider/key_management.rs @@ -1,9 +1,10 @@ // Copyright 2020 Contributors to the Parsec project. // SPDX-License-Identifier: Apache-2.0 -use super::{LocalIdStore, MbedProvider}; +use super::MbedProvider; use crate::authenticators::ApplicationName; use crate::key_info_managers; use crate::key_info_managers::{KeyInfo, KeyTriple, ManageKeyInfo}; +use log::error; use log::{info, warn}; use parsec_interface::operations::psa_key_attributes::Attributes; use parsec_interface::operations::{ @@ -12,8 +13,7 @@ use parsec_interface::operations::{ use parsec_interface::requests::{ProviderID, ResponseStatus, Result}; use psa_crypto::operations::key_management as psa_crypto_key_management; use psa_crypto::types::key; -use rand::rngs::SmallRng; -use rand::{Rng, SeedableRng}; +use std::sync::atomic::{AtomicU32, Ordering::Relaxed}; /// Gets a PSA Key ID from the Key Info Manager. /// Wrapper around the get method of the Key Info Manager to convert the key ID to the psa_key_id_t @@ -46,16 +46,23 @@ fn create_key_id( key_triple: KeyTriple, key_attributes: Attributes, store_handle: &mut dyn ManageKeyInfo, - local_ids_handle: &mut LocalIdStore, + max_current_id: &AtomicU32, ) -> Result { - let mut rng = SmallRng::from_entropy(); - let mut key_id = rng.gen_range(key::PSA_KEY_ID_USER_MIN, key::PSA_KEY_ID_USER_MAX + 1); - - while local_ids_handle.contains(&key_id) { - key_id = rng.gen_range(key::PSA_KEY_ID_USER_MIN, key::PSA_KEY_ID_USER_MAX + 1); + // fetch_add adds 1 to the old value and returns the old value, so add 1 to local value for new ID + let new_key_id = max_current_id.fetch_add(1, Relaxed) + 1; + if new_key_id > key::PSA_KEY_ID_USER_MAX { + // If storing key failed and no other keys were created in the mean time, it is safe to + // decrement the key counter. + let _ = max_current_id.store(key::PSA_KEY_ID_USER_MAX, Relaxed); + error!( + "PSA max key ID limit of {} reached", + key::PSA_KEY_ID_USER_MAX + ); + return Err(ResponseStatus::PsaErrorInsufficientMemory); } + let key_info = KeyInfo { - id: key_id.to_ne_bytes().to_vec(), + id: new_key_id.to_ne_bytes().to_vec(), attributes: key_attributes, }; match store_handle.insert(key_triple.clone(), key_info) { @@ -63,25 +70,16 @@ fn create_key_id( if insert_option.is_some() { warn!("Overwriting Key triple mapping ({})", key_triple); } - let _ = local_ids_handle.insert(key_id); - - Ok(key_id) + Ok(new_key_id) } Err(string) => Err(key_info_managers::to_response_status(string)), } } -fn remove_key_id( - key_triple: &KeyTriple, - key_id: key::psa_key_id_t, - store_handle: &mut dyn ManageKeyInfo, - local_ids_handle: &mut LocalIdStore, -) -> Result<()> { +fn remove_key_id(key_triple: &KeyTriple, store_handle: &mut dyn ManageKeyInfo) -> Result<()> { + // ID Counter not affected as overhead and extra complication deemed unnecessary match store_handle.remove(key_triple) { - Ok(_) => { - let _ = local_ids_handle.remove(&key_id); - Ok(()) - } + Ok(_) => Ok(()), Err(string) => Err(key_info_managers::to_response_status(string)), } } @@ -99,7 +97,6 @@ impl MbedProvider { op: psa_generate_key::Operation, ) -> Result { info!("Mbed Provider - Create Key"); - let _semaphore_guard = self.key_slot_semaphore.access(); let key_name = op.key_name; let key_attributes = op.attributes; let key_triple = KeyTriple::new(app_name, ProviderID::MbedCrypto, key_name); @@ -107,7 +104,6 @@ impl MbedProvider { .key_info_store .write() .expect("Key store lock poisoned"); - let mut local_ids_handle = self.local_ids.write().expect("Local ID lock poisoned"); if key_info_exists(&key_triple, &*store_handle)? { return Err(ResponseStatus::PsaErrorAlreadyExists); } @@ -115,7 +111,7 @@ impl MbedProvider { key_triple.clone(), key_attributes, &mut *store_handle, - &mut local_ids_handle, + &self.id_counter, )?; let _guard = self @@ -126,12 +122,7 @@ impl MbedProvider { match psa_crypto_key_management::generate(key_attributes, Some(key_id)) { Ok(_) => Ok(psa_generate_key::Result {}), Err(error) => { - remove_key_id( - &key_triple, - key_id, - &mut *store_handle, - &mut local_ids_handle, - )?; + remove_key_id(&key_triple, &mut *store_handle)?; let error = ResponseStatus::from(error); format_error!("Generate key status: {}", error); Err(error) @@ -145,7 +136,6 @@ impl MbedProvider { op: psa_import_key::Operation, ) -> Result { info!("Mbed Provider - Import Key"); - let _semaphore_guard = self.key_slot_semaphore.access(); let key_name = op.key_name; let key_attributes = op.attributes; let key_data = op.data; @@ -154,7 +144,6 @@ impl MbedProvider { .key_info_store .write() .expect("Key store lock poisoned"); - let mut local_ids_handle = self.local_ids.write().expect("Local ID lock poisoned"); if key_info_exists(&key_triple, &*store_handle)? { return Err(ResponseStatus::PsaErrorAlreadyExists); } @@ -162,7 +151,7 @@ impl MbedProvider { key_triple.clone(), key_attributes, &mut *store_handle, - &mut local_ids_handle, + &self.id_counter, )?; let _guard = self @@ -173,12 +162,7 @@ impl MbedProvider { match psa_crypto_key_management::import(key_attributes, Some(key_id), &key_data[..]) { Ok(_) => Ok(psa_import_key::Result {}), Err(error) => { - remove_key_id( - &key_triple, - key_id, - &mut *store_handle, - &mut local_ids_handle, - )?; + remove_key_id(&key_triple, &mut *store_handle)?; let error = ResponseStatus::from(error); format_error!("Import key status: {}", error); Err(error) @@ -192,7 +176,6 @@ impl MbedProvider { op: psa_export_public_key::Operation, ) -> Result { info!("Mbed Provider - Export Public Key"); - let _semaphore_guard = self.key_slot_semaphore.access(); let key_name = op.key_name; let key_triple = KeyTriple::new(app_name, ProviderID::MbedCrypto, key_name); let store_handle = self.key_info_store.read().expect("Key store lock poisoned"); @@ -220,14 +203,12 @@ impl MbedProvider { op: psa_destroy_key::Operation, ) -> Result { info!("Mbed Provider - Destroy Key"); - let _semaphore_guard = self.key_slot_semaphore.access(); let key_name = op.key_name; let key_triple = KeyTriple::new(app_name, ProviderID::MbedCrypto, key_name); let mut store_handle = self .key_info_store .write() .expect("Key store lock poisoned"); - let mut local_ids_handle = self.local_ids.write().expect("Local ID lock poisoned"); let key_id = get_key_id(&key_triple, &*store_handle)?; let _guard = self @@ -247,12 +228,7 @@ impl MbedProvider { match destroy_key_status { Ok(()) => { - remove_key_id( - &key_triple, - key_id, - &mut *store_handle, - &mut local_ids_handle, - )?; + remove_key_id(&key_triple, &mut *store_handle)?; Ok(psa_destroy_key::Result {}) } Err(error) => { diff --git a/src/providers/mbed_provider/mod.rs b/src/providers/mbed_provider/mod.rs index a086d2f4..41a555d8 100644 --- a/src/providers/mbed_provider/mod.rs +++ b/src/providers/mbed_provider/mod.rs @@ -14,16 +14,16 @@ use parsec_interface::requests::{Opcode, ProviderID, ResponseStatus, Result}; use psa_crypto::types::{key, status}; use std::collections::HashSet; use std::io::{Error, ErrorKind}; -use std::sync::{Arc, Mutex, RwLock}; -use std_semaphore::Semaphore; +use std::sync::{ + atomic::{AtomicU32, Ordering::Relaxed}, + Arc, Mutex, RwLock, +}; use uuid::Uuid; mod asym_sign; #[allow(dead_code)] mod key_management; -type LocalIdStore = HashSet; -const PSA_KEY_SLOT_COUNT: isize = 32; const SUPPORTED_OPCODES: [Opcode; 6] = [ Opcode::PsaGenerateKey, Opcode::PsaDestroyKey, @@ -42,18 +42,16 @@ pub struct MbedProvider { // reference it to match with the method prototypes. #[derivative(Debug = "ignore")] key_info_store: Arc>, - local_ids: RwLock, // Calls to `psa_open_key`, `psa_generate_key` and `psa_destroy_key` are not thread safe - the slot // allocation mechanism in Mbed Crypto can return the same key slot for overlapping calls. // `key_handle_mutex` is use as a way of securing access to said operations among the threads. // This issue tracks progress on fixing the original problem in Mbed Crypto: // https://github.com/ARMmbed/mbed-crypto/issues/266 key_handle_mutex: Mutex<()>, - // As mentioned above, calls dealing with key slot allocation are not secured for concurrency. - // `key_slot_semaphore` is used to ensure that only `PSA_KEY_SLOT_COUNT` threads can have slots - // assigned at any time. - #[derivative(Debug = "ignore")] - key_slot_semaphore: Semaphore, + + // Holds the highest ID of all keys (including destroyed keys). New keys will receive an ID of + // id_counter + 1. Once id_counter reaches the highest allowed ID, no more keys can be created. + id_counter: AtomicU32, } impl MbedProvider { @@ -70,10 +68,10 @@ impl MbedProvider { } let mbed_provider = MbedProvider { key_info_store, - local_ids: RwLock::new(HashSet::new()), key_handle_mutex: Mutex::new(()), - key_slot_semaphore: Semaphore::new(PSA_KEY_SLOT_COUNT), + id_counter: AtomicU32::new(key::PSA_KEY_ID_USER_MIN), }; + let mut max_key_id: key::psa_key_id_t = key::PSA_KEY_ID_USER_MIN; { // The local scope allows to drop store_handle and local_ids_handle in order to return // the mbed_provider. @@ -81,10 +79,6 @@ impl MbedProvider { .key_info_store .write() .expect("Key store lock poisoned"); - let mut local_ids_handle = mbed_provider - .local_ids - .write() - .expect("Local ID lock poisoned"); let mut to_remove: Vec = Vec::new(); // Go through all MbedProvider key triple to key info mappings and check if they are still // present. @@ -104,7 +98,9 @@ impl MbedProvider { let pc_key_id = key::Id::from_persistent_key_id(key_id); match key::Attributes::from_key_id(pc_key_id) { Ok(_) => { - let _ = local_ids_handle.insert(key_id); + if key_id > max_key_id { + max_key_id = key_id; + } } Err(status::Error::DoesNotExist) => to_remove.push(key_triple.clone()), Err(e) => { @@ -129,7 +125,7 @@ impl MbedProvider { } } } - + mbed_provider.id_counter.store(max_key_id, Relaxed); Some(mbed_provider) } }