| Message ID | 20180625211026.15819-11-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
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@gondor.apana.org.au> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Eric Biggers <ebiggers@google.com> > Cc: Tim Chen <tim.c.chen@linux.intel.com> > Cc: Rabin Vincent <rabinv@axis.com> > Cc: Lars Persson <larper@axis.com> > Cc: linux-crypto@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.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 ]--- -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, Jun 25, 2018 at 3:56 PM, Eric Biggers <ebiggers3@gmail.com> wrote: > 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. >> > > 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: Ugh, right. Wow, that _really_ gets big. Which are likely to wrap which others? Looks like software case plus hardware case? i.e. mcryptd_hash_request_ctx with artpec6_hash_request_context is the largest we could get? So: 80 + 80 + 200 + 664 ? Oh, hilarious. That comes exactly to 1024. :P So ... 1024? -Kees
On Mon, Jun 25, 2018 at 03:56:09PM -0700, Eric Biggers wrote: > > > 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 I think this patch is on the wrong track. The stack requests are only ever meant to be used for synchronous algorithms (IOW shash algorithms) and were a quick-and-dirty fix for legacy users. So either check SHASH_MAX_REQSIZE or just convert the users to kmalloc or even better make them real async users. Cheers,
On Tue, Jun 26, 2018 at 2:19 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Mon, Jun 25, 2018 at 03:56:09PM -0700, Eric Biggers wrote: >> >> > 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 > > I think this patch is on the wrong track. The stack requests are > only ever meant to be used for synchronous algorithms (IOW shash > algorithms) and were a quick-and-dirty fix for legacy users. > > So either check SHASH_MAX_REQSIZE or just convert the users to > kmalloc or even better make them real async users. There is no SHASH_MAX_REQSIZE? As for users of AHASH_REQUEST_ON_STACK, I see: $ git grep AHASH_REQUEST_ON_STACK arch/x86/power/hibernate_64.c: AHASH_REQUEST_ON_STACK(req, tfm); crypto/ccm.c: AHASH_REQUEST_ON_STACK(ahreq, ctx->mac); drivers/block/drbd/drbd_worker.c: AHASH_REQUEST_ON_STACK(req, tfm); drivers/block/drbd/drbd_worker.c: AHASH_REQUEST_ON_STACK(req, tfm); drivers/md/dm-crypt.c: AHASH_REQUEST_ON_STACK(req, essiv->hash_tfm); drivers/net/ppp/ppp_mppe.c: AHASH_REQUEST_ON_STACK(req, state->sha1); drivers/staging/rtl8192e/rtllib_crypt_tkip.c: AHASH_REQUEST_ON_STACK(req, tfm_michael); drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c: AHASH_REQUEST_ON_STACK(req, tfm_michael); net/wireless/lib80211_crypt_tkip.c: AHASH_REQUEST_ON_STACK(req, tfm_michael); Regardless, I'll take a closer look at these. The other patches leading up to the REQSIZE ones, though, I think are ready to go? They're distinct from the last two, so the first 9 patches could be applied and I'll continue to improve the two REQSIZE ones? If that sounds okay, do you want me to resend just first 9, or do you want to take them from this series? Thanks! -Kees
On Tue, Jun 26, 2018 at 10:02:31AM -0700, Kees Cook wrote: > > There is no SHASH_MAX_REQSIZE? > > As for users of AHASH_REQUEST_ON_STACK, I see: These users are only using the top-level ahash interface. The underlying algorithms must all be shas. Cheers,
On Wed, Jun 27, 2018 at 7:34 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, Jun 26, 2018 at 10:02:31AM -0700, Kees Cook wrote: >> >> There is no SHASH_MAX_REQSIZE? >> >> As for users of AHASH_REQUEST_ON_STACK, I see: > > These users are only using the top-level ahash interface. The > underlying algorithms must all be shas. typo? "shash" you mean? I don't really understand the crypto APIs -- are you or Eric able to help me a bit more here? I don't understand that things can wrap other things, so I'm not sure the best way to reason about the maximum size to choose here. (And the same for skcipher.) -Kees
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; }
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 <ebiggers@google.com> Cc: Tim Chen <tim.c.chen@linux.intel.com> Cc: Rabin Vincent <rabinv@axis.com> Cc: Lars Persson <larper@axis.com> Cc: linux-crypto@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- include/crypto/hash.h | 3 ++- include/crypto/internal/hash.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)