Make return value of |BN_MONT_CTX_set_locked| int.
This reduces the chance of double-frees.
BUG=10
Change-Id: I11a240e2ea5572effeddc05acb94db08c54a2e0b
Reviewed-on: https://boringssl-review.googlesource.com/7583
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/crypto/bn/montgomery.c b/crypto/bn/montgomery.c
index 7e4a76f..f56998e 100644
--- a/crypto/bn/montgomery.c
+++ b/crypto/bn/montgomery.c
@@ -289,14 +289,14 @@
return ret;
}
-BN_MONT_CTX *BN_MONT_CTX_set_locked(BN_MONT_CTX **pmont, CRYPTO_MUTEX *lock,
- const BIGNUM *mod, BN_CTX *bn_ctx) {
+int 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);
if (ctx) {
- return ctx;
+ return 1;
}
CRYPTO_MUTEX_lock_write(lock);
@@ -318,7 +318,7 @@
out:
CRYPTO_MUTEX_unlock(lock);
- return ctx;
+ return ctx != NULL;
}
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 aed8720..a5cf94d 100644
--- a/crypto/dh/dh.c
+++ b/crypto/dh/dh.c
@@ -236,7 +236,6 @@
int generate_new_key = 0;
unsigned l;
BN_CTX *ctx = NULL;
- BN_MONT_CTX *mont = NULL;
BIGNUM *pub_key = NULL, *priv_key = NULL;
BIGNUM local_priv;
@@ -269,9 +268,8 @@
pub_key = dh->pub_key;
}
- mont = BN_MONT_CTX_set_locked(&dh->method_mont_p, &dh->method_mont_p_lock,
- dh->p, ctx);
- if (!mont) {
+ if (!BN_MONT_CTX_set_locked(&dh->method_mont_p, &dh->method_mont_p_lock,
+ dh->p, ctx)) {
goto err;
}
@@ -293,7 +291,8 @@
}
BN_with_flags(&local_priv, priv_key, BN_FLG_CONSTTIME);
- if (!BN_mod_exp_mont(pub_key, dh->g, &local_priv, dh->p, ctx, mont)) {
+ if (!BN_mod_exp_mont(pub_key, dh->g, &local_priv, dh->p, ctx,
+ dh->method_mont_p)) {
goto err;
}
@@ -318,7 +317,6 @@
int DH_compute_key(unsigned char *out, const BIGNUM *peers_key, DH *dh) {
BN_CTX *ctx = NULL;
- BN_MONT_CTX *mont = NULL;
BIGNUM *shared_key;
int ret = -1;
int check_result;
@@ -344,9 +342,8 @@
goto err;
}
- mont = BN_MONT_CTX_set_locked(&dh->method_mont_p, &dh->method_mont_p_lock,
- dh->p, ctx);
- if (!mont) {
+ if (!BN_MONT_CTX_set_locked(&dh->method_mont_p, &dh->method_mont_p_lock,
+ dh->p, ctx)) {
goto err;
}
@@ -357,7 +354,7 @@
BN_with_flags(&local_priv, dh->priv_key, BN_FLG_CONSTTIME);
if (!BN_mod_exp_mont(shared_key, peers_key, &local_priv, dh->p, ctx,
- mont)) {
+ dh->method_mont_p)) {
OPENSSL_PUT_ERROR(DH, ERR_R_BN_LIB);
goto err;
}
diff --git a/crypto/dsa/dsa.c b/crypto/dsa/dsa.c
index 979079d..fe29aa0 100644
--- a/crypto/dsa/dsa.c
+++ b/crypto/dsa/dsa.c
@@ -591,7 +591,6 @@
size_t digest_len, DSA_SIG *sig, const DSA *dsa) {
BN_CTX *ctx;
BIGNUM u1, u2, t1;
- BN_MONT_CTX *mont = NULL;
int ret = 0;
unsigned i;
@@ -662,15 +661,14 @@
goto err;
}
- mont = BN_MONT_CTX_set_locked((BN_MONT_CTX **)&dsa->method_mont_p,
- (CRYPTO_MUTEX *)&dsa->method_mont_p_lock,
- dsa->p, ctx);
- if (!mont) {
+ if (!BN_MONT_CTX_set_locked((BN_MONT_CTX **)&dsa->method_mont_p,
+ (CRYPTO_MUTEX *)&dsa->method_mont_p_lock, dsa->p,
+ ctx)) {
goto err;
}
if (!BN_mod_exp2_mont(&t1, dsa->g, &u1, dsa->pub_key, &u2, dsa->p, ctx,
- mont)) {
+ dsa->method_mont_p)) {
goto err;
}
@@ -823,9 +821,9 @@
BN_set_flags(&k, BN_FLG_CONSTTIME);
- if (BN_MONT_CTX_set_locked((BN_MONT_CTX **)&dsa->method_mont_p,
- (CRYPTO_MUTEX *)&dsa->method_mont_p_lock, dsa->p,
- ctx) == NULL) {
+ if (!BN_MONT_CTX_set_locked((BN_MONT_CTX **)&dsa->method_mont_p,
+ (CRYPTO_MUTEX *)&dsa->method_mont_p_lock, dsa->p,
+ ctx)) {
goto err;
}
diff --git a/crypto/rsa/rsa_impl.c b/crypto/rsa/rsa_impl.c
index bb8789f..ad8e3f9 100644
--- a/crypto/rsa/rsa_impl.c
+++ b/crypto/rsa/rsa_impl.c
@@ -171,7 +171,7 @@
goto err;
}
- if (BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx) == NULL ||
+ if (!BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx) ||
!BN_mod_exp_mont(result, f, rsa->e, rsa->n, ctx, rsa->mont_n)) {
goto err;
}
@@ -487,7 +487,7 @@
goto err;
}
- if (BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx) == NULL ||
+ if (!BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx) ||
!BN_mod_exp_mont(result, f, rsa->e, rsa->n, ctx, rsa->mont_n)) {
goto err;
}
@@ -557,7 +557,7 @@
}
if (!(rsa->flags & RSA_FLAG_NO_BLINDING)) {
- if (BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx) == NULL) {
+ if (!BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx)) {
OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR);
goto err;
}
@@ -585,7 +585,7 @@
d = &local_d;
BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME);
- if (BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx) == NULL ||
+ if (!BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx) ||
!BN_mod_exp_mont_consttime(result, f, d, rsa->n, ctx, rsa->mont_n)) {
goto err;
}
@@ -662,13 +662,13 @@
q = &local_q;
BN_with_flags(q, rsa->q, BN_FLG_CONSTTIME);
- if (BN_MONT_CTX_set_locked(&rsa->mont_p, &rsa->lock, p, ctx) == NULL ||
- BN_MONT_CTX_set_locked(&rsa->mont_q, &rsa->lock, q, ctx) == NULL) {
+ if (!BN_MONT_CTX_set_locked(&rsa->mont_p, &rsa->lock, p, ctx) ||
+ !BN_MONT_CTX_set_locked(&rsa->mont_q, &rsa->lock, q, ctx)) {
goto err;
}
}
- if (BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx) == NULL) {
+ if (!BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx)) {
goto err;
}
@@ -756,7 +756,7 @@
goto err;
}
- if (BN_MONT_CTX_set_locked(&ap->mont, &rsa->lock, prime, ctx) == NULL ||
+ if (!BN_MONT_CTX_set_locked(&ap->mont, &rsa->lock, prime, ctx) ||
!BN_mod_exp_mont_consttime(m1, r1, exp, prime, ctx, ap->mont)) {
goto err;
}
diff --git a/include/openssl/bn.h b/include/openssl/bn.h
index 64ec5c9..2f61001 100644
--- a/include/openssl/bn.h
+++ b/include/openssl/bn.h
@@ -749,11 +749,11 @@
/* 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.
+ * then stores it as |*pmont|. It returns one on success and zero on error.
*
- * If |*pmont| is already non-NULL then the existing value is returned. */
-BN_MONT_CTX *BN_MONT_CTX_set_locked(BN_MONT_CTX **pmont, CRYPTO_MUTEX *lock,
- const BIGNUM *mod, BN_CTX *bn_ctx);
+ * If |*pmont| is already non-NULL then it does nothing and returns one. */
+int 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. */
