From f5690fecb3b0db604e4826323ea5a5e2c403e62f Mon Sep 17 00:00:00 2001 From: Pauli Date: Tue, 13 Jun 2023 11:39:23 +1000 Subject: [PATCH] fips: use memory ordering rather than locks The FIPS provider accesses it's current state under lock. This is overkill, little or no synchronisation is actually required in practice (because it's essentially a read only setting). Switch to using TSAN operations in preference. Fixes #21179 Reviewed-by: Shane Lontis Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/21187) (cherry picked from commit 8e9ca334528e0a923c4deb0af250a60510974be0) --- providers/fips/self_test.c | 50 +++++++++++--------------------------- 1 file changed, 14 insertions(+), 36 deletions(-) diff --git a/providers/fips/self_test.c b/providers/fips/self_test.c index 10804d9f59fb4..9a55bfb79de89 100644 --- a/providers/fips/self_test.c +++ b/providers/fips/self_test.c @@ -17,6 +17,7 @@ #include #include #include "internal/e_os.h" +#include "internal/tsan_assist.h" #include "prov/providercommon.h" /* @@ -48,7 +49,6 @@ static int FIPS_conditional_error_check = 1; static CRYPTO_RWLOCK *self_test_lock = NULL; -static CRYPTO_RWLOCK *fips_state_lock = NULL; static unsigned char fixed_key[32] = { FIPS_KEY_ELEMENTS }; static CRYPTO_ONCE fips_self_test_init = CRYPTO_ONCE_STATIC_INIT; @@ -60,7 +60,6 @@ DEFINE_RUN_ONCE_STATIC(do_fips_self_test_init) * platform then we just leak it deliberately. */ self_test_lock = CRYPTO_THREAD_lock_new(); - fips_state_lock = CRYPTO_THREAD_lock_new(); return self_test_lock != NULL; } @@ -156,12 +155,12 @@ void __TERM__cleanup(void) { # define DEP_INITIAL_STATE FIPS_STATE_SELFTEST #endif -static int FIPS_state = DEP_INITIAL_STATE; +static TSAN_QUALIFIER int FIPS_state = DEP_INITIAL_STATE; #if defined(DEP_INIT_ATTRIBUTE) DEP_INIT_ATTRIBUTE void init(void) { - FIPS_state = FIPS_STATE_SELFTEST; + tsan_store(&FIPS_state, FIPS_STATE_SELFTEST); } #endif @@ -169,7 +168,6 @@ DEP_INIT_ATTRIBUTE void init(void) DEP_FINI_ATTRIBUTE void cleanup(void) { CRYPTO_THREAD_lock_free(self_test_lock); - CRYPTO_THREAD_lock_free(fips_state_lock); } #endif @@ -291,10 +289,7 @@ static int verify_integrity(OSSL_CORE_BIO *bio, OSSL_FUNC_BIO_read_ex_fn read_ex static void set_fips_state(int state) { - if (ossl_assert(CRYPTO_THREAD_write_lock(fips_state_lock) != 0)) { - FIPS_state = state; - CRYPTO_THREAD_unlock(fips_state_lock); - } + tsan_store(&FIPS_state, state); } /* This API is triggered either on loading of the FIPS module or on demand */ @@ -314,10 +309,7 @@ int SELF_TEST_post(SELF_TEST_POST_PARAMS *st, int on_demand_test) if (!RUN_ONCE(&fips_self_test_init, do_fips_self_test_init)) return 0; - if (!CRYPTO_THREAD_read_lock(fips_state_lock)) - return 0; - loclstate = FIPS_state; - CRYPTO_THREAD_unlock(fips_state_lock); + loclstate = tsan_load(&FIPS_state); if (loclstate == FIPS_STATE_RUNNING) { if (!on_demand_test) @@ -329,24 +321,17 @@ int SELF_TEST_post(SELF_TEST_POST_PARAMS *st, int on_demand_test) if (!CRYPTO_THREAD_write_lock(self_test_lock)) return 0; - if (!CRYPTO_THREAD_read_lock(fips_state_lock)) { - CRYPTO_THREAD_unlock(self_test_lock); - return 0; - } - if (FIPS_state == FIPS_STATE_RUNNING) { - CRYPTO_THREAD_unlock(fips_state_lock); + loclstate = tsan_load(&FIPS_state); + if (loclstate == FIPS_STATE_RUNNING) { if (!on_demand_test) { CRYPTO_THREAD_unlock(self_test_lock); return 1; } set_fips_state(FIPS_STATE_SELFTEST); - } else if (FIPS_state != FIPS_STATE_SELFTEST) { - CRYPTO_THREAD_unlock(fips_state_lock); + } else if (loclstate != FIPS_STATE_SELFTEST) { CRYPTO_THREAD_unlock(self_test_lock); ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_STATE); return 0; - } else { - CRYPTO_THREAD_unlock(fips_state_lock); } if (st == NULL @@ -469,20 +454,13 @@ void ossl_set_error_state(const char *type) int ossl_prov_is_running(void) { - int res; - static unsigned int rate_limit = 0; + int res, loclstate; + static TSAN_QUALIFIER unsigned int rate_limit = 0; - if (!CRYPTO_THREAD_read_lock(fips_state_lock)) - return 0; - res = FIPS_state == FIPS_STATE_RUNNING - || FIPS_state == FIPS_STATE_SELFTEST; - if (FIPS_state == FIPS_STATE_ERROR) { - CRYPTO_THREAD_unlock(fips_state_lock); - if (!CRYPTO_THREAD_write_lock(fips_state_lock)) - return 0; - if (rate_limit++ < FIPS_ERROR_REPORTING_RATE_LIMIT) + loclstate = tsan_load(&FIPS_state); + res = loclstate == FIPS_STATE_RUNNING || loclstate == FIPS_STATE_SELFTEST; + if (loclstate == FIPS_STATE_ERROR) + if (tsan_add(&rate_limit, 1) < FIPS_ERROR_REPORTING_RATE_LIMIT) ERR_raise(ERR_LIB_PROV, PROV_R_FIPS_MODULE_IN_ERROR_STATE); - } - CRYPTO_THREAD_unlock(fips_state_lock); return res; }