Skip to content

Commit c7eb5ee

Browse files
ionut-armionut-arm
committed
Add locking around key handle operations in mbed provider
This commit adds locking around the operations that deal with allocating or closing key handles in the mbed provider. Said calls are not thread safe, so a mutex protects access to the calls and a semaphore limits the number of threads that can hold key slots. Change-Id: Icbcad5c05179275d2138d830b33e43976329e931 Signed-off-by: Ionut Mihalcea <[email protected]>
1 parent 71093b4 commit c7eb5ee

File tree

5 files changed

+86
-9
lines changed

5 files changed

+86
-9
lines changed

‎Cargo.lock

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ rand = "0.7.0"
1111
base64 = "0.10.1"
1212
uuid = "0.7.4"
1313
threadpool = "1.7.1"
14+
std-semaphore = "0.1.0"
1415

1516
[dev-dependencies]
1617
parsec-client-test = { git = "https://github.com/parallaxsecond/parsec-client-test", tag = "0.1.1" }

‎README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ This project uses the following third party crates:
136136
* base64 (MIT and Apache-2.0)
137137
* uuid (Apache-2.0)
138138
* threadpool (Apache-2.0)
139+
* std-semaphore (MIT and Apache-2.0)
139140

140141
This project uses the following third party libraries:
141142
* [Mbed Crypto](https://github.com/ARMmbed/mbed-crypto) (Apache-2.0)

‎src/providers/mbed_provider/constants.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ pub const PSA_ERROR_INVALID_PADDING: psa_status_t = -150;
3838
pub const PSA_ERROR_INSUFFICIENT_DATA: psa_status_t = -143;
3939
pub const PSA_ERROR_INVALID_HANDLE: psa_status_t = -136;
4040

41+
pub const PSA_KEY_SLOT_COUNT: isize = 32;
4142
pub const EMPTY_KEY_HANDLE: psa_key_handle_t = 0;
4243
pub const PSA_KEY_TYPE_NONE: psa_key_type_t = 0x0000_0000;
4344
pub const PSA_KEY_TYPE_VENDOR_FLAG: psa_key_type_t = 0x8000_0000;

‎src/providers/mbed_provider/mod.rs

Lines changed: 76 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use crate::authenticators::ApplicationName;
1717
use crate::key_id_managers::{KeyTriple, ManageKeyIDs};
1818
use std::collections::HashSet;
1919
use std::convert::TryInto;
20-
use std::sync::{Arc, RwLock};
20+
use std::sync::{Arc, Mutex, RwLock};
2121

2222
use parsec_interface::operations::key_attributes::KeyLifetime;
2323
use parsec_interface::operations::ProviderInfo;
@@ -29,6 +29,7 @@ use parsec_interface::operations::{OpExportPublicKey, ResultExportPublicKey};
2929
use parsec_interface::operations::{OpImportKey, ResultImportKey};
3030
use parsec_interface::operations::{OpListOpcodes, ResultListOpcodes};
3131
use parsec_interface::requests::{Opcode, ProviderID, ResponseStatus, Result};
32+
use std_semaphore::Semaphore;
3233
use uuid::Uuid;
3334

3435
#[allow(
@@ -63,6 +64,16 @@ pub struct MbedProvider {
6364
// reference it to match with the method prototypes.
6465
key_id_store: Arc<RwLock<dyn ManageKeyIDs + Send + Sync>>,
6566
local_ids: RwLock<LocalIdStore>,
67+
// Calls to `psa_open_key`, `psa_create_key` and `psa_close_key` are not thread safe - the slot
68+
// allocation mechanism in Mbed Crypto can return the same key slot for overlapping calls.
69+
// `key_handle_mutex` is use as a way of securing access to said operations among the threads.
70+
// This issue tracks progress on fixing the original problem in Mbed Crypto:
71+
// https://github.com/ARMmbed/mbed-crypto/issues/266
72+
key_handle_mutex: Mutex<()>,
73+
// As mentioned above, calls dealing with key slot allocation are not secured for concurrency.
74+
// `key_slot_semaphore` is used to ensure that only `PSA_KEY_SLOT_COUNT` threads can have slots
75+
// assigned at any time.
76+
key_slot_semaphore: Semaphore,
6677
}
6778

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

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

265279
let create_key_status = unsafe {
280+
let _guard = self
281+
.key_handle_mutex
282+
.lock()
283+
.expect("Grabbing key handle mutex failed");
266284
psa_crypto_binding::psa_create_key(key_attrs.key_lifetime, key_id, &mut key_handle)
267285
};
268286

@@ -307,6 +325,10 @@ impl Provide for MbedProvider {
307325
}
308326

309327
unsafe {
328+
let _guard = self
329+
.key_handle_mutex
330+
.lock()
331+
.expect("Grabbing key handle mutex failed");
310332
psa_crypto_binding::psa_close_key(key_handle);
311333
}
312334
} else {
@@ -328,6 +350,7 @@ impl Provide for MbedProvider {
328350

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

349372
let create_key_status = unsafe {
373+
let _guard = self
374+
.key_handle_mutex
375+
.lock()
376+
.expect("Grabbing key handle mutex failed");
350377
psa_crypto_binding::psa_create_key(key_attrs.key_lifetime, key_id, &mut key_handle)
351378
};
352379

@@ -390,6 +417,10 @@ impl Provide for MbedProvider {
390417
}
391418

392419
unsafe {
420+
let _guard = self
421+
.key_handle_mutex
422+
.lock()
423+
.expect("Grabbing key handle mutex failed");
393424
psa_crypto_binding::psa_close_key(key_handle);
394425
}
395426
} else {
@@ -415,6 +446,7 @@ impl Provide for MbedProvider {
415446
op: OpExportPublicKey,
416447
) -> Result<ResultExportPublicKey> {
417448
println!("Mbed Provider - Export Public Key");
449+
let _semaphore_guard = self.key_slot_semaphore.access();
418450
let key_name = op.key_name;
419451
let key_lifetime = op.key_lifetime;
420452
let key_triple = KeyTriple::new(app_name, ProviderID::MbedProvider, key_name);
@@ -426,8 +458,13 @@ impl Provide for MbedProvider {
426458

427459
let ret_val: Result<ResultExportPublicKey>;
428460

429-
let open_key_status =
430-
unsafe { psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle) };
461+
let open_key_status = unsafe {
462+
let _guard = self
463+
.key_handle_mutex
464+
.lock()
465+
.expect("Grabbing key handle mutex failed");
466+
psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle)
467+
};
431468

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

457494
unsafe {
495+
let _guard = self
496+
.key_handle_mutex
497+
.lock()
498+
.expect("Grabbing key handle mutex failed");
458499
psa_crypto_binding::psa_close_key(key_handle);
459500
}
460501
} else {
@@ -467,6 +508,7 @@ impl Provide for MbedProvider {
467508

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

480-
let open_key_status =
481-
unsafe { psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle) };
522+
let open_key_status = unsafe {
523+
let _guard = self
524+
.key_handle_mutex
525+
.lock()
526+
.expect("Grabbing key handle mutex failed");
527+
psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle)
528+
};
482529

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

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

514-
let open_key_status =
515-
unsafe { psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle) };
562+
let open_key_status = unsafe {
563+
let _guard = self
564+
.key_handle_mutex
565+
.lock()
566+
.expect("Grabbing key handle mutex failed");
567+
psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle)
568+
};
516569

517570
if open_key_status == constants::PSA_SUCCESS {
518571
let mut policy = psa_crypto_binding::psa_key_policy_t {
@@ -543,6 +596,10 @@ impl Provide for MbedProvider {
543596
};
544597

545598
unsafe {
599+
let _guard = self
600+
.key_handle_mutex
601+
.lock()
602+
.expect("Grabbing key handle mutex failed");
546603
psa_crypto_binding::psa_close_key(key_handle);
547604
}
548605

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

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

578-
let open_key_status =
579-
unsafe { psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle) };
636+
let open_key_status = unsafe {
637+
let _guard = self
638+
.key_handle_mutex
639+
.lock()
640+
.expect("Grabbing key handle mutex failed");
641+
psa_crypto_binding::psa_open_key(lifetime, key_id, &mut key_handle)
642+
};
580643

581644
if open_key_status == constants::PSA_SUCCESS {
582645
let mut policy = psa_crypto_binding::psa_key_policy_t {
@@ -605,6 +668,10 @@ impl Provide for MbedProvider {
605668
};
606669

607670
unsafe {
671+
let _guard = self
672+
.key_handle_mutex
673+
.lock()
674+
.expect("Grabbing key handle mutex failed");
608675
psa_crypto_binding::psa_close_key(key_handle);
609676
}
610677

0 commit comments

Comments
 (0)