Merge pull request #191 from sbailey-arm/further-simplify-mbedcrypto-provider

Changed local_ids for Atomic counter and removed key_slot_semaphore.

Author: ionut-arm

src/providers/mbed_provider/asym_sign.rs

@@ -16,7 +16,6 @@ impl MbedProvider {
         op: psa_sign_hash::Operation,
     ) -> Result<psa_sign_hash::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<psa_verify_hash::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;

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,42 +46,40 @@ 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<key::psa_key_id_t> {
-    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) {
         Ok(insert_option) => {
             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,23 +97,21 @@ impl MbedProvider {
         op: psa_generate_key::Operation,
     ) -> Result<psa_generate_key::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);
         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");
         if key_info_exists(&key_triple, &*store_handle)? {
             return Err(ResponseStatus::PsaErrorAlreadyExists);
         }
         let key_id = create_key_id(
             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<psa_import_key::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,15 +144,14 @@ 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);
         }
         let key_id = create_key_id(
             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<psa_export_public_key::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<psa_destroy_key::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) => {

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<key::psa_key_id_t>;
-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<RwLock<dyn ManageKeyInfo + Send + Sync>>,
-    local_ids: RwLock<LocalIdStore>,
     // 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,21 +68,17 @@ 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.
             let mut store_handle = mbed_provider
                 .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<KeyTriple> = 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)
     }
 }