Skip to content

Commit

Permalink
fips: use memory ordering rather than locks
Browse files Browse the repository at this point in the history
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 <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21187)

(cherry picked from commit 8e9ca33)
  • Loading branch information
paulidale committed Jun 14, 2023
1 parent c553c08 commit f5690fe
Showing 1 changed file with 14 additions and 36 deletions.
50 changes: 14 additions & 36 deletions providers/fips/self_test.c
Expand Up @@ -17,6 +17,7 @@
#include <openssl/proverr.h>
#include <openssl/rand.h>
#include "internal/e_os.h"
#include "internal/tsan_assist.h"
#include "prov/providercommon.h"

/*
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}

Expand Down Expand Up @@ -156,20 +155,19 @@ 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

#if defined(DEP_FINI_ATTRIBUTE)
DEP_FINI_ATTRIBUTE void cleanup(void)
{
CRYPTO_THREAD_lock_free(self_test_lock);
CRYPTO_THREAD_lock_free(fips_state_lock);
}
#endif

Expand Down Expand Up @@ -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 */
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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;
}

0 comments on commit f5690fe

Please sign in to comment.