From 04b9435a991585d0f9a775a203cc3986d4872a6e Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 26 Jan 2021 17:00:25 +0000 Subject: Always ensure we hold ctx->lock when calling CRYPTO_get_ex_data() Otherwise we can get data races. Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/13987) --- crypto/context.c | 22 ++++++++++++++++++---- crypto/ex_data.c | 13 ++++++++++--- include/crypto/cryptlib.h | 3 +++ 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/crypto/context.c b/crypto/context.c index 7efe475b70..5a46921778 100644 --- a/crypto/context.c +++ b/crypto/context.c @@ -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]); diff --git a/crypto/ex_data.c b/crypto/ex_data.c index 5de99b4735..0d87ea7f0e 100644 --- a/crypto/ex_data.c +++ b/crypto/ex_data.c @@ -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; diff --git a/include/crypto/cryptlib.h b/include/crypto/cryptlib.h index 69d94be54a..8fd04fa16f 100644 --- a/include/crypto/cryptlib.h +++ b/include/crypto/cryptlib.h @@ -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); -- cgit v1.2.3