All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] dm-crypt: Fix access beyond the end of allocated space
@ 2014-08-28 15:09 Mikulas Patocka
  2014-08-28 17:59 ` Milan Broz
  0 siblings, 1 reply; 4+ messages in thread
From: Mikulas Patocka @ 2014-08-28 15:09 UTC (permalink / raw)
  To: Alasdair G. Kergon, Mike Snitzer, Milan Broz, kkolasa; +Cc: dm-devel

dm-crypt has a bug that it accesses memory beyond allocated space.

To minimize allocation overhead, dm-crypt puts several structures into one
block allocated with kmalloc. The block holds struct ablkcipher_request,
cipher-specific scratch pad (crypto_ablkcipher_reqsize(any_tfm(cc))),
struct dm_crypt_request and initialization vector.

The variable dmreq_start is set to offset of struct dm_crypt_request
within this memory block. dm-crypt allocates block with this size:
cc->dmreq_start + sizeof(struct dm_crypt_request) + cc->iv_size.

When accessing the initialization vector, dm-crypt uses the function
iv_of_dmreq, which performs this calculation: ALIGN((unsigned long)(dmreq
+ 1), crypto_ablkcipher_alignmask(any_tfm(cc)) + 1).

dm-crypt allocated "cc->iv_size" bytes beyond the end of dm_crypt_request
structure. However, when dm-crypt accesses the initialization vector, it
takes a pointer to the end of dm_crypt_request, aligns it, and then uses
it as the initialization vector.

If the end of dm_crypt_request is not aligned on
crypto_ablkcipher_alignmask(any_tfm(cc)), the alignment causes
initialization vector to point beyond the allocated space. This bug is
very old (it dates back to commit 3a7f6c990ad04e6f576a159876c602d14d6f7fef
in 2.6.25). However, the bug was masked by the fact that kmalloc rounds up
the size to the next power of two. Recent change in dm-crypt that puts
this structure to per-bio data (298a9fa08a1577211d42a75e8fc073baef61e0d9)
made this bug show up, because there is no longer any padding beyond the
end of iv_size.

This patch fixes the bug by calculating the variable iv_size_padding and
adding it to the allocated size.

The patch also corrects alignment of dm_crypt_request. struct
dm_crypt_request is specific to dm-crypt (it isn't used by the crypto
subsystem at all), so it is aligned on __alignof__(struct
dm_crypt_request).

The patch also aligns per_bio_data_size on ARCH_KMALLOC_MINALIGN, so that
it is aligned as if the block was allocated with kmalloc.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-crypt.c |   29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

Index: linux-3.17-rc1/drivers/md/dm-crypt.c
===================================================================
--- linux-3.17-rc1.orig/drivers/md/dm-crypt.c	2014-08-27 18:38:55.622401228 +0200
+++ linux-3.17-rc1/drivers/md/dm-crypt.c	2014-08-28 01:12:15.971720819 +0200
@@ -1688,6 +1688,7 @@ static int crypt_ctr(struct dm_target *t
 	unsigned int key_size, opt_params;
 	unsigned long long tmpll;
 	int ret;
+	size_t iv_size_padding;
 	struct dm_arg_set as;
 	const char *opt_string;
 	char dummy;
@@ -1724,20 +1725,34 @@ static int crypt_ctr(struct dm_target *t
 
 	cc->dmreq_start = sizeof(struct ablkcipher_request);
 	cc->dmreq_start += crypto_ablkcipher_reqsize(any_tfm(cc));
-	cc->dmreq_start = ALIGN(cc->dmreq_start, crypto_tfm_ctx_alignment());
-	cc->dmreq_start += crypto_ablkcipher_alignmask(any_tfm(cc)) &
-			   ~(crypto_tfm_ctx_alignment() - 1);
+	cc->dmreq_start = ALIGN(cc->dmreq_start, __alignof__(struct dm_crypt_request));
+
+	if (crypto_ablkcipher_alignmask(any_tfm(cc)) < CRYPTO_MINALIGN) {
+		/* Allocate the padding exactly */
+		iv_size_padding = -(cc->dmreq_start + sizeof(struct dm_crypt_request))
+				& crypto_ablkcipher_alignmask(any_tfm(cc));
+	} else {
+		/*
+		 * If the cipher requires greater alignment than kmalloc
+		 * alignment, we don't know the exact position of the
+		 * initialization vector. We must assume worst case.
+		 */
+		iv_size_padding = crypto_ablkcipher_alignmask(any_tfm(cc));
+	}
 
 	cc->req_pool = mempool_create_kmalloc_pool(MIN_IOS, cc->dmreq_start +
-			sizeof(struct dm_crypt_request) + cc->iv_size);
+			sizeof(struct dm_crypt_request) +
+			iv_size_padding + cc->iv_size);
 	if (!cc->req_pool) {
 		ti->error = "Cannot allocate crypt request mempool";
 		goto bad;
 	}
 
-	cc->per_bio_data_size = ti->per_bio_data_size =
-				sizeof(struct dm_crypt_io) + cc->dmreq_start +
-				sizeof(struct dm_crypt_request) + cc->iv_size;
+	cc->per_bio_data_size = ti->per_bio_data_size = ALIGN(
+			sizeof(struct dm_crypt_io) + cc->dmreq_start +
+			sizeof(struct dm_crypt_request) +
+			iv_size_padding + cc->iv_size,
+		ARCH_KMALLOC_MINALIGN);
 
 	cc->page_pool = mempool_create_page_pool(MIN_POOL_PAGES, 0);
 	if (!cc->page_pool) {

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-08-28 19:28 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-28 15:09 [PATCH] dm-crypt: Fix access beyond the end of allocated space Mikulas Patocka
2014-08-28 17:59 ` Milan Broz
2014-08-28 18:40   ` Mike Snitzer
2014-08-28 19:28   ` [PATCH] " Krzysztof Kolasa

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.