From 683d7bd20a96a34d85341cd04b4c6309b0730852 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Mon, 13 Apr 2015 11:04:14 -0700 Subject: [PATCH] Convert BN_MONT_CTX to new-style locking. This introduces a per-RSA/DSA/DH lock. This is good for lock contention, although pthread locks are depressingly bloated. Change-Id: I07c4d1606fc35135fc141ebe6ba904a28c8f8a0c Reviewed-on: https://boringssl-review.googlesource.com/4324 Reviewed-by: Adam Langley --- crypto/bn/montgomery.c | 55 ++++++++++++++++++------------------------ crypto/dh/dh.c | 4 +++ crypto/dh/dh_impl.c | 9 ++++--- crypto/dsa/dsa.c | 4 +++ crypto/dsa/dsa_impl.c | 11 ++++++--- crypto/rsa/blinding.c | 8 +++--- crypto/rsa/rsa.c | 3 +++ crypto/rsa/rsa_impl.c | 42 +++++++++++++++++--------------- include/openssl/bn.h | 13 +++++----- include/openssl/dh.h | 3 +++ include/openssl/dsa.h | 2 ++ include/openssl/rsa.h | 12 ++++++--- 12 files changed, 94 insertions(+), 72 deletions(-) diff --git a/crypto/bn/montgomery.c b/crypto/bn/montgomery.c index 5a9d686e..152cf2d8 100644 --- a/crypto/bn/montgomery.c +++ b/crypto/bn/montgomery.c @@ -114,6 +114,7 @@ #include #include "internal.h" +#include "../internal.h" #if !defined(OPENSSL_NO_ASM) && \ @@ -292,44 +293,36 @@ err: return ret; } -BN_MONT_CTX *BN_MONT_CTX_set_locked(BN_MONT_CTX **pmont, int lock, - const BIGNUM *mod, BN_CTX *ctx) { - BN_MONT_CTX *ret; +BN_MONT_CTX *BN_MONT_CTX_set_locked(BN_MONT_CTX **pmont, CRYPTO_MUTEX *lock, + const BIGNUM *mod, BN_CTX *bn_ctx) { + CRYPTO_MUTEX_lock_read(lock); + BN_MONT_CTX *ctx = *pmont; + CRYPTO_MUTEX_unlock(lock); - CRYPTO_r_lock(lock); - ret = *pmont; - CRYPTO_r_unlock(lock); - if (ret) { - return ret; + if (ctx) { + return ctx; } - /* We don't want to serialise globally while doing our lazy-init math in - * BN_MONT_CTX_set. That punishes threads that are doing independent - * things. Instead, punish the case where more than one thread tries to - * lazy-init the same 'pmont', by having each do the lazy-init math work - * independently and only use the one from the thread that wins the race - * (the losers throw away the work they've done). */ - ret = BN_MONT_CTX_new(); - if (!ret) { - return NULL; - } - if (!BN_MONT_CTX_set(ret, mod, ctx)) { - BN_MONT_CTX_free(ret); - return NULL; + CRYPTO_MUTEX_lock_write(lock); + ctx = *pmont; + if (ctx) { + goto out; } - /* The locked compare-and-set, after the local work is done. */ - CRYPTO_w_lock(lock); - if (*pmont) { - BN_MONT_CTX_free(ret); - ret = *pmont; - } else { - *pmont = ret; + ctx = BN_MONT_CTX_new(); + if (ctx == NULL) { + goto out; } + if (!BN_MONT_CTX_set(ctx, mod, bn_ctx)) { + BN_MONT_CTX_free(ctx); + ctx = NULL; + goto out; + } + *pmont = ctx; - CRYPTO_w_unlock(lock); - - return ret; +out: + CRYPTO_MUTEX_unlock(lock); + return ctx; } int BN_to_montgomery(BIGNUM *ret, const BIGNUM *a, const BN_MONT_CTX *mont, diff --git a/crypto/dh/dh.c b/crypto/dh/dh.c index 7a50da7b..86804bf8 100644 --- a/crypto/dh/dh.c +++ b/crypto/dh/dh.c @@ -66,6 +66,7 @@ #include #include "internal.h" +#include "../internal.h" extern const DH_METHOD DH_default_method; @@ -90,6 +91,8 @@ DH *DH_new_method(const ENGINE *engine) { } METHOD_ref(dh->meth); + CRYPTO_MUTEX_init(&dh->method_mont_p_lock); + dh->references = 1; if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_DH, dh, &dh->ex_data)) { OPENSSL_free(dh); @@ -131,6 +134,7 @@ void DH_free(DH *dh) { if (dh->counter != NULL) BN_clear_free(dh->counter); if (dh->pub_key != NULL) BN_clear_free(dh->pub_key); if (dh->priv_key != NULL) BN_clear_free(dh->priv_key); + CRYPTO_MUTEX_cleanup(&dh->method_mont_p_lock); OPENSSL_free(dh); } diff --git a/crypto/dh/dh_impl.c b/crypto/dh/dh_impl.c index 5c4d6372..81d777d0 100644 --- a/crypto/dh/dh_impl.c +++ b/crypto/dh/dh_impl.c @@ -62,6 +62,7 @@ #include "internal.h" + #define OPENSSL_DH_MAX_MODULUS_BITS 10000 static int generate_parameters(DH *ret, int prime_bits, int generator, BN_GENCB *cb) { @@ -207,8 +208,8 @@ static int generate_key(DH *dh) { pub_key = dh->pub_key; } - mont = - BN_MONT_CTX_set_locked(&dh->method_mont_p, CRYPTO_LOCK_DH, dh->p, ctx); + mont = BN_MONT_CTX_set_locked(&dh->method_mont_p, &dh->method_mont_p_lock, + dh->p, ctx); if (!mont) { goto err; } @@ -282,8 +283,8 @@ static int compute_key(DH *dh, unsigned char *out, const BIGNUM *pub_key) { goto err; } - mont = - BN_MONT_CTX_set_locked(&dh->method_mont_p, CRYPTO_LOCK_DH, dh->p, ctx); + mont = BN_MONT_CTX_set_locked(&dh->method_mont_p, &dh->method_mont_p_lock, + dh->p, ctx); if (!mont) { goto err; } diff --git a/crypto/dsa/dsa.c b/crypto/dsa/dsa.c index 5303714c..c5809562 100644 --- a/crypto/dsa/dsa.c +++ b/crypto/dsa/dsa.c @@ -70,6 +70,7 @@ #include #include "internal.h" +#include "../internal.h" extern const DSA_METHOD DSA_default_method; @@ -97,6 +98,8 @@ DSA *DSA_new_method(const ENGINE *engine) { dsa->write_params = 1; dsa->references = 1; + CRYPTO_MUTEX_init(&dsa->method_mont_p_lock); + if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_DSA, dsa, &dsa->ex_data)) { METHOD_unref(dsa->meth); OPENSSL_free(dsa); @@ -150,6 +153,7 @@ void DSA_free(DSA *dsa) { if (dsa->r != NULL) { BN_clear_free(dsa->r); } + CRYPTO_MUTEX_cleanup(&dsa->method_mont_p_lock); OPENSSL_free(dsa); } diff --git a/crypto/dsa/dsa_impl.c b/crypto/dsa/dsa_impl.c index aba7f854..c4df80bd 100644 --- a/crypto/dsa/dsa_impl.c +++ b/crypto/dsa/dsa_impl.c @@ -123,8 +123,9 @@ static int sign_setup(const DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp, BN_set_flags(&k, BN_FLG_CONSTTIME); - if (!BN_MONT_CTX_set_locked((BN_MONT_CTX **)&dsa->method_mont_p, - CRYPTO_LOCK_DSA, dsa->p, ctx)) { + if (BN_MONT_CTX_set_locked((BN_MONT_CTX **)&dsa->method_mont_p, + (CRYPTO_MUTEX *)&dsa->method_mont_p_lock, dsa->p, + ctx) == NULL) { goto err; } @@ -365,12 +366,14 @@ static int verify(int *out_valid, const uint8_t *dgst, size_t digest_len, } mont = BN_MONT_CTX_set_locked((BN_MONT_CTX **)&dsa->method_mont_p, - CRYPTO_LOCK_DSA, dsa->p, ctx); + (CRYPTO_MUTEX *)&dsa->method_mont_p_lock, + dsa->p, ctx); if (!mont) { goto err; } - if (!BN_mod_exp2_mont(&t1, dsa->g, &u1, dsa->pub_key, &u2, dsa->p, ctx, mont)) { + if (!BN_mod_exp2_mont(&t1, dsa->g, &u1, dsa->pub_key, &u2, dsa->p, ctx, + mont)) { goto err; } diff --git a/crypto/rsa/blinding.c b/crypto/rsa/blinding.c index c5a1604b..88682ef4 100644 --- a/crypto/rsa/blinding.c +++ b/crypto/rsa/blinding.c @@ -414,6 +414,7 @@ BN_BLINDING *rsa_setup_blinding(RSA *rsa, BN_CTX *in_ctx) { BIGNUM *e, *n; BN_CTX *ctx; BN_BLINDING *ret = NULL; + BN_MONT_CTX *mont_ctx = NULL; if (in_ctx == NULL) { ctx = BN_CTX_new(); @@ -445,14 +446,15 @@ BN_BLINDING *rsa_setup_blinding(RSA *rsa, BN_CTX *in_ctx) { BN_with_flags(n, rsa->n, BN_FLG_CONSTTIME); if (rsa->flags & RSA_FLAG_CACHE_PUBLIC) { - if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, CRYPTO_LOCK_RSA, rsa->n, - ctx)) { + mont_ctx = + BN_MONT_CTX_set_locked(&rsa->_method_mod_n, &rsa->lock, rsa->n, ctx); + if (mont_ctx == NULL) { goto err; } } ret = BN_BLINDING_create_param(NULL, e, n, ctx, rsa->meth->bn_mod_exp, - rsa->_method_mod_n); + mont_ctx); if (ret == NULL) { OPENSSL_PUT_ERROR(RSA, rsa_setup_blinding, ERR_R_BN_LIB); goto err; diff --git a/crypto/rsa/rsa.c b/crypto/rsa/rsa.c index 884f67e4..88d38a23 100644 --- a/crypto/rsa/rsa.c +++ b/crypto/rsa/rsa.c @@ -67,6 +67,7 @@ #include #include "internal.h" +#include "../internal.h" extern const RSA_METHOD RSA_default_method; @@ -93,6 +94,7 @@ RSA *RSA_new_method(const ENGINE *engine) { rsa->references = 1; rsa->flags = rsa->meth->flags; + CRYPTO_MUTEX_init(&rsa->lock); if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_RSA, rsa, &rsa->ex_data)) { METHOD_unref(rsa->meth); @@ -161,6 +163,7 @@ void RSA_free(RSA *rsa) { if (rsa->blindings_inuse != NULL) { OPENSSL_free(rsa->blindings_inuse); } + CRYPTO_MUTEX_cleanup(&rsa->lock); OPENSSL_free(rsa); } diff --git a/crypto/rsa/rsa_impl.c b/crypto/rsa/rsa_impl.c index f790e641..e8cbd97c 100644 --- a/crypto/rsa/rsa_impl.c +++ b/crypto/rsa/rsa_impl.c @@ -64,6 +64,7 @@ #include #include "internal.h" +#include "../internal.h" #define OPENSSL_RSA_MAX_MODULUS_BITS 16384 @@ -166,13 +167,14 @@ static int encrypt(RSA *rsa, size_t *out_len, uint8_t *out, size_t max_out, } if (rsa->flags & RSA_FLAG_CACHE_PUBLIC) { - if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, CRYPTO_LOCK_RSA, rsa->n, - ctx)) { + if (BN_MONT_CTX_set_locked(&rsa->_method_mod_n, &rsa->lock, rsa->n, ctx) == + NULL) { goto err; } } - if (!rsa->meth->bn_mod_exp(result, f, rsa->e, rsa->n, ctx, rsa->_method_mod_n)) { + if (!rsa->meth->bn_mod_exp(result, f, rsa->e, rsa->n, ctx, + rsa->_method_mod_n)) { goto err; } @@ -218,7 +220,7 @@ static BN_BLINDING *rsa_blinding_get(RSA *rsa, unsigned *index_used, uint8_t *new_blindings_inuse; char overflow = 0; - CRYPTO_w_lock(CRYPTO_LOCK_RSA_BLINDING); + CRYPTO_MUTEX_lock_write(&rsa->lock); unsigned i; for (i = 0; i < rsa->num_blindings; i++) { @@ -231,7 +233,7 @@ static BN_BLINDING *rsa_blinding_get(RSA *rsa, unsigned *index_used, } if (ret != NULL) { - CRYPTO_w_unlock(CRYPTO_LOCK_RSA_BLINDING); + CRYPTO_MUTEX_unlock(&rsa->lock); return ret; } @@ -240,7 +242,7 @@ static BN_BLINDING *rsa_blinding_get(RSA *rsa, unsigned *index_used, /* We didn't find a free BN_BLINDING to use so increase the length of * the arrays by one and use the newly created element. */ - CRYPTO_w_unlock(CRYPTO_LOCK_RSA_BLINDING); + CRYPTO_MUTEX_unlock(&rsa->lock); ret = rsa_setup_blinding(rsa, ctx); if (ret == NULL) { return NULL; @@ -253,7 +255,7 @@ static BN_BLINDING *rsa_blinding_get(RSA *rsa, unsigned *index_used, return ret; } - CRYPTO_w_lock(CRYPTO_LOCK_RSA_BLINDING); + CRYPTO_MUTEX_lock_write(&rsa->lock); new_blindings = OPENSSL_malloc(sizeof(BN_BLINDING *) * (rsa->num_blindings + 1)); @@ -282,14 +284,14 @@ static BN_BLINDING *rsa_blinding_get(RSA *rsa, unsigned *index_used, rsa->blindings_inuse = new_blindings_inuse; rsa->num_blindings++; - CRYPTO_w_unlock(CRYPTO_LOCK_RSA_BLINDING); + CRYPTO_MUTEX_unlock(&rsa->lock); return ret; err2: OPENSSL_free(new_blindings); err1: - CRYPTO_w_unlock(CRYPTO_LOCK_RSA_BLINDING); + CRYPTO_MUTEX_unlock(&rsa->lock); BN_BLINDING_free(ret); return NULL; } @@ -304,9 +306,9 @@ static void rsa_blinding_release(RSA *rsa, BN_BLINDING *blinding, return; } - CRYPTO_w_lock(CRYPTO_LOCK_RSA_BLINDING); + CRYPTO_MUTEX_lock_write(&rsa->lock); rsa->blindings_inuse[blinding_index] = 0; - CRYPTO_w_unlock(CRYPTO_LOCK_RSA_BLINDING); + CRYPTO_MUTEX_unlock(&rsa->lock); } /* signing */ @@ -479,8 +481,8 @@ static int verify_raw(RSA *rsa, size_t *out_len, uint8_t *out, size_t max_out, } if (rsa->flags & RSA_FLAG_CACHE_PUBLIC) { - if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, CRYPTO_LOCK_RSA, rsa->n, - ctx)) { + if (BN_MONT_CTX_set_locked(&rsa->_method_mod_n, &rsa->lock, rsa->n, ctx) == + NULL) { goto err; } } @@ -583,8 +585,8 @@ static int private_transform(RSA *rsa, uint8_t *out, const uint8_t *in, BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME); if (rsa->flags & RSA_FLAG_CACHE_PUBLIC) { - if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, CRYPTO_LOCK_RSA, rsa->n, - ctx)) { + if (BN_MONT_CTX_set_locked(&rsa->_method_mod_n, &rsa->lock, rsa->n, + ctx) == NULL) { goto err; } } @@ -645,18 +647,20 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) { BN_with_flags(q, rsa->q, BN_FLG_CONSTTIME); if (rsa->flags & RSA_FLAG_CACHE_PRIVATE) { - if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_p, CRYPTO_LOCK_RSA, p, ctx)) { + if (BN_MONT_CTX_set_locked(&rsa->_method_mod_p, &rsa->lock, p, ctx) == + NULL) { goto err; } - if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_q, CRYPTO_LOCK_RSA, q, ctx)) { + if (BN_MONT_CTX_set_locked(&rsa->_method_mod_q, &rsa->lock, q, ctx) == + NULL) { goto err; } } } if (rsa->flags & RSA_FLAG_CACHE_PUBLIC) { - if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, CRYPTO_LOCK_RSA, rsa->n, - ctx)) { + if (BN_MONT_CTX_set_locked(&rsa->_method_mod_n, &rsa->lock, rsa->n, ctx) == + NULL) { goto err; } } diff --git a/include/openssl/bn.h b/include/openssl/bn.h index 838870db..917beaf0 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -124,6 +124,7 @@ #define OPENSSL_HEADER_BN_H #include +#include #include /* for FILE* */ @@ -711,15 +712,13 @@ OPENSSL_EXPORT BN_MONT_CTX *BN_MONT_CTX_copy(BN_MONT_CTX *to, OPENSSL_EXPORT int BN_MONT_CTX_set(BN_MONT_CTX *mont, const BIGNUM *mod, BN_CTX *ctx); -/* BN_MONT_CTX_set_locked takes the lock indicated by |lock| and checks whether - * |*pmont| is NULL. If so, it creates a new |BN_MONT_CTX| and sets the modulus - * for it to |mod|. It then stores it as |*pmont| and returns it, or NULL on - * error. +/* BN_MONT_CTX_set_locked takes |lock| and checks whether |*pmont| is NULL. If + * so, it creates a new |BN_MONT_CTX| and sets the modulus for it to |mod|. It + * then stores it as |*pmont| and returns it, or NULL on error. * * If |*pmont| is already non-NULL then the existing value is returned. */ -OPENSSL_EXPORT BN_MONT_CTX *BN_MONT_CTX_set_locked(BN_MONT_CTX **pmont, - int lock, const BIGNUM *mod, - BN_CTX *ctx); +BN_MONT_CTX *BN_MONT_CTX_set_locked(BN_MONT_CTX **pmont, CRYPTO_MUTEX *lock, + const BIGNUM *mod, BN_CTX *bn_ctx); /* BN_to_montgomery sets |ret| equal to |a| in the Montgomery domain. It * returns one on success and zero on error. */ diff --git a/include/openssl/dh.h b/include/openssl/dh.h index 3c8f2900..39614ffa 100644 --- a/include/openssl/dh.h +++ b/include/openssl/dh.h @@ -61,6 +61,7 @@ #include #include +#include #if defined(__cplusplus) extern "C" { @@ -236,6 +237,8 @@ struct dh_st { /* priv_length contains the length, in bits, of the private value. If zero, * the private value will be the same length as |p|. */ unsigned priv_length; + + CRYPTO_MUTEX method_mont_p_lock; BN_MONT_CTX *method_mont_p; /* Place holders if we want to do X9.42 DH */ diff --git a/include/openssl/dsa.h b/include/openssl/dsa.h index 69dd56b6..47270f86 100644 --- a/include/openssl/dsa.h +++ b/include/openssl/dsa.h @@ -64,6 +64,7 @@ #include #include +#include #if defined(__cplusplus) extern "C" { @@ -351,6 +352,7 @@ struct dsa_st { int flags; /* Normally used to cache montgomery values */ + CRYPTO_MUTEX method_mont_p_lock; BN_MONT_CTX *method_mont_p; int references; CRYPTO_EX_DATA ex_data; diff --git a/include/openssl/rsa.h b/include/openssl/rsa.h index f49eb143..889ad192 100644 --- a/include/openssl/rsa.h +++ b/include/openssl/rsa.h @@ -61,6 +61,7 @@ #include #include +#include #if defined(__cplusplus) extern "C" { @@ -471,18 +472,21 @@ struct rsa_st { int references; int flags; - /* Used to cache montgomery values */ + CRYPTO_MUTEX lock; + + /* Used to cache montgomery values. The creation of these values is protected + * by |lock|. */ BN_MONT_CTX *_method_mod_n; BN_MONT_CTX *_method_mod_p; BN_MONT_CTX *_method_mod_q; /* num_blindings contains the size of the |blindings| and |blindings_inuse| * arrays. This member and the |blindings_inuse| array are protected by - * CRYPTO_LOCK_RSA_BLINDING. */ + * |lock|. */ unsigned num_blindings; /* blindings is an array of BN_BLINDING structures that can be reserved by a - * thread by locking CRYPTO_LOCK_RSA_BLINDING and changing the corresponding - * element in |blindings_inuse| from 0 to 1. */ + * thread by locking |lock| and changing the corresponding element in + * |blindings_inuse| from 0 to 1. */ BN_BLINDING **blindings; unsigned char *blindings_inuse; };