From 078439d78d1d1435f0ebaf97819daa38a8c81ad5 Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 13 Jan 2022 12:19:23 +1100 Subject: ssl: better support TSAN operations For platforms that do not have native TSAN support, locking needs to be used instead. This adds the locking. Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/17489) (cherry picked from commit acce055778ecbf72e06a254b3a9bf2a2907e5170) --- ssl/ssl_lib.c | 47 +++++++++++++++++++++++++++++++++++------------ ssl/ssl_local.h | 30 ++++++++++++++++++++++++++++++ ssl/ssl_sess.c | 11 ++++++----- ssl/statem/extensions.c | 13 +++++++++++-- ssl/statem/statem_clnt.c | 2 +- ssl/statem/statem_lib.c | 17 ++++++++++------- 6 files changed, 93 insertions(+), 27 deletions(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index f3993f0bc3..14030f8ebc 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -2451,6 +2451,17 @@ LHASH_OF(SSL_SESSION) *SSL_CTX_sessions(SSL_CTX *ctx) return ctx->sessions; } +static int ssl_tsan_load(SSL_CTX *ctx, TSAN_QUALIFIER int *stat) +{ + int res = 0; + + if (ssl_tsan_lock(ctx)) { + res = tsan_load(stat); + ssl_tsan_unlock(ctx); + } + return res; +} + long SSL_CTX_ctrl(SSL_CTX *ctx, int cmd, long larg, void *parg) { long l; @@ -2506,27 +2517,27 @@ long SSL_CTX_ctrl(SSL_CTX *ctx, int cmd, long larg, void *parg) case SSL_CTRL_SESS_NUMBER: return lh_SSL_SESSION_num_items(ctx->sessions); case SSL_CTRL_SESS_CONNECT: - return tsan_load(&ctx->stats.sess_connect); + return ssl_tsan_load(ctx, &ctx->stats.sess_connect); case SSL_CTRL_SESS_CONNECT_GOOD: - return tsan_load(&ctx->stats.sess_connect_good); + return ssl_tsan_load(ctx, &ctx->stats.sess_connect_good); case SSL_CTRL_SESS_CONNECT_RENEGOTIATE: - return tsan_load(&ctx->stats.sess_connect_renegotiate); + return ssl_tsan_load(ctx, &ctx->stats.sess_connect_renegotiate); case SSL_CTRL_SESS_ACCEPT: - return tsan_load(&ctx->stats.sess_accept); + return ssl_tsan_load(ctx, &ctx->stats.sess_accept); case SSL_CTRL_SESS_ACCEPT_GOOD: - return tsan_load(&ctx->stats.sess_accept_good); + return ssl_tsan_load(ctx, &ctx->stats.sess_accept_good); case SSL_CTRL_SESS_ACCEPT_RENEGOTIATE: - return tsan_load(&ctx->stats.sess_accept_renegotiate); + return ssl_tsan_load(ctx, &ctx->stats.sess_accept_renegotiate); case SSL_CTRL_SESS_HIT: - return tsan_load(&ctx->stats.sess_hit); + return ssl_tsan_load(ctx, &ctx->stats.sess_hit); case SSL_CTRL_SESS_CB_HIT: - return tsan_load(&ctx->stats.sess_cb_hit); + return ssl_tsan_load(ctx, &ctx->stats.sess_cb_hit); case SSL_CTRL_SESS_MISSES: - return tsan_load(&ctx->stats.sess_miss); + return ssl_tsan_load(ctx, &ctx->stats.sess_miss); case SSL_CTRL_SESS_TIMEOUTS: - return tsan_load(&ctx->stats.sess_timeout); + return ssl_tsan_load(ctx, &ctx->stats.sess_timeout); case SSL_CTRL_SESS_CACHE_FULL: - return tsan_load(&ctx->stats.sess_cache_full); + return ssl_tsan_load(ctx, &ctx->stats.sess_cache_full); case SSL_CTRL_MODE: return (ctx->mode |= larg); case SSL_CTRL_CLEAR_MODE: @@ -3199,6 +3210,14 @@ SSL_CTX *SSL_CTX_new_ex(OSSL_LIB_CTX *libctx, const char *propq, return NULL; } +#ifdef TSAN_REQUIRES_LOCKING + ret->tsan_lock = CRYPTO_THREAD_lock_new(); + if (ret->tsan_lock == NULL) { + ERR_raise(ERR_LIB_SSL, ERR_R_MALLOC_FAILURE); + goto err; + } +#endif + ret->libctx = libctx; if (propq != NULL) { ret->propq = OPENSSL_strdup(propq); @@ -3465,6 +3484,9 @@ void SSL_CTX_free(SSL_CTX *a) OPENSSL_free(a->sigalg_lookup_cache); CRYPTO_THREAD_lock_free(a->lock); +#ifdef TSAN_REQUIRES_LOCKING + CRYPTO_THREAD_lock_free(a->tsan_lock); +#endif OPENSSL_free(a->propq); @@ -3733,11 +3755,12 @@ void ssl_update_cache(SSL *s, int mode) /* auto flush every 255 connections */ if ((!(i & SSL_SESS_CACHE_NO_AUTO_CLEAR)) && ((i & mode) == mode)) { TSAN_QUALIFIER int *stat; + if (mode & SSL_SESS_CACHE_CLIENT) stat = &s->session_ctx->stats.sess_connect_good; else stat = &s->session_ctx->stats.sess_accept_good; - if ((tsan_load(stat) & 0xff) == 0xff) + if ((ssl_tsan_load(s->session_ctx, stat) & 0xff) == 0xff) SSL_CTX_flush_sessions(s->session_ctx, (unsigned long)time(NULL)); } } diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h index ce93049180..9f119a9d79 100644 --- a/ssl/ssl_local.h +++ b/ssl/ssl_local.h @@ -898,6 +898,9 @@ struct ssl_ctx_st { * other processes - spooky * :-) */ } stats; +#ifdef TSAN_REQUIRES_LOCKING + CRYPTO_RWLOCK *tsan_lock; +#endif CRYPTO_REF_COUNT references; @@ -2847,4 +2850,31 @@ void ssl_session_calculate_timeout(SSL_SESSION* ss); # define ssl3_setup_buffers SSL_test_functions()->p_ssl3_setup_buffers # endif + +/* Some helper routines to support TSAN operations safely */ +static ossl_unused ossl_inline int ssl_tsan_lock(const SSL_CTX *ctx) +{ +#ifdef TSAN_REQUIRES_LOCKING + if (!CRYPTO_THREAD_write_lock(ctx->tsan_lock)) + return 0; +#endif + return 1; +} + +static ossl_unused ossl_inline void ssl_tsan_unlock(const SSL_CTX *ctx) +{ +#ifdef TSAN_REQUIRES_LOCKING + CRYPTO_THREAD_unlock(ctx->tsan_lock); +#endif +} + +static ossl_unused ossl_inline void ssl_tsan_counter(const SSL_CTX *ctx, + TSAN_QUALIFIER int *stat) +{ + if (ssl_tsan_lock(ctx)) { + tsan_counter(stat); + ssl_tsan_unlock(ctx); + } +} + #endif diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 0e756c37c1..b9e8a9c9c5 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -502,7 +502,7 @@ SSL_SESSION *lookup_sess_in_cache(SSL *s, const unsigned char *sess_id, } CRYPTO_THREAD_unlock(s->session_ctx->lock); if (ret == NULL) - tsan_counter(&s->session_ctx->stats.sess_miss); + ssl_tsan_counter(s->session_ctx, &s->session_ctx->stats.sess_miss); } if (ret == NULL && s->session_ctx->get_session_cb != NULL) { @@ -511,7 +511,8 @@ SSL_SESSION *lookup_sess_in_cache(SSL *s, const unsigned char *sess_id, ret = s->session_ctx->get_session_cb(s, sess_id, sess_id_len, ©); if (ret != NULL) { - tsan_counter(&s->session_ctx->stats.sess_cb_hit); + ssl_tsan_counter(s->session_ctx, + &s->session_ctx->stats.sess_cb_hit); /* * Increment reference count now if the session callback asks us @@ -642,7 +643,7 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello) } if (sess_timedout(time(NULL), ret)) { - tsan_counter(&s->session_ctx->stats.sess_timeout); + ssl_tsan_counter(s->session_ctx, &s->session_ctx->stats.sess_timeout); if (try_session_cache) { /* session was from the cache, so remove it */ SSL_CTX_remove_session(s->session_ctx, ret); @@ -669,7 +670,7 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello) s->session = ret; } - tsan_counter(&s->session_ctx->stats.sess_hit); + ssl_tsan_counter(s->session_ctx, &s->session_ctx->stats.sess_hit); s->verify_result = s->session->verify_result; return 1; @@ -769,7 +770,7 @@ int SSL_CTX_add_session(SSL_CTX *ctx, SSL_SESSION *c) if (!remove_session_lock(ctx, ctx->session_cache_tail, 0)) break; else - tsan_counter(&ctx->stats.sess_cache_full); + ssl_tsan_counter(ctx, &ctx->stats.sess_cache_full); } } } diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index bc437be26a..0c8c2df767 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -897,6 +897,15 @@ static int final_renegotiate(SSL *s, unsigned int context, int sent) return 1; } +static ossl_inline void ssl_tsan_decr(const SSL_CTX *ctx, + TSAN_QUALIFIER int *stat) +{ + if (ssl_tsan_lock(ctx)) { + tsan_decr(stat); + ssl_tsan_unlock(ctx); + } +} + static int init_server_name(SSL *s, unsigned int context) { if (s->server) { @@ -954,8 +963,8 @@ static int final_server_name(SSL *s, unsigned int context, int sent) */ if (SSL_IS_FIRST_HANDSHAKE(s) && s->ctx != s->session_ctx && s->hello_retry_request == SSL_HRR_NONE) { - tsan_counter(&s->ctx->stats.sess_accept); - tsan_decr(&s->session_ctx->stats.sess_accept); + ssl_tsan_counter(s->ctx, &s->ctx->stats.sess_accept); + ssl_tsan_decr(s->session_ctx, &s->session_ctx->stats.sess_accept); } /* diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index f4e2c15600..4ecfc0b546 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1569,7 +1569,7 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) * overwritten if the server refuses resumption. */ if (s->session->session_id_length > 0) { - tsan_counter(&s->session_ctx->stats.sess_miss); + ssl_tsan_counter(s->session_ctx, &s->session_ctx->stats.sess_miss); if (!ssl_get_new_session(s, 0)) { /* SSLfatal() already called */ goto err; diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 79ac9be04b..6fcf561b14 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -175,18 +175,19 @@ int tls_setup_handshake(SSL *s) } if (SSL_IS_FIRST_HANDSHAKE(s)) { /* N.B. s->session_ctx == s->ctx here */ - tsan_counter(&s->session_ctx->stats.sess_accept); + ssl_tsan_counter(s->session_ctx, &s->session_ctx->stats.sess_accept); } else { /* N.B. s->ctx may not equal s->session_ctx */ - tsan_counter(&s->ctx->stats.sess_accept_renegotiate); + ssl_tsan_counter(s->ctx, &s->ctx->stats.sess_accept_renegotiate); s->s3.tmp.cert_request = 0; } } else { if (SSL_IS_FIRST_HANDSHAKE(s)) - tsan_counter(&s->session_ctx->stats.sess_connect); + ssl_tsan_counter(s->session_ctx, &s->session_ctx->stats.sess_connect); else - tsan_counter(&s->session_ctx->stats.sess_connect_renegotiate); + ssl_tsan_counter(s->session_ctx, + &s->session_ctx->stats.sess_connect_renegotiate); /* mark client_random uninitialized */ memset(s->s3.client_random, 0, sizeof(s->s3.client_random)); @@ -1096,7 +1097,7 @@ WORK_STATE tls_finish_handshake(SSL *s, ossl_unused WORK_STATE wst, ssl_update_cache(s, SSL_SESS_CACHE_SERVER); /* N.B. s->ctx may not equal s->session_ctx */ - tsan_counter(&s->ctx->stats.sess_accept_good); + ssl_tsan_counter(s->ctx, &s->ctx->stats.sess_accept_good); s->handshake_func = ossl_statem_accept; } else { if (SSL_IS_TLS13(s)) { @@ -1115,10 +1116,12 @@ WORK_STATE tls_finish_handshake(SSL *s, ossl_unused WORK_STATE wst, ssl_update_cache(s, SSL_SESS_CACHE_CLIENT); } if (s->hit) - tsan_counter(&s->session_ctx->stats.sess_hit); + ssl_tsan_counter(s->session_ctx, + &s->session_ctx->stats.sess_hit); s->handshake_func = ossl_statem_connect; - tsan_counter(&s->session_ctx->stats.sess_connect_good); + ssl_tsan_counter(s->session_ctx, + &s->session_ctx->stats.sess_connect_good); } if (SSL_IS_DTLS(s)) { -- cgit v1.2.3