Re: [dm-devel] [PATCH v2 10/11] crypto: ahash: Remove VLA usage for AHASH_REQUEST_ON_STACK
From: Eric Biggers <hidden>
Date: 2018-06-25 22:56:19
Also in:
dm-devel, lkml
On Mon, Jun 25, 2018 at 02:10:25PM -0700, Kees Cook wrote:
quoted hunk ↗ jump to hunk
In the quest to remove all stack VLA usage from the kernel[1], this caps the ahash request size similar to the other limits and adds a sanity check at registration. Unfortunately, these reqsizes can be huge. Looking at all callers of crypto_ahash_set_reqsize(), the largest appears to be 664 bytes, based on a combination of manual inspection and pahole output: 4 dcp_sha_req_ctx 40 crypto4xx_ctx 52 hash_req_ctx 80 ahash_request 88 rk_ahash_rctx 104 sun4i_req_ctx 200 mcryptd_hash_request_ctx 216 safexcel_ahash_req 228 sha1_hash_ctx 228 sha256_hash_ctx 248 img_hash_request_ctx 252 mtk_sha_reqctx 276 sahara_sha_reqctx 304 mv_cesa_ahash_req 316 iproc_reqctx_s 320 caam_hash_state 320 qce_sha_reqctx 356 sha512_hash_ctx 384 ahash_req_ctx 400 chcr_ahash_req_ctx 416 stm32_hash_request_ctx 432 talitos_ahash_req_ctx 462 atmel_sha_reqctx 496 ccp_aes_cmac_req_ctx 616 ccp_sha_req_ctx 664 artpec6_hash_request_context So, this chooses 720 as a larger "round" number for the max. [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Biggers <redacted> Cc: Tim Chen <redacted> Cc: Rabin Vincent <redacted> Cc: Lars Persson <redacted> Cc: linux-crypto@vger.kernel.org Signed-off-by: Kees Cook <redacted> --- include/crypto/hash.h | 3 ++- include/crypto/internal/hash.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)diff --git a/include/crypto/hash.h b/include/crypto/hash.h index 5d79e2f0936e..b550077c4767 100644 --- a/include/crypto/hash.h +++ b/include/crypto/hash.h@@ -66,10 +66,11 @@ struct ahash_request { #define AHASH_MAX_DIGESTSIZE 512 #define AHASH_MAX_STATESIZE 512 +#define AHASH_MAX_REQSIZE 720 #define AHASH_REQUEST_ON_STACK(name, ahash) \ char __##name##_desc[sizeof(struct ahash_request) + \ - crypto_ahash_reqsize(ahash)] CRYPTO_MINALIGN_ATTR; \ + AHASH_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \ struct ahash_request *name = (void *)__##name##_desc /**diff --git a/include/crypto/internal/hash.h b/include/crypto/internal/hash.h index a0b0ad9d585e..d96ae5f52125 100644 --- a/include/crypto/internal/hash.h +++ b/include/crypto/internal/hash.h@@ -142,6 +142,7 @@ static inline struct ahash_alg *__crypto_ahash_alg(struct crypto_alg *alg) static inline void crypto_ahash_set_reqsize(struct crypto_ahash *tfm, unsigned int reqsize) { + BUG_ON(reqsize > AHASH_MAX_REQSIZE); tfm->reqsize = reqsize; }
This isn't accounting for the cases where a hash algorithm is "wrapped" with another one, which increases the request size. For example, "sha512_mb" ends up with a request size of sizeof(struct ahash_request) + sizeof(struct mcryptd_hash_request_ctx) + sizeof(struct ahash_request) + sizeof(struct sha512_hash_ctx) == 808 bytes, on x86_64 with CONFIG_DEBUG_SG enabled. (Note also that structure sizes can vary depending on the architecture and the kernel config.) So, with the self-tests enabled your new BUG_ON() is hit on boot: ------------[ cut here ]------------ kernel BUG at ./include/crypto/internal/hash.h:145! invalid opcode: 0000 [#1] SMP PTI CPU: 4 PID: 337 Comm: cryptomgr_test Not tainted 4.18.0-rc2-00048-gda396e1e8ca5 #11 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 RIP: 0010:mcryptd_hash_init_tfm+0x36/0x40 crypto/mcryptd.c:289 Code: 01 00 00 e8 0c 05 fd ff 48 3d 00 f0 ff ff 77 18 48 89 43 40 8b 40 40 05 c8 00 00 00 3d d0 02 00 00 77 07 89 43 f8 31 c0 5b c3 <0f> 0b 0f 1f 84 00 00 00 00 00 80 7f 0c 00 74 01 c3 65 8b 05 d2 e2 RSP: 0000:ffffb180c0dafc50 EFLAGS: 00010202 RAX: 00000000000002d8 RBX: ffffa1867c9267c8 RCX: ffffffffb66f5ef0 RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffa1867c927c48 RBP: ffffa1867c926780 R08: 0000000000000001 R09: ffffa1867c927c00 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 R13: ffffa1867c9c6848 R14: ffffa1867c9c6848 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffffa1867fd00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000000 CR3: 0000000035c10001 CR4: 00000000001606e0 Call Trace: crypto_create_tfm+0x86/0xd0 crypto/api.c:475 crypto_alloc_tfm+0x4b/0xb0 crypto/api.c:543 mcryptd_alloc_ahash+0x6f/0xb0 crypto/mcryptd.c:603 sha512_mb_async_init_tfm+0x1a/0x50 arch/x86/crypto/sha512-mb/sha512_mb.c:724 crypto_create_tfm+0x86/0xd0 crypto/api.c:475 crypto_alloc_tfm+0x4b/0xb0 crypto/api.c:543 __alg_test_hash+0x1c/0x90 crypto/testmgr.c:1799 alg_test_hash+0x6b/0x100 crypto/testmgr.c:1841 alg_test.part.5+0x119/0x2a0 crypto/testmgr.c:3687 cryptomgr_test+0x3b/0x40 crypto/algboss.c:223 kthread+0x114/0x130 kernel/kthread.c:240 ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412 Modules linked in: ---[ end trace d726be03a53bddb5 ]---