| Message ID | 20180625211026.15819-12-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On Mon, Jun 25, 2018 at 02:10:26PM -0700, Kees Cook wrote: > In the quest to remove all stack VLA usage from the kernel[1], this > caps the skcipher request size similar to other limits and adds a > sanity check at registration. In a manual review of the callers of > crypto_skcipher_set_reqsize(), the largest was 384: > > 4 sun4i_cipher_req_ctx > 6 safexcel_cipher_req > 8 cryptd_skcipher_request_ctx > 80 cipher_req_ctx > 80 skcipher_request > 96 crypto_rfc3686_req_ctx > 104 nitrox_kcrypt_request > 144 mv_cesa_skcipher_std_req > 384 rctx > > [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: linux-crypto@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> This has the same issue as the ahash reqsize patch. Cheers,
On Tue, Jun 26, 2018 at 2:20 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Mon, Jun 25, 2018 at 02:10:26PM -0700, Kees Cook wrote: >> In the quest to remove all stack VLA usage from the kernel[1], this >> caps the skcipher request size similar to other limits and adds a >> sanity check at registration. In a manual review of the callers of >> crypto_skcipher_set_reqsize(), the largest was 384: >> >> 4 sun4i_cipher_req_ctx >> 6 safexcel_cipher_req >> 8 cryptd_skcipher_request_ctx >> 80 cipher_req_ctx >> 80 skcipher_request >> 96 crypto_rfc3686_req_ctx >> 104 nitrox_kcrypt_request >> 144 mv_cesa_skcipher_std_req >> 384 rctx >> >> [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: linux-crypto@vger.kernel.org >> Signed-off-by: Kees Cook <keescook@chromium.org> > > This has the same issue as the ahash reqsize patch. Which are likely to be wrapped together? Should I take this to 512 or something else? -Kees
On Tue, Jun 26, 2018 at 09:45:09AM -0700, Kees Cook wrote: > > Which are likely to be wrapped together? Should I take this to 512 or > something else? The situation is similar to ahash. While they're using the same skcipher interface, the underlying algorithms must all be synchronous. In fact, if they're not then they're buggy. Therefore it makes no sense to use the general skcipher request size as a threshold. You should look at synchronous skcipher algorithms only. Cheers,
On Wed, Jun 27, 2018 at 7:36 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, Jun 26, 2018 at 09:45:09AM -0700, Kees Cook wrote: >> >> Which are likely to be wrapped together? Should I take this to 512 or >> something else? > > The situation is similar to ahash. While they're using the same > skcipher interface, the underlying algorithms must all be > synchronous. In fact, if they're not then they're buggy. > > Therefore it makes no sense to use the general skcipher request > size as a threshold. You should look at synchronous skcipher > algorithms only. I might be catching on... so from this list, I should only "count" the synchronous ones as being wrappable? The skcipher list is actually pretty short: crypto/cryptd.c: crypto_skcipher_set_reqsize( crypto/cryptd.c- tfm, sizeof(struct cryptd_skcipher_request_ctx)); The above is, AIUI, unwrapped, so I only need to count sizeof(struct cryptd_skcipher_request_ctx)? These are "simple" wrappers: crypto/lrw.c: crypto_skcipher_set_reqsize(tfm, crypto_skcipher_reqsize(cipher) + crypto/lrw.c- sizeof(struct rctx)); crypto/simd.c- reqsize = sizeof(struct skcipher_request); crypto/simd.c- reqsize += crypto_skcipher_reqsize(&cryptd_tfm->base); crypto/simd.c: crypto_skcipher_set_reqsize(tfm, reqsize); crypto/xts.c: crypto_skcipher_set_reqsize(tfm, crypto_skcipher_reqsize(child) + crypto/xts.c- sizeof(struct rctx)); But what are the "legitimate" existing crypto_skcipher_reqsize() values here? These are "complex" wrappers, with cts even adding blocksize to the mix... crypto/ctr.c- align = crypto_skcipher_alignmask(tfm); crypto/ctr.c- align &= ~(crypto_tfm_ctx_alignment() - 1); crypto/ctr.c- reqsize = align + sizeof(struct crypto_rfc3686_req_ctx) + crypto/ctr.c- crypto_skcipher_reqsize(cipher); crypto/ctr.c: crypto_skcipher_set_reqsize(tfm, reqsize); crypto/cts.c- align = crypto_skcipher_alignmask(tfm); crypto/cts.c- bsize = crypto_skcipher_blocksize(cipher); crypto/cts.c- reqsize = ALIGN(sizeof(struct crypto_cts_reqctx) + crypto/cts.c- crypto_skcipher_reqsize(cipher), crypto/cts.c- crypto_tfm_ctx_alignment()) + crypto/cts.c- (align & ~(crypto_tfm_ctx_alignment() - 1)) + bsize; crypto/cts.c- crypto/cts.c: crypto_skcipher_set_reqsize(tfm, reqsize); What values might be expected here? It seems the entire blocksize needs to be included as well... -Kees
On Wed, Jun 27, 2018 at 11:31:09AM -0700, Kees Cook wrote: > > I might be catching on... so from this list, I should only "count" the > synchronous ones as being wrappable? The skcipher list is actually > pretty short: > > crypto/cryptd.c: crypto_skcipher_set_reqsize( > crypto/cryptd.c- tfm, sizeof(struct > cryptd_skcipher_request_ctx)); cryptd is async so you don't have to include it. > These are "simple" wrappers: > > crypto/lrw.c: crypto_skcipher_set_reqsize(tfm, > crypto_skcipher_reqsize(cipher) + > crypto/lrw.c- sizeof(struct rctx)); > > crypto/simd.c- reqsize = sizeof(struct skcipher_request); > crypto/simd.c- reqsize += crypto_skcipher_reqsize(&cryptd_tfm->base); > crypto/simd.c: crypto_skcipher_set_reqsize(tfm, reqsize); simd is async. > crypto/xts.c: crypto_skcipher_set_reqsize(tfm, > crypto_skcipher_reqsize(child) + > crypto/xts.c- sizeof(struct rctx)); > > But what are the "legitimate" existing crypto_skcipher_reqsize() values here? > > These are "complex" wrappers, with cts even adding blocksize to the mix... > > crypto/ctr.c- align = crypto_skcipher_alignmask(tfm); > crypto/ctr.c- align &= ~(crypto_tfm_ctx_alignment() - 1); > crypto/ctr.c- reqsize = align + sizeof(struct crypto_rfc3686_req_ctx) + > crypto/ctr.c- crypto_skcipher_reqsize(cipher); > crypto/ctr.c: crypto_skcipher_set_reqsize(tfm, reqsize); > > crypto/cts.c- align = crypto_skcipher_alignmask(tfm); > crypto/cts.c- bsize = crypto_skcipher_blocksize(cipher); > crypto/cts.c- reqsize = ALIGN(sizeof(struct crypto_cts_reqctx) + > crypto/cts.c- crypto_skcipher_reqsize(cipher), > crypto/cts.c- crypto_tfm_ctx_alignment()) + > crypto/cts.c- (align & ~(crypto_tfm_ctx_alignment() - 1)) + bsize; > crypto/cts.c- > crypto/cts.c: crypto_skcipher_set_reqsize(tfm, reqsize); > > What values might be expected here? It seems the entire blocksize > needs to be included as well... But otherwise yes these are the ones that count. Cheers,
On Wed, Jun 27, 2018 at 3:27 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Wed, Jun 27, 2018 at 11:31:09AM -0700, Kees Cook wrote: >> crypto/lrw.c: crypto_skcipher_set_reqsize(tfm, >> crypto_skcipher_reqsize(cipher) + >> crypto/lrw.c- sizeof(struct rctx)); >> ... >> crypto/cts.c- align = crypto_skcipher_alignmask(tfm); >> crypto/cts.c- bsize = crypto_skcipher_blocksize(cipher); >> crypto/cts.c- reqsize = ALIGN(sizeof(struct crypto_cts_reqctx) + >> crypto/cts.c- crypto_skcipher_reqsize(cipher), >> crypto/cts.c- crypto_tfm_ctx_alignment()) + >> crypto/cts.c- (align & ~(crypto_tfm_ctx_alignment() - 1)) + bsize; >> crypto/cts.c- >> crypto/cts.c: crypto_skcipher_set_reqsize(tfm, reqsize); >> >> What values might be expected here? It seems the entire blocksize >> needs to be included as well... > > But otherwise yes these are the ones that count. In both cases here, what is "cipher"? i.e. what ciphers could lrw be wrapping, and what ciphers could cts be wrapping, so that I can examine the blocksizes, etc? FWIW, looking at the non-ASYNC wrappers, I see only: crypto/ctr.c crypto/cts.c crypto/lrw.c crypto/xts.c drivers/crypto/sunxi-ss/sun4i-ss-cipher.c Building in all crypto things and running tcrypt with an instrumented crypto_skcipher_set_reqsize, I see: # dmesg | grep skcipher_set_req | cut -c16- | sort -u | sort +1 -n crypto_skcipher_set_reqsize: 8 crypto_skcipher_set_reqsize: 88 crypto_skcipher_set_reqsize: 184 crypto_skcipher_set_reqsize: 256 crypto_skcipher_set_reqsize: 472 The 472 maps to lrw with its 384 struct rctx: [ 553.843884] tcrypt: testing lrw(twofish) [ 553.844479] crypto_skcipher_set_reqsize: 8 [ 553.845076] crypto_skcipher_set_reqsize: 88 [ 553.845658] crypto_skcipher_set_reqsize: 472 [ 553.860578] tcrypt: testing lrw(serpent) [ 553.861349] crypto_skcipher_set_reqsize: 8 [ 553.861960] crypto_skcipher_set_reqsize: 88 [ 553.862534] crypto_skcipher_set_reqsize: 472 [ 553.871676] tcrypt: testing lrw(aes) [ 553.872398] crypto_skcipher_set_reqsize: 8 [ 553.873002] crypto_skcipher_set_reqsize: 88 [ 553.873574] crypto_skcipher_set_reqsize: 472 [ 553.957282] tcrypt: testing lrw(cast6) [ 553.958098] crypto_skcipher_set_reqsize: 8 [ 553.958691] crypto_skcipher_set_reqsize: 88 [ 553.959311] crypto_skcipher_set_reqsize: 472 [ 553.982514] tcrypt: testing lrw(camellia) [ 553.983308] crypto_skcipher_set_reqsize: 8 [ 553.983907] crypto_skcipher_set_reqsize: 88 [ 553.984470] crypto_skcipher_set_reqsize: 472 And while I'm using tcrypt, ahash shows: 44 124 336 368 528 536 568 616 648 728 808 The largest seems to be sha512: [ 553.883440] tcrypt: testing sha512 [ 553.884179] sha512_mb: crypto_ahash_set_reqsize: 528 [ 553.884904] crypto_ahash_set_reqsize: 728 [ 553.885449] sha512_mb: crypto_ahash_set_reqsize: 808 So ... should I use 472 for skcipher and 808 for ahash? -Kees
On Wed, Jun 27, 2018 at 05:10:05PM -0700, Kees Cook wrote: > > In both cases here, what is "cipher"? i.e. what ciphers could lrw be > wrapping, and what ciphers could cts be wrapping, so that I can > examine the blocksizes, etc? A cipher is a simple cipher like aes that operates on a single block. Cheers,
diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h index e42f7063f245..5035482cbe68 100644 --- a/include/crypto/internal/skcipher.h +++ b/include/crypto/internal/skcipher.h @@ -130,6 +130,7 @@ static inline struct crypto_skcipher *crypto_spawn_skcipher( static inline void crypto_skcipher_set_reqsize( struct crypto_skcipher *skcipher, unsigned int reqsize) { + BUG_ON(reqsize > SKCIPHER_MAX_REQSIZE); skcipher->reqsize = reqsize; } diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h index 2f327f090c3e..26eba8304d1d 100644 --- a/include/crypto/skcipher.h +++ b/include/crypto/skcipher.h @@ -139,9 +139,11 @@ struct skcipher_alg { struct crypto_alg base; }; +#define SKCIPHER_MAX_REQSIZE 384 + #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \ char __##name##_desc[sizeof(struct skcipher_request) + \ - crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \ + SKCIPHER_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \ struct skcipher_request *name = (void *)__##name##_desc /**
In the quest to remove all stack VLA usage from the kernel[1], this caps the skcipher request size similar to other limits and adds a sanity check at registration. In a manual review of the callers of crypto_skcipher_set_reqsize(), the largest was 384: 4 sun4i_cipher_req_ctx 6 safexcel_cipher_req 8 cryptd_skcipher_request_ctx 80 cipher_req_ctx 80 skcipher_request 96 crypto_rfc3686_req_ctx 104 nitrox_kcrypt_request 144 mv_cesa_skcipher_std_req 384 rctx [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: linux-crypto@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- include/crypto/internal/skcipher.h | 1 + include/crypto/skcipher.h | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)