| Message ID | 20190930121520.1388317-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 74e6bd472b6d9e80ec9972989d8991736fe46c51 |
| Headers | show |
| Series | [1/3] crypto: inside-secure - Fix a maybe-uninitialized warning | expand |
> -----Original Message----- > From: Arnd Bergmann <arnd@arndb.de> > Sent: Monday, September 30, 2019 2:15 PM > To: Antoine Tenart <antoine.tenart@bootlin.com>; Herbert Xu <herbert@gondor.apana.org.au>; > David S. Miller <davem@davemloft.net> > Cc: Arnd Bergmann <arnd@arndb.de>; Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>; Pascal > van Leeuwen <pascalvanl@gmail.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Eric Biggers > <ebiggers@google.com>; linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH 2/3] crypto: inside-secure - Reduce stack usage > > safexcel_aead_setkey() contains three large stack variables, totalling > slightly more than the 1024 byte warning limit: > > drivers/crypto/inside-secure/safexcel_cipher.c:303:12: error: stack frame size of 1032 bytes > in function 'safexcel_aead_setkey' [-Werror,-Wframe-larger-than=] > Ok, I did not realise that, so thanks for pointing that out to me. > The function already contains a couple of dynamic allocations, so it is > likely not performance critical and it can only be called in a context > that allows sleeping, so the easiest workaround is to add change it > to use dynamic allocations. Combining istate and ostate into a single > variable simplifies the allocation at the cost of making it slightly > less readable. > Hmmm... I wouldn't exactly consider it to be not performance critical - it can be under certain circumstanced, but I guess it's already wasting lots of cycles on allocations and key precomputes in safexcel_hmac_setkey, so for now dynamically allocating the state is fine. > Alternatively, it should be possible to shrink these allocations > as the extra buffers appear to be largely unnecessary, but doing > this would be a much more invasive change. > Actually, for HMAC-SHA512 you DO need all that buffer space. You could shrink it to 2 * ctx->state_sz but then your simple indexing is no longer going to fly. Not sure if that would be worth the effort. I don't like the part where you dynamically allocate the cryto_aes_ctx though, I think that was not necessary considering its a lot smaller. And it conflicts with another change I have waiting that gets rid of aes_expandkey and that struct alltogether (since it was really just abused to do a key size check, which was very wasteful since the function actually generates all roundkeys we don't need at all ...) So I'll get rid of that struct anyway and doing it in this patch just complicates applying your patch to my code or rebasing my stuff later. > Fixes: 0e17e3621a28 ("crypto: inside-secure - add support for > authenc(hmac(sha*),rfc3686(ctr(aes))) suites") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > .../crypto/inside-secure/safexcel_cipher.c | 53 ++++++++++++------- > 1 file changed, 35 insertions(+), 18 deletions(-) > > diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside- > secure/safexcel_cipher.c > index ef51f8c2b473..51a4112aa9bc 100644 > --- a/drivers/crypto/inside-secure/safexcel_cipher.c > +++ b/drivers/crypto/inside-secure/safexcel_cipher.c > @@ -305,10 +305,10 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 > *key, > { > struct crypto_tfm *tfm = crypto_aead_tfm(ctfm); > struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm); > - struct safexcel_ahash_export_state istate, ostate; > + struct safexcel_ahash_export_state *state; > struct safexcel_crypto_priv *priv = ctx->priv; > + struct crypto_aes_ctx *aes; > struct crypto_authenc_keys keys; > - struct crypto_aes_ctx aes; > int err = -EINVAL; > > if (crypto_authenc_extractkeys(&keys, key, len) != 0) > @@ -334,7 +334,14 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key, > goto badkey_expflags; > break; > case SAFEXCEL_AES: > - err = aes_expandkey(&aes, keys.enckey, keys.enckeylen); > + aes = kzalloc(sizeof(*aes), GFP_KERNEL); > + if (!aes) { > + err = -ENOMEM; > + goto badkey; > + } > + > + err = aes_expandkey(aes, keys.enckey, keys.enckeylen); > + kfree(aes); > if (unlikely(err)) > goto badkey; > break; > @@ -347,56 +354,66 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 > *key, > memcmp(ctx->key, keys.enckey, keys.enckeylen)) > ctx->base.needs_inv = true; > > + state = kzalloc(sizeof(struct safexcel_ahash_export_state) * 2, GFP_KERNEL); > + if (!state) { > + err = -ENOMEM; > + goto badkey; > + } > + > /* Auth key */ > switch (ctx->hash_alg) { > case CONTEXT_CONTROL_CRYPTO_ALG_SHA1: > if (safexcel_hmac_setkey("safexcel-sha1", keys.authkey, > - keys.authkeylen, &istate, &ostate)) > - goto badkey; > + keys.authkeylen, &state[0], &state[1])) > + goto badkey_free; > break; > case CONTEXT_CONTROL_CRYPTO_ALG_SHA224: > if (safexcel_hmac_setkey("safexcel-sha224", keys.authkey, > - keys.authkeylen, &istate, &ostate)) > - goto badkey; > + keys.authkeylen, &state[0], &state[1])) > + goto badkey_free; > break; > case CONTEXT_CONTROL_CRYPTO_ALG_SHA256: > if (safexcel_hmac_setkey("safexcel-sha256", keys.authkey, > - keys.authkeylen, &istate, &ostate)) > - goto badkey; > + keys.authkeylen, &state[0], &state[1])) > + goto badkey_free; > break; > case CONTEXT_CONTROL_CRYPTO_ALG_SHA384: > if (safexcel_hmac_setkey("safexcel-sha384", keys.authkey, > - keys.authkeylen, &istate, &ostate)) > - goto badkey; > + keys.authkeylen, &state[0], &state[1])) > + goto badkey_free; > break; > case CONTEXT_CONTROL_CRYPTO_ALG_SHA512: > if (safexcel_hmac_setkey("safexcel-sha512", keys.authkey, > - keys.authkeylen, &istate, &ostate)) > - goto badkey; > + keys.authkeylen, &state[0], &state[1])) > + goto badkey_free; > break; > default: > dev_err(priv->dev, "aead: unsupported hash algorithm\n"); > - goto badkey; > + goto badkey_free; > } > > crypto_aead_set_flags(ctfm, crypto_aead_get_flags(ctfm) & > CRYPTO_TFM_RES_MASK); > > if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma && > - (memcmp(ctx->ipad, istate.state, ctx->state_sz) || > - memcmp(ctx->opad, ostate.state, ctx->state_sz))) > + (memcmp(ctx->ipad, &state[0].state, ctx->state_sz) || > + memcmp(ctx->opad, &state[1].state, ctx->state_sz))) > ctx->base.needs_inv = true; > > /* Now copy the keys into the context */ > memcpy(ctx->key, keys.enckey, keys.enckeylen); > ctx->key_len = keys.enckeylen; > > - memcpy(ctx->ipad, &istate.state, ctx->state_sz); > - memcpy(ctx->opad, &ostate.state, ctx->state_sz); > + memcpy(ctx->ipad, &state[0].state, ctx->state_sz); > + memcpy(ctx->opad, &state[1].state, ctx->state_sz); > > memzero_explicit(&keys, sizeof(keys)); > + kfree(state); > + > return 0; > > +badkey_free: > + kfree(state); > badkey: > crypto_aead_set_flags(ctfm, CRYPTO_TFM_RES_BAD_KEY_LEN); > badkey_expflags: > -- > 2.20.0 Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
On Mon, Sep 30, 2019 at 02:14:34PM +0200, Arnd Bergmann wrote: > safexcel_aead_setkey() contains three large stack variables, totalling > slightly more than the 1024 byte warning limit: > > drivers/crypto/inside-secure/safexcel_cipher.c:303:12: error: stack frame size of 1032 bytes in function 'safexcel_aead_setkey' [-Werror,-Wframe-larger-than=] > > The function already contains a couple of dynamic allocations, so it is > likely not performance critical and it can only be called in a context > that allows sleeping, so the easiest workaround is to add change it > to use dynamic allocations. Combining istate and ostate into a single > variable simplifies the allocation at the cost of making it slightly > less readable. > > Alternatively, it should be possible to shrink these allocations > as the extra buffers appear to be largely unnecessary, but doing > this would be a much more invasive change. > > Fixes: 0e17e3621a28 ("crypto: inside-secure - add support for authenc(hmac(sha*),rfc3686(ctr(aes))) suites") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > .../crypto/inside-secure/safexcel_cipher.c | 53 ++++++++++++------- > 1 file changed, 35 insertions(+), 18 deletions(-) This patch doesn't apply against the current cryptodev tree. Please respin it. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c index 4ab1bde8dd9b..311bf60df39f 100644 --- a/drivers/crypto/inside-secure/safexcel.c +++ b/drivers/crypto/inside-secure/safexcel.c @@ -1120,6 +1120,8 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, irq_name, irq); return irq; } + } else { + return -ENXIO; } ret = devm_request_threaded_irq(dev, irq, handler,
A previous fixup avoided an unused variable warning but replaced it with a slightly scarier warning: drivers/crypto/inside-secure/safexcel.c:1100:6: error: variable 'irq' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] This is harmless as it is impossible to get into this case, but the compiler has no way of knowing that. Add an explicit error handling case to make it obvious to both compilers and humans reading the source. Fixes: 212ef6f29e5b ("crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/crypto/inside-secure/safexcel.c | 2 ++ 1 file changed, 2 insertions(+) -- 2.20.0