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)

diff --git a/crypto/context.c b/crypto/context.c
index 7efe475b70bcbd33d24994a1670b328a6281de70..5a46921778a6fe7fa0ebdc2b2d10baa5348797e5 100644 (file)
--- 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 5de99b4735df5b74ea9327b88ccccf677e7144a3..0d87ea7f0ebe71fde9bb19e1fcdabcf24192b98b 100644 (file)
--- 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 69d94be54a8a28d50e2978dedf9707869e7b1923..8fd04fa16f987162c826a4d47398ff77b0cce74a 100644 (file)
--- 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);