diff options
author | Matt Caswell <matt@openssl.org> | 2021-01-27 17:18:27 +0000 |
---|---|---|
committer | Matt Caswell <matt@openssl.org> | 2021-02-02 12:21:33 +0000 |
commit | 0b07db6f56e0240de6cc2ea122eee6431459ef20 (patch) | |
tree | 6303ccd6e4831899954437de1e7e5702ec3779ef | |
parent | 40994605140b9fcbe98a786dc75bdc1b9e9fee3f (diff) |
Ensure the EVP_PKEY operation_cache is appropriately locked
The EVP_PKEY operation_cache caches references to provider side key
objects that have previously been exported for this EVP_PKEY, and their
associated key managers. The cache may be updated from time to time as the
EVP_PKEY is exported to more providers. Since an EVP_PKEY may be shared by
multiple threads simultaneously we must be careful to ensure the cache
updates are locked.
Fixes #13818
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/13987)
-rw-r--r-- | crypto/evp/keymgmt_lib.c | 39 | ||||
-rw-r--r-- | crypto/evp/p_lib.c | 22 | ||||
-rw-r--r-- | doc/internal/man3/evp_keymgmt_util_export_to_provider.pod | 16 | ||||
-rw-r--r-- | include/crypto/evp.h | 2 |
4 files changed, 67 insertions, 12 deletions
diff --git a/crypto/evp/keymgmt_lib.c b/crypto/evp/keymgmt_lib.c index 763982e58f..0c643b3b49 100644 --- a/crypto/evp/keymgmt_lib.c +++ b/crypto/evp/keymgmt_lib.c @@ -102,10 +102,16 @@ void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt) return pk->keydata; /* If this key is already exported to |keymgmt|, no more to do */ + CRYPTO_THREAD_read_lock(pk->lock); i = evp_keymgmt_util_find_operation_cache_index(pk, keymgmt); if (i < OSSL_NELEM(pk->operation_cache) - && pk->operation_cache[i].keymgmt != NULL) - return pk->operation_cache[i].keydata; + && pk->operation_cache[i].keymgmt != NULL) { + void *ret = pk->operation_cache[i].keydata; + + CRYPTO_THREAD_unlock(pk->lock); + return ret; + } + CRYPTO_THREAD_unlock(pk->lock); /* If the "origin" |keymgmt| doesn't support exporting, give up */ /* @@ -153,20 +159,42 @@ void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt) return NULL; } + CRYPTO_THREAD_write_lock(pk->lock); + /* Check to make sure some other thread didn't get there first */ + i = evp_keymgmt_util_find_operation_cache_index(pk, keymgmt); + if (i < OSSL_NELEM(pk->operation_cache) + && pk->operation_cache[i].keymgmt != NULL) { + void *ret = pk->operation_cache[i].keydata; + + CRYPTO_THREAD_unlock(pk->lock); + + /* + * Another thread seemms to have already exported this so we abandon + * all the work we just did. + */ + evp_keymgmt_freedata(keymgmt, import_data.keydata); + + return ret; + } + /* Add the new export to the operation cache */ if (!evp_keymgmt_util_cache_keydata(pk, i, keymgmt, import_data.keydata)) { evp_keymgmt_freedata(keymgmt, import_data.keydata); return NULL; } + CRYPTO_THREAD_unlock(pk->lock); + return import_data.keydata; } -void evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk) +int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk, int locking) { size_t i, end = OSSL_NELEM(pk->operation_cache); if (pk != NULL) { + if (locking && pk->lock != NULL && !CRYPTO_THREAD_write_lock(pk->lock)) + return 0; for (i = 0; i < end && pk->operation_cache[i].keymgmt != NULL; i++) { EVP_KEYMGMT *keymgmt = pk->operation_cache[i].keymgmt; void *keydata = pk->operation_cache[i].keydata; @@ -176,7 +204,11 @@ void evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk) evp_keymgmt_freedata(keymgmt, keydata); EVP_KEYMGMT_free(keymgmt); } + if (locking && pk->lock != NULL) + CRYPTO_THREAD_unlock(pk->lock); } + + return 1; } size_t evp_keymgmt_util_find_operation_cache_index(EVP_PKEY *pk, @@ -198,6 +230,7 @@ int evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, size_t index, if (keydata != NULL) { if (!EVP_KEYMGMT_up_ref(keymgmt)) return 0; + pk->operation_cache[index].keydata = keydata; pk->operation_cache[index].keymgmt = keymgmt; } diff --git a/crypto/evp/p_lib.c b/crypto/evp/p_lib.c index 5df9b19eae..21ce51d573 100644 --- a/crypto/evp/p_lib.c +++ b/crypto/evp/p_lib.c @@ -1621,7 +1621,7 @@ static void evp_pkey_free_it(EVP_PKEY *x) { /* internal function; x is never NULL */ - evp_keymgmt_util_clear_operation_cache(x); + evp_keymgmt_util_clear_operation_cache(x, 1); #ifndef FIPS_MODULE evp_pkey_free_legacy(x); #endif @@ -1735,6 +1735,8 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx, * |i| remains zero, and we will clear the cache further down. */ if (pk->ameth->dirty_cnt(pk) == pk->dirty_cnt_copy) { + if (!CRYPTO_THREAD_read_lock(pk->lock)) + goto end; i = evp_keymgmt_util_find_operation_cache_index(pk, tmp_keymgmt); /* @@ -1746,8 +1748,10 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx, if (i < OSSL_NELEM(pk->operation_cache) && pk->operation_cache[i].keymgmt != NULL) { keydata = pk->operation_cache[i].keydata; + CRYPTO_THREAD_unlock(pk->lock); goto end; } + CRYPTO_THREAD_unlock(pk->lock); } /* @@ -1782,12 +1786,22 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx, keydata = NULL; goto end; } - if (pk->ameth->dirty_cnt(pk) != pk->dirty_cnt_copy) - evp_keymgmt_util_clear_operation_cache(pk); + + if (!CRYPTO_THREAD_write_lock(pk->lock)) + goto end; + if (pk->ameth->dirty_cnt(pk) != pk->dirty_cnt_copy + && !evp_keymgmt_util_clear_operation_cache(pk, 0)) { + CRYPTO_THREAD_unlock(pk->lock); + evp_keymgmt_freedata(tmp_keymgmt, keydata); + keydata = NULL; + EVP_KEYMGMT_free(tmp_keymgmt); + goto end; + } EVP_KEYMGMT_free(tmp_keymgmt); /* refcnt-- */ /* Add the new export to the operation cache */ if (!evp_keymgmt_util_cache_keydata(pk, i, tmp_keymgmt, keydata)) { + CRYPTO_THREAD_unlock(pk->lock); evp_keymgmt_freedata(tmp_keymgmt, keydata); keydata = NULL; goto end; @@ -1795,6 +1809,8 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx, /* Synchronize the dirty count */ pk->dirty_cnt_copy = pk->ameth->dirty_cnt(pk); + + CRYPTO_THREAD_unlock(pk->lock); goto end; } #endif /* FIPS_MODULE */ diff --git a/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod b/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod index bb2ad9ba8e..31f8b00e47 100644 --- a/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod +++ b/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod @@ -20,9 +20,9 @@ evp_keymgmt_util_fromdata void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt); size_t evp_keymgmt_util_find_operation_cache_index(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt); - void evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk); - void evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, size_t index, - EVP_KEYMGMT *keymgmt, void *keydata); + int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk, int locking); + int evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, size_t index, + EVP_KEYMGMT *keymgmt, void *keydata); void evp_keymgmt_util_cache_keyinfo(EVP_PKEY *pk); void *evp_keymgmt_util_fromdata(EVP_PKEY *target, EVP_KEYMGMT *keymgmt, int selection, const OSSL_PARAM params[]); @@ -44,10 +44,13 @@ as this function ignores any legacy key data. evp_keymgmt_util_find_operation_cache_index() finds the location if I<keymgmt> in I<pk>'s cache of provided keys for operations. If I<keymgmt> is NULL or couldn't be found in the cache, it finds the -first empty slot instead if there is any. +first empty slot instead if there is any. It should only be called while +holding I<pk>'s lock (read or write). evp_keymgmt_util_clear_operation_cache() can be used to explicitly -clear the cache of operation key references. +clear the cache of operation key references. If I<locking> is set to 1 then +then I<pk>'s lock will be obtained while doing the clear. Otherwise it will be +assumed that the lock has already been obtained or is not required. evp_keymgmt_util_cache_keydata() can be used to assign a provider key object to a specific cache slot in the given I<target>. @@ -72,6 +75,9 @@ operation cache slot. If I<keymgmt> is NULL, or if there is no slot with a match for I<keymgmt>, the index of the first empty slot is returned, or the maximum number of slots if there isn't an empty one. +evp_keymgmt_util_cache_keydata() and evp_keymgmt_util_clear_operation_cache() +return 1 on success or 0 otherwise. + =head1 NOTES "Legacy key" is the term used for any key that has been assigned to an diff --git a/include/crypto/evp.h b/include/crypto/evp.h index 20335e9a32..bed75f406c 100644 --- a/include/crypto/evp.h +++ b/include/crypto/evp.h @@ -729,7 +729,7 @@ int evp_keymgmt_util_export(const EVP_PKEY *pk, int selection, void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt); size_t evp_keymgmt_util_find_operation_cache_index(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt); -void evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk); +int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk, int locking); int evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, size_t index, EVP_KEYMGMT *keymgmt, void *keydata); void evp_keymgmt_util_cache_keyinfo(EVP_PKEY *pk); |