Always ensure we hold ctx->lock when calling CRYPTO_get_ex_data()

Otherwise we can get data races.

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/13987)
This commit is contained in:
Matt Caswell 2021-01-26 17:00:25 +00:00
parent b233ea8276
commit 04b9435a99
3 changed files with 31 additions and 7 deletions

View File

@ -283,7 +283,9 @@ void *ossl_lib_ctx_get_data(OSSL_LIB_CTX *ctx, int index,
if (dynidx != -1) {
CRYPTO_THREAD_read_lock(ctx->index_locks[index]);
CRYPTO_THREAD_read_lock(ctx->lock);
data = CRYPTO_get_ex_data(&ctx->data, dynidx);
CRYPTO_THREAD_unlock(ctx->lock);
CRYPTO_THREAD_unlock(ctx->index_locks[index]);
return data;
}
@ -293,8 +295,8 @@ void *ossl_lib_ctx_get_data(OSSL_LIB_CTX *ctx, int index,
dynidx = ctx->dyn_indexes[index];
if (dynidx != -1) {
CRYPTO_THREAD_unlock(ctx->lock);
data = CRYPTO_get_ex_data(&ctx->data, dynidx);
CRYPTO_THREAD_unlock(ctx->lock);
CRYPTO_THREAD_unlock(ctx->index_locks[index]);
return data;
}
@ -307,10 +309,22 @@ void *ossl_lib_ctx_get_data(OSSL_LIB_CTX *ctx, int index,
CRYPTO_THREAD_unlock(ctx->lock);
/* The alloc call ensures there's a value there */
if (CRYPTO_alloc_ex_data(CRYPTO_EX_INDEX_OSSL_LIB_CTX, NULL,
&ctx->data, ctx->dyn_indexes[index]))
/*
* The alloc call ensures there's a value there. We release the ctx->lock
* for this, because the allocation itself may recursively call
* ossl_lib_ctx_get_data for other indexes (never this one). The allocation
* will itself aquire the ctx->lock when it actually comes to store the
* allocated data (see ossl_lib_ctx_generic_new() above). We call
* ossl_crypto_alloc_ex_data_intern() here instead of CRYPTO_alloc_ex_data().
* They do the same thing except that the latter calls CRYPTO_get_ex_data()
* as well - which we must not do without holding the ctx->lock.
*/
if (ossl_crypto_alloc_ex_data_intern(CRYPTO_EX_INDEX_OSSL_LIB_CTX, NULL,
&ctx->data, ctx->dyn_indexes[index])) {
CRYPTO_THREAD_read_lock(ctx->lock);
data = CRYPTO_get_ex_data(&ctx->data, ctx->dyn_indexes[index]);
CRYPTO_THREAD_unlock(ctx->lock);
}
CRYPTO_THREAD_unlock(ctx->index_locks[index]);

View File

@ -392,16 +392,23 @@ void CRYPTO_free_ex_data(int class_index, void *obj, CRYPTO_EX_DATA *ad)
int CRYPTO_alloc_ex_data(int class_index, void *obj, CRYPTO_EX_DATA *ad,
int idx)
{
EX_CALLBACK *f;
EX_CALLBACKS *ip;
void *curval;
OSSL_EX_DATA_GLOBAL *global;
curval = CRYPTO_get_ex_data(ad, idx);
/* Already there, no need to allocate */
if (curval != NULL)
return 1;
return ossl_crypto_alloc_ex_data_intern(class_index, obj, ad, idx);
}
int ossl_crypto_alloc_ex_data_intern(int class_index, void *obj,
CRYPTO_EX_DATA *ad, int idx)
{
EX_CALLBACK *f;
EX_CALLBACKS *ip;
OSSL_EX_DATA_GLOBAL *global;
global = ossl_lib_ctx_get_ex_data_global(ad->ctx);
if (global == NULL)
return 0;

View File

@ -29,3 +29,6 @@ void ossl_ctx_thread_stop(void *arg);
void ossl_trace_cleanup(void);
void ossl_malloc_setup_failures(void);
int ossl_crypto_alloc_ex_data_intern(int class_index, void *obj,
CRYPTO_EX_DATA *ad, int idx);