Skip to content
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

Jump to bottom

Add locking around key handle operations in mbed provider #41

Merged
merged 2 commits into from
Oct 16, 2019
Merged

Add locking around key handle operations in mbed provider #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c7eb5ee
Add locking around key handle operations in mbed provider
ionut-arm Oct 15, 2019
3bb76f3
Merge branch 'master' into locks
ionut-arm Oct 16, 2019
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
7 changes: 7 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ rand = "0.7.2"
base64 = "0.10.1"
uuid = "0.7.4"
threadpool = "1.7.1"
std-semaphore = "0.1.0"

[dev-dependencies]
parsec-client-test = { git = "https://github.com/parallaxsecond/parsec-client-test", tag = "0.1.2" }
Expand Down
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ This project uses the following third party crates:
* base64 (MIT and Apache-2.0)
* uuid (Apache-2.0)
* threadpool (Apache-2.0)
* std-semaphore (MIT and Apache-2.0)
* num_cpus (MIT and Apache-2.0)

This project uses the following third party libraries:
Expand Down
1 change: 1 addition & 0 deletions src/providers/mbed_provider/constants.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub const PSA_ERROR_INVALID_PADDING: psa_status_t = -150;
pub const PSA_ERROR_INSUFFICIENT_DATA: psa_status_t = -143;
pub const PSA_ERROR_INVALID_HANDLE: psa_status_t = -136;

pub const PSA_KEY_SLOT_COUNT: isize = 32;
pub const EMPTY_KEY_HANDLE: psa_key_handle_t = 0;
pub const PSA_KEY_TYPE_NONE: psa_key_type_t = 0x0000_0000;
pub const PSA_KEY_TYPE_VENDOR_FLAG: psa_key_type_t = 0x8000_0000;
Expand Down
85 changes: 76 additions & 9 deletions src/providers/mbed_provider/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use crate::authenticators::ApplicationName;
use crate::key_id_managers::{KeyTriple, ManageKeyIDs};
use std::collections::HashSet;
use std::convert::TryInto;
use std::sync::{Arc, RwLock};
use std::sync::{Arc, Mutex, RwLock};

use parsec_interface::operations::key_attributes::KeyLifetime;
use parsec_interface::operations::ProviderInfo;
Expand All @@ -29,6 +29,7 @@ use parsec_interface::operations::{OpExportPublicKey, ResultExportPublicKey};
use parsec_interface::operations::{OpImportKey, ResultImportKey};
use parsec_interface::operations::{OpListOpcodes, ResultListOpcodes};
use parsec_interface::requests::{Opcode, ProviderID, ResponseStatus, Result};
use std_semaphore::Semaphore;
use uuid::Uuid;

#[allow(
Expand Down Expand Up @@ -63,6 +64,16 @@ pub struct MbedProvider {
// reference it to match with the method prototypes.
key_id_store: Arc<RwLock<dyn ManageKeyIDs + Send + Sync>>,
local_ids: RwLock<LocalIdStore>,
// Calls to `psa_open_key`, `psa_create_key` and `psa_close_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<()>,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be nice to add a comment on top of those lines to explain why the Mutex, Semaphore are needed. Just as an help for future code readers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

// 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.
key_slot_semaphore: Semaphore,
}

/// Gets a PSA Key ID from the Key ID Manager.
Expand Down Expand Up @@ -156,6 +167,8 @@ impl MbedProvider {
let mbed_provider = MbedProvider {
key_id_store,
local_ids: RwLock::new(HashSet::new()),
key_handle_mutex: Mutex::new(()),
key_slot_semaphore: Semaphore::new(constants::PSA_KEY_SLOT_COUNT),
};
{
// The local scope allows to drop store_handle and local_ids_handle in order to return
Expand Down Expand Up @@ -245,6 +258,7 @@ impl Provide for MbedProvider {

fn create_key(&self, app_name: ApplicationName, op: OpCreateKey) -> Result<ResultCreateKey> {
println!("Mbed Provider - Create Key");
let _semaphore_guard = self.key_slot_semaphore.access();
let key_name = op.key_name;
let key_attributes = op.key_attributes;
let key_triple = KeyTriple::new(app_name, ProviderID::MbedProvider, key_name);
Expand All @@ -263,6 +277,10 @@ impl Provide for MbedProvider {
let mut key_handle: psa_crypto_binding::psa_key_handle_t = 0;

let create_key_status = unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_create_key(key_attrs.key_lifetime, key_id, &mut key_handle)
};

Expand Down Expand Up @@ -307,6 +325,10 @@ impl Provide for MbedProvider {
}

unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_close_key(key_handle);
}
} else {
Expand All @@ -328,6 +350,7 @@ impl Provide for MbedProvider {

fn import_key(&self, app_name: ApplicationName, op: OpImportKey) -> Result<ResultImportKey> {
println!("Mbed Provider - Import Key");
let _semaphore_guard = self.key_slot_semaphore.access();
let key_name = op.key_name;
let key_attributes = op.key_attributes;
let key_data = op.key_data;
Expand All @@ -347,6 +370,10 @@ impl Provide for MbedProvider {
let mut key_handle: psa_crypto_binding::psa_key_handle_t = 0;

let create_key_status = unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_create_key(key_attrs.key_lifetime, key_id, &mut key_handle)
};

Expand Down Expand Up @@ -390,6 +417,10 @@ impl Provide for MbedProvider {
}

unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_close_key(key_handle);
}
} else {
Expand All @@ -415,6 +446,7 @@ impl Provide for MbedProvider {
op: OpExportPublicKey,
) -> Result<ResultExportPublicKey> {
println!("Mbed Provider - Export Public Key");
let _semaphore_guard = self.key_slot_semaphore.access();
let key_name = op.key_name;
let key_lifetime = op.key_lifetime;
let key_triple = KeyTriple::new(app_name, ProviderID::MbedProvider, key_name);
Expand All @@ -426,8 +458,13 @@ impl Provide for MbedProvider {

let ret_val: Result<ResultExportPublicKey>;

let open_key_status =
unsafe { psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle) };
let open_key_status = unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle)
};

if open_key_status == constants::PSA_SUCCESS {
// TODO: There's no calculation of the buffer size here, nor is there any handling of the
Expand Down Expand Up @@ -455,6 +492,10 @@ impl Provide for MbedProvider {
}

unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_close_key(key_handle);
}
} else {
Expand All @@ -467,6 +508,7 @@ impl Provide for MbedProvider {

fn destroy_key(&self, app_name: ApplicationName, op: OpDestroyKey) -> Result<ResultDestroyKey> {
println!("Mbed Provider - Destroy Key");
let _semaphore_guard = self.key_slot_semaphore.access();
let key_name = op.key_name;
let key_lifetime = op.key_lifetime;
let key_triple = KeyTriple::new(app_name, ProviderID::MbedProvider, key_name);
Expand All @@ -477,8 +519,13 @@ impl Provide for MbedProvider {
let lifetime = conversion_utils::convert_key_lifetime(key_lifetime);
let mut key_handle: psa_crypto_binding::psa_key_handle_t = 0;

let open_key_status =
unsafe { psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle) };
let open_key_status = unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle)
};

if open_key_status == constants::PSA_SUCCESS {
let destroy_key_status = unsafe { psa_crypto_binding::psa_destroy_key(key_handle) };
Expand All @@ -501,6 +548,7 @@ impl Provide for MbedProvider {

fn asym_sign(&self, app_name: ApplicationName, op: OpAsymSign) -> Result<ResultAsymSign> {
println!("Mbed Provider - Asym Sign");
let _semaphore_guard = self.key_slot_semaphore.access();
let key_name = op.key_name;
let key_lifetime = op.key_lifetime;
let hash = op.hash;
Expand All @@ -511,8 +559,13 @@ impl Provide for MbedProvider {
let lifetime = conversion_utils::convert_key_lifetime(key_lifetime);
let mut key_handle: psa_crypto_binding::psa_key_handle_t = 0;

let open_key_status =
unsafe { psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle) };
let open_key_status = unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle)
};

if open_key_status == constants::PSA_SUCCESS {
let mut policy = psa_crypto_binding::psa_key_policy_t {
Expand Down Expand Up @@ -543,6 +596,10 @@ impl Provide for MbedProvider {
};

unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_close_key(key_handle);
}

Expand All @@ -564,6 +621,7 @@ impl Provide for MbedProvider {

fn asym_verify(&self, app_name: ApplicationName, op: OpAsymVerify) -> Result<ResultAsymVerify> {
println!("Mbed Provider - Asym Verify");
let _semaphore_guard = self.key_slot_semaphore.access();
let key_name = op.key_name;
let key_lifetime = op.key_lifetime;
let hash = op.hash;
Expand All @@ -575,8 +633,13 @@ impl Provide for MbedProvider {
let lifetime = conversion_utils::convert_key_lifetime(key_lifetime);
let mut key_handle: psa_crypto_binding::psa_key_handle_t = 0;

let open_key_status =
unsafe { psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle) };
let open_key_status = unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle)
};

if open_key_status == constants::PSA_SUCCESS {
let mut policy = psa_crypto_binding::psa_key_policy_t {
Expand Down Expand Up @@ -605,6 +668,10 @@ impl Provide for MbedProvider {
};

unsafe {
let _guard = self
.key_handle_mutex
.lock()
.expect("Grabbing key handle mutex failed");
psa_crypto_binding::psa_close_key(key_handle);
}

Expand Down