| Message ID | 20171101222517.41602-2-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
Hi, Eric, On 11/02/2017 12:25 AM, Eric Biggers wrote: > When setting the secret with the software Diffie-Hellman implementation, > if allocating 'g' failed (e.g. if it was longer than > MAX_EXTERN_MPI_BITS), then 'p' was freed twice: once immediately, and > once later when the crypto_kpp tfm was destroyed. Fix it by using > dh_free_ctx() in the error paths, as that sets the pointers to NULL. Other solution would be to have just an one-line patch that sets p to NULL after freeing it. The benefit of just setting p to NULL and not using dh_free_ctx() is that you'll spare some cpu cycles. If g fails, g and a will already be NULL, so freeing them and set them to NULL is redundant. However, if you decide to always use dh_free_ctx(), you'll have to get rid of dh_clear_params(), because it will be used just in dh_free_ctx(). You can copy dh_clear_params() body to dh_free_ctx(). Cheers, ta
Hi Tudor, On Thu, Nov 02, 2017 at 12:55:56PM +0200, Tudor Ambarus wrote: > Hi, Eric, > > On 11/02/2017 12:25 AM, Eric Biggers wrote: > >When setting the secret with the software Diffie-Hellman implementation, > >if allocating 'g' failed (e.g. if it was longer than > >MAX_EXTERN_MPI_BITS), then 'p' was freed twice: once immediately, and > >once later when the crypto_kpp tfm was destroyed. Fix it by using > >dh_free_ctx() in the error paths, as that sets the pointers to NULL. > > Other solution would be to have just an one-line patch that sets p to > NULL after freeing it. The benefit of just setting p to NULL and not > using dh_free_ctx() is that you'll spare some cpu cycles. If g fails, > g and a will already be NULL, so freeing them and set them to NULL is > redundant. > > However, if you decide to always use dh_free_ctx(), you'll have to get > rid of dh_clear_params(), because it will be used just in dh_free_ctx(). > You can copy dh_clear_params() body to dh_free_ctx(). > This is on an error path, so a few cycles don't matter. I would much rather have the simpler code, with less chance to introduce exploitable bugs. Yes, I should inline dh_clear_params() into dh_free_ctx(). Eric
diff --git a/crypto/dh.c b/crypto/dh.c index b1032a5c1bfa..b488f1782ced 100644 --- a/crypto/dh.c +++ b/crypto/dh.c @@ -71,10 +71,8 @@ static int dh_set_params(struct dh_ctx *ctx, struct dh *params) return -EINVAL; ctx->g = mpi_read_raw_data(params->g, params->g_size); - if (!ctx->g) { - mpi_free(ctx->p); + if (!ctx->g) return -EINVAL; - } return 0; } @@ -89,18 +87,20 @@ static int dh_set_secret(struct crypto_kpp *tfm, const void *buf, dh_free_ctx(ctx); if (crypto_dh_decode_key(buf, len, ¶ms) < 0) - return -EINVAL; + goto err_free_ctx; if (dh_set_params(ctx, ¶ms) < 0) - return -EINVAL; + goto err_free_ctx; ctx->xa = mpi_read_raw_data(params.key, params.key_size); - if (!ctx->xa) { - dh_clear_params(ctx); - return -EINVAL; - } + if (!ctx->xa) + goto err_free_ctx; return 0; + +err_free_ctx: + dh_free_ctx(ctx); + return -EINVAL; } static int dh_compute_value(struct kpp_request *req)