All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Biggers <ebiggers3@gmail.com>
To: linux-crypto@vger.kernel.org, Herbert Xu <herbert@gondor.apana.org.au>
Cc: keyrings@vger.kernel.org,
	Tudor-Dan Ambarus <tudor.ambarus@microchip.com>,
	Mat Martineau <mathew.j.martineau@linux.intel.com>,
	Salvatore Benedetto <salvatore.benedetto@intel.com>,
	Stephan Mueller <smueller@chronox.de>,
	Eric Biggers <ebiggers@google.com>,
	stable@vger.kernel.org
Subject: [PATCH 1/4] crypto: dh - fix double free of ctx->p
Date: Wed, 01 Nov 2017 22:25:14 +0000	[thread overview]
Message-ID: <20171101222517.41602-2-ebiggers3@gmail.com> (raw)
In-Reply-To: <20171101222517.41602-1-ebiggers3@gmail.com>

From: Eric Biggers <ebiggers@google.com>

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.

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")
Cc: <stable@vger.kernel.org> # v4.8+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/dh.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

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, &params) < 0)
-		return -EINVAL;
+		goto err_free_ctx;
 
 	if (dh_set_params(ctx, &params) < 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)
-- 
2.15.0.403.gc27cc4dac6-goog


WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers3@gmail.com>
To: linux-crypto@vger.kernel.org, Herbert Xu <herbert@gondor.apana.org.au>
Cc: keyrings@vger.kernel.org,
	Tudor-Dan Ambarus <tudor.ambarus@microchip.com>,
	Mat Martineau <mathew.j.martineau@linux.intel.com>,
	Salvatore Benedetto <salvatore.benedetto@intel.com>,
	Stephan Mueller <smueller@chronox.de>,
	Eric Biggers <ebiggers@google.com>,
	stable@vger.kernel.org
Subject: [PATCH 1/4] crypto: dh - fix double free of ctx->p
Date: Wed,  1 Nov 2017 15:25:14 -0700	[thread overview]
Message-ID: <20171101222517.41602-2-ebiggers3@gmail.com> (raw)
In-Reply-To: <20171101222517.41602-1-ebiggers3@gmail.com>

From: Eric Biggers <ebiggers@google.com>

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.

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")
Cc: <stable@vger.kernel.org> # v4.8+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/dh.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

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, &params) < 0)
-		return -EINVAL;
+		goto err_free_ctx;
 
 	if (dh_set_params(ctx, &params) < 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)
-- 
2.15.0.403.gc27cc4dac6-goog

  reply	other threads:[~2017-11-01 22:25 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-01 22:25 [PATCH 0/4] crypto: dh - input validation fixes Eric Biggers
2017-11-01 22:25 ` Eric Biggers
2017-11-01 22:25 ` Eric Biggers [this message]
2017-11-01 22:25   ` [PATCH 1/4] crypto: dh - fix double free of ctx->p Eric Biggers
2017-11-02 10:55   ` Tudor Ambarus
2017-11-02 10:55     ` Tudor Ambarus
2017-11-02 17:30     ` Eric Biggers
2017-11-02 17:30       ` Eric Biggers
2017-11-01 22:25 ` [PATCH 2/4] crypto: dh - don't permit 'p' to be 0 Eric Biggers
2017-11-01 22:25   ` Eric Biggers
2017-11-02 11:40   ` Tudor Ambarus
2017-11-02 11:40     ` Tudor Ambarus
2017-11-02 17:31     ` Eric Biggers
2017-11-02 17:31       ` Eric Biggers
2017-11-03  6:23   ` Tudor Ambarus
2017-11-03  6:23     ` Tudor Ambarus
2017-11-01 22:25 ` [PATCH 3/4] crypto: qat - fix double free of ctx->p Eric Biggers
2017-11-01 22:25   ` Eric Biggers
2017-11-02 17:34   ` Eric Biggers
2017-11-02 17:34     ` Eric Biggers
2017-11-01 22:25 ` [PATCH 4/4] crypto: dh - don't permit 'key' or 'g' size longer than 'p' Eric Biggers
2017-11-01 22:25   ` Eric Biggers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171101222517.41602-2-ebiggers3@gmail.com \
    --to=ebiggers3@gmail.com \
    --cc=ebiggers@google.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=mathew.j.martineau@linux.intel.com \
    --cc=salvatore.benedetto@intel.com \
    --cc=smueller@chronox.de \
    --cc=stable@vger.kernel.org \
    --cc=tudor.ambarus@microchip.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.