| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 25 Jun 2018 15:56:09 -0700
From: Eric Biggers <ebiggers3@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Herbert Xu <herbert@...dor.apana.org.au>,
Giovanni Cabiddu <giovanni.cabiddu@...el.com>,
Arnd Bergmann <arnd@...db.de>,
Eric Biggers <ebiggers@...gle.com>,
Mike Snitzer <snitzer@...hat.com>,
"Gustavo A. R. Silva" <gustavo@...eddedor.com>,
qat-linux@...el.com, linux-kernel@...r.kernel.org,
dm-devel@...hat.com, linux-crypto@...r.kernel.org,
Lars Persson <larper@...s.com>,
Tim Chen <tim.c.chen@...ux.intel.com>,
"David S. Miller" <davem@...emloft.net>,
Alasdair Kergon <agk@...hat.com>,
Rabin Vincent <rabinv@...s.com>
Subject: Re: [dm-devel] [PATCH v2 10/11] crypto: ahash: Remove VLA usage for
AHASH_REQUEST_ON_STACK
On Mon, Jun 25, 2018 at 02:10:25PM -0700, Kees Cook wrote:
> 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@...dor.apana.org.au>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Eric Biggers <ebiggers@...gle.com>
> Cc: Tim Chen <tim.c.chen@...ux.intel.com>
> Cc: Rabin Vincent <rabinv@...s.com>
> Cc: Lars Persson <larper@...s.com>
> Cc: linux-crypto@...r.kernel.org
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> 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 ]---
Powered by blists - more mailing lists