Skip to content

Changed local_ids for Atomic counter and removed key_slot_semaphore. #191

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 25, 2020
Merged

Changed local_ids for Atomic counter and removed key_slot_semaphore. #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/providers/mbed_provider/asym_sign.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
76 changes: 26 additions & 50 deletions src/providers/mbed_provider/key_management.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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::{
Expand All @@ -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
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is fine to not care about overflows as PSA_KEY_ID_USER_MAX is equal to the defined value of 0x3FFFFFFF.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better safe than sorry or overkill? 😉

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)),
}
}
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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;
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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");
Expand Down Expand Up @@ -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
Expand All @@ -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) => {
Expand Down
32 changes: 14 additions & 18 deletions src/providers/mbed_provider/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand All @@ -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.
Expand All @@ -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) => {
Expand All @@ -129,7 +125,7 @@ impl MbedProvider {
}
}
}

mbed_provider.id_counter.store(max_key_id, Relaxed);
Some(mbed_provider)
}
}
Expand Down