The branch master has been updated via c2ec2bb7c146d1e48568f27d11dca02c06c36338 (commit) via d60a8e0a2345205242e21aae35815645708580c4 (commit) via 2f17e978a0ec5becda8a61dcf3e7840740ccdfd3 (commit) from 8c631cfaa1f812ed990053c1b0c73f3a3f369aca (commit)
- Log ----------------------------------------------------------------- commit c2ec2bb7c146d1e48568f27d11dca02c06c36338 Author: Richard Levitte <levi...@openssl.org> Date: Mon Mar 1 13:27:24 2021 +0100 Make provider provider_init thread safe, and flag checking/setting too provider_init() makes changes in the provider structure, and needs a bit of protection to ensure that doesn't happen concurrently with race conditions. This also demands a bit of protection of the flags, since they are bits and presumably occupy the same byte in memory. Reviewed-by: Paul Dale <pa...@openssl.org> Reviewed-by: Matt Caswell <m...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/14354) commit d60a8e0a2345205242e21aae35815645708580c4 Author: Richard Levitte <levi...@openssl.org> Date: Mon Mar 1 13:27:15 2021 +0100 Make ossl_provider_disable_fallback_loading() thread safe Reviewed-by: Paul Dale <pa...@openssl.org> Reviewed-by: Matt Caswell <m...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/14354) commit 2f17e978a0ec5becda8a61dcf3e7840740ccdfd3 Author: Richard Levitte <levi...@openssl.org> Date: Mon Mar 1 16:31:34 2021 +0100 test/threadstest.c: Add a test to load providers concurrently If we don't synchronize properly in the core provider code, and build with a thread sanitizer, this should cause a crash. Reviewed-by: Paul Dale <pa...@openssl.org> Reviewed-by: Matt Caswell <m...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/14354) ----------------------------------------------------------------------- Summary of changes: crypto/provider_core.c | 48 +++++++++++++++++++++++++++++++++++++----------- test/threadstest.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/crypto/provider_core.c b/crypto/provider_core.c index d210026e25..1326f83f7e 100644 --- a/crypto/provider_core.c +++ b/crypto/provider_core.c @@ -48,6 +48,9 @@ struct ossl_provider_st { unsigned int flag_fallback:1; /* Can be used as fallback */ unsigned int flag_activated_as_fallback:1; + /* Getting and setting the flags require synchronization */ + CRYPTO_RWLOCK *flag_lock; + /* OpenSSL library side data */ CRYPTO_REF_COUNT refcnt; CRYPTO_RWLOCK *refcnt_lock; /* For the ref counter */ @@ -201,7 +204,9 @@ int ossl_provider_disable_fallback_loading(OSSL_LIB_CTX *libctx) struct provider_store_st *store; if ((store = get_provider_store(libctx)) != NULL) { + CRYPTO_THREAD_write_lock(store->lock); store->use_fallbacks = 0; + CRYPTO_THREAD_unlock(store->lock); return 1; } return 0; @@ -255,6 +260,7 @@ static OSSL_PROVIDER *provider_new(const char *name, #endif || !ossl_provider_up_ref(prov) /* +1 One reference to be returned */ || (prov->opbits_lock = CRYPTO_THREAD_lock_new()) == NULL + || (prov->flag_lock = CRYPTO_THREAD_lock_new()) == NULL || (prov->name = OPENSSL_strdup(name)) == NULL) { ossl_provider_free(prov); ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE); @@ -375,6 +381,7 @@ void ossl_provider_free(OSSL_PROVIDER *prov) OPENSSL_free(prov->path); sk_INFOPAIR_pop_free(prov->parameters, free_infopair); CRYPTO_THREAD_lock_free(prov->opbits_lock); + CRYPTO_THREAD_lock_free(prov->flag_lock); #ifndef HAVE_ATOMICS CRYPTO_THREAD_lock_free(prov->refcnt_lock); CRYPTO_THREAD_lock_free(prov->activatecnt_lock); @@ -470,9 +477,19 @@ static int provider_init(OSSL_PROVIDER *prov) OSSL_FUNC_provider_get_reason_strings_fn *p_get_reason_strings = NULL; # endif #endif + int ok = 0; - if (prov->flag_initialized) - return 1; + /* + * The flag lock is used to lock init, not only because the flag is + * checked here and set at the end, but also because this function + * modifies a number of things in the provider structure that this + * function needs to perform under lock anyway. + */ + CRYPTO_THREAD_write_lock(prov->flag_lock); + if (prov->flag_initialized) { + ok = 1; + goto end; + } /* * If the init function isn't set, it indicates that this provider is @@ -480,7 +497,7 @@ static int provider_init(OSSL_PROVIDER *prov) */ if (prov->init_function == NULL) { #ifdef FIPS_MODULE - return 0; + goto end; #else if (prov->module == NULL) { char *allocated_path = NULL; @@ -491,13 +508,14 @@ static int provider_init(OSSL_PROVIDER *prov) if ((prov->module = DSO_new()) == NULL) { /* DSO_new() generates an error already */ - return 0; + goto end; } if ((store = get_provider_store(prov->libctx)) == NULL || !CRYPTO_THREAD_read_lock(store->lock)) - return 0; + goto end; load_dir = store->default_path; + CRYPTO_THREAD_unlock(store->lock); if (load_dir == NULL) { load_dir = ossl_safe_getenv("OPENSSL_MODULES"); @@ -514,7 +532,6 @@ static int provider_init(OSSL_PROVIDER *prov) DSO_convert_filename(prov->module, prov->name); if (module_path != NULL) merged_path = DSO_merge(prov->module, module_path, load_dir); - CRYPTO_THREAD_unlock(store->lock); if (merged_path == NULL || (DSO_load(prov->module, merged_path, NULL, 0)) == NULL) { @@ -542,7 +559,7 @@ static int provider_init(OSSL_PROVIDER *prov) DSO_free(prov->module); prov->module = NULL; #endif - return 0; + goto end; } prov->provctx = tmp_provctx; @@ -603,7 +620,7 @@ static int provider_init(OSSL_PROVIDER *prov) cnt = 0; while (reasonstrings[cnt].id != 0) { if (ERR_GET_LIB(reasonstrings[cnt].id) != 0) - return 0; + goto end; cnt++; } cnt++; /* One for the terminating item */ @@ -612,7 +629,7 @@ static int provider_init(OSSL_PROVIDER *prov) prov->error_strings = OPENSSL_zalloc(sizeof(ERR_STRING_DATA) * (cnt + 1)); if (prov->error_strings == NULL) - return 0; + goto end; /* * Set the "library" name. @@ -635,7 +652,11 @@ static int provider_init(OSSL_PROVIDER *prov) /* With this flag set, this provider has become fully "loaded". */ prov->flag_initialized = 1; - return 1; + ok = 1; + + end: + CRYPTO_THREAD_unlock(prov->flag_lock); + return ok; } static int provider_deactivate(OSSL_PROVIDER *prov) @@ -648,8 +669,11 @@ static int provider_deactivate(OSSL_PROVIDER *prov) if (CRYPTO_DOWN_REF(&prov->activatecnt, &ref, prov->activatecnt_lock) <= 0) return 0; - if (ref < 1) + if (ref < 1) { + CRYPTO_THREAD_write_lock(prov->flag_lock); prov->flag_activated = 0; + CRYPTO_THREAD_unlock(prov->flag_lock); + } /* We don't deinit here, that's done in ossl_provider_free() */ return 1; @@ -663,7 +687,9 @@ static int provider_activate(OSSL_PROVIDER *prov) return 0; if (provider_init(prov)) { + CRYPTO_THREAD_write_lock(prov->flag_lock); prov->flag_activated = 1; + CRYPTO_THREAD_unlock(prov->flag_lock); return 1; } diff --git a/test/threadstest.c b/test/threadstest.c index 9c8e2181d0..26807e294c 100644 --- a/test/threadstest.c +++ b/test/threadstest.c @@ -473,6 +473,34 @@ static int test_multi(int idx) return testresult; } +/* + * This test attempts to load several providers at the same time, and if + * run with a thread sanitizer, should crash if the core provider code + * doesn't synchronize well enough. + */ +#define MULTI_LOAD_THREADS 3 +static void test_multi_load_worker(void) +{ + OSSL_PROVIDER *prov; + + TEST_ptr(prov = OSSL_PROVIDER_load(NULL, "default")); + TEST_true(OSSL_PROVIDER_unload(prov)); +} + +static int test_multi_load(void) +{ + thread_t threads[MULTI_LOAD_THREADS]; + int i; + + for (i = 0; i < MULTI_LOAD_THREADS; i++) + TEST_true(run_thread(&threads[i], test_multi_load_worker)); + + for (i = 0; i < MULTI_LOAD_THREADS; i++) + TEST_true(wait_for_thread(threads[i])); + + return 1; +} + typedef enum OPTION_choice { OPT_ERR = -1, OPT_EOF = 0, @@ -518,6 +546,7 @@ int setup_tests(void) ADD_TEST(test_once); ADD_TEST(test_thread_local); ADD_TEST(test_atomic); + ADD_TEST(test_multi_load); ADD_ALL_TESTS(test_multi, 4); return 1; }