| From 12d41a023efb01b846457ccdbbcbe2b65a87d530 Mon Sep 17 00:00:00 2001 |
| From: Eric Biggers <ebiggers@google.com> |
| Date: Sun, 5 Nov 2017 18:30:44 -0800 |
| Subject: crypto: dh - Fix double free of ctx->p |
| |
| From: Eric Biggers <ebiggers@google.com> |
| |
| commit 12d41a023efb01b846457ccdbbcbe2b65a87d530 upstream. |
| |
| 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() (renamed to dh_clear_ctx()) in the error |
| paths, as that correctly sets the pointers to NULL. |
| |
| KASAN report: |
| |
| MPI: mpi too large (32760 bits) |
| ================================================================== |
| BUG: KASAN: use-after-free in mpi_free+0x131/0x170 |
| Read of size 4 at addr ffff88006c7cdf90 by task reproduce_doubl/367 |
| |
| CPU: 1 PID: 367 Comm: reproduce_doubl Not tainted 4.14.0-rc7-00040-g05298abde6fe #7 |
| Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 |
| Call Trace: |
| dump_stack+0xb3/0x10b |
| ? mpi_free+0x131/0x170 |
| print_address_description+0x79/0x2a0 |
| ? mpi_free+0x131/0x170 |
| kasan_report+0x236/0x340 |
| ? akcipher_register_instance+0x90/0x90 |
| __asan_report_load4_noabort+0x14/0x20 |
| mpi_free+0x131/0x170 |
| ? akcipher_register_instance+0x90/0x90 |
| dh_exit_tfm+0x3d/0x140 |
| crypto_kpp_exit_tfm+0x52/0x70 |
| crypto_destroy_tfm+0xb3/0x250 |
| __keyctl_dh_compute+0x640/0xe90 |
| ? kasan_slab_free+0x12f/0x180 |
| ? dh_data_from_key+0x240/0x240 |
| ? key_create_or_update+0x1ee/0xb20 |
| ? key_instantiate_and_link+0x440/0x440 |
| ? lock_contended+0xee0/0xee0 |
| ? kfree+0xcf/0x210 |
| ? SyS_add_key+0x268/0x340 |
| keyctl_dh_compute+0xb3/0xf1 |
| ? __keyctl_dh_compute+0xe90/0xe90 |
| ? SyS_add_key+0x26d/0x340 |
| ? entry_SYSCALL_64_fastpath+0x5/0xbe |
| ? trace_hardirqs_on_caller+0x3f4/0x560 |
| SyS_keyctl+0x72/0x2c0 |
| entry_SYSCALL_64_fastpath+0x1f/0xbe |
| RIP: 0033:0x43ccf9 |
| RSP: 002b:00007ffeeec96158 EFLAGS: 00000246 ORIG_RAX: 00000000000000fa |
| RAX: ffffffffffffffda RBX: 000000000248b9b9 RCX: 000000000043ccf9 |
| RDX: 00007ffeeec96170 RSI: 00007ffeeec96160 RDI: 0000000000000017 |
| RBP: 0000000000000046 R08: 0000000000000000 R09: 0248b9b9143dc936 |
| R10: 0000000000001000 R11: 0000000000000246 R12: 0000000000000000 |
| R13: 0000000000409670 R14: 0000000000409700 R15: 0000000000000000 |
| |
| Allocated by task 367: |
| save_stack_trace+0x16/0x20 |
| kasan_kmalloc+0xeb/0x180 |
| kmem_cache_alloc_trace+0x114/0x300 |
| mpi_alloc+0x4b/0x230 |
| mpi_read_raw_data+0xbe/0x360 |
| dh_set_secret+0x1dc/0x460 |
| __keyctl_dh_compute+0x623/0xe90 |
| keyctl_dh_compute+0xb3/0xf1 |
| SyS_keyctl+0x72/0x2c0 |
| entry_SYSCALL_64_fastpath+0x1f/0xbe |
| |
| Freed by task 367: |
| save_stack_trace+0x16/0x20 |
| kasan_slab_free+0xab/0x180 |
| kfree+0xb5/0x210 |
| mpi_free+0xcb/0x170 |
| dh_set_secret+0x2d7/0x460 |
| __keyctl_dh_compute+0x623/0xe90 |
| keyctl_dh_compute+0xb3/0xf1 |
| SyS_keyctl+0x72/0x2c0 |
| entry_SYSCALL_64_fastpath+0x1f/0xbe |
| |
| Fixes: 802c7f1c84e4 ("crypto: dh - Add DH software implementation") |
| Signed-off-by: Eric Biggers <ebiggers@google.com> |
| Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> |
| Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| crypto/dh.c | 33 +++++++++++++-------------------- |
| 1 file changed, 13 insertions(+), 20 deletions(-) |
| |
| --- a/crypto/dh.c |
| +++ b/crypto/dh.c |
| @@ -21,19 +21,12 @@ struct dh_ctx { |
| MPI xa; |
| }; |
| |
| -static inline void dh_clear_params(struct dh_ctx *ctx) |
| +static void dh_clear_ctx(struct dh_ctx *ctx) |
| { |
| mpi_free(ctx->p); |
| mpi_free(ctx->g); |
| - ctx->p = NULL; |
| - ctx->g = NULL; |
| -} |
| - |
| -static void dh_free_ctx(struct dh_ctx *ctx) |
| -{ |
| - dh_clear_params(ctx); |
| mpi_free(ctx->xa); |
| - ctx->xa = NULL; |
| + memset(ctx, 0, sizeof(*ctx)); |
| } |
| |
| /* |
| @@ -71,10 +64,8 @@ static int dh_set_params(struct dh_ctx * |
| 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; |
| } |
| @@ -86,21 +77,23 @@ static int dh_set_secret(struct crypto_k |
| struct dh params; |
| |
| /* Free the old MPI key if any */ |
| - dh_free_ctx(ctx); |
| + dh_clear_ctx(ctx); |
| |
| if (crypto_dh_decode_key(buf, len, ¶ms) < 0) |
| - return -EINVAL; |
| + goto err_clear_ctx; |
| |
| if (dh_set_params(ctx, ¶ms) < 0) |
| - return -EINVAL; |
| + goto err_clear_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_clear_ctx; |
| |
| return 0; |
| + |
| +err_clear_ctx: |
| + dh_clear_ctx(ctx); |
| + return -EINVAL; |
| } |
| |
| static int dh_compute_value(struct kpp_request *req) |
| @@ -158,7 +151,7 @@ static void dh_exit_tfm(struct crypto_kp |
| { |
| struct dh_ctx *ctx = dh_get_ctx(tfm); |
| |
| - dh_free_ctx(ctx); |
| + dh_clear_ctx(ctx); |
| } |
| |
| static struct kpp_alg dh = { |