[PATCH v4 00/10] crypto: Provide a function for zeroizing crypto_aes_ctx

[PATCH v4 00/10] crypto: Provide a function for zeroizing crypto_aes_ctx

[Thomas Huth]

Several crypto drivers need to zeroize their local crypto_aes_ctx
structures after use to avoid leaking key material on the stack.
Currently some call sites do this with their own memzero_explicit()
call, which is error-prone since it is easy to miss a return path
(what already happened in a driver). Some other call sites miss
to clear crypto_aes_ctx completely.

To improve this situation, the first patch introduces an aes_zeroize_ctx()
helper that can be used with __cleanup() to automatically zeroize the
context when it goes out of scope. The following 6 patches add this
__cleanup() to spots in the code where this has been forgotten so far.
The final patches change some files to do the zeroization with
the new __cleanup() way instead of calling memzero_explicit() manually.

v4:
- Updated the function description in the first patch
- Fixed "return err" bug in the "safexcel - Rework cleanup..." patch

v3:
- Renamed aes_clear_ctx() to aes_zeroize_ctx()
- Split up the safeexcel patch to rework safexcel_aead_setkey in a
  separate patch
- Removed goto in the padlock patch

v2:
- Rebased onto cryptodev master branch, updated the "qat" patch accordingly

Thomas Huth (10):
  crypto: Provide a wrapper function for zeroizing crypto_aes_ctx
  crypto: aspeed - clear the crypto_aes_ctx when done
  crypto: inside-secure/eip93 - clear the crypto_aes_ctx when done
  crypto: padlock-aes - clear the crypto_aes_ctx when done
  crypto: sa2ul - clear the crypto_aes_ctx when done
  crypto: arm/aes-neonbs - clear the crypto_aes_ctx when done
  crypto: arm64/aes-neonbs - clear the crypto_aes_ctx when done
  crypto: qat - zeroize crypto_aes_ctx with __cleanup(aes_zeroize_ctx)
  crypto: safexcel - Rework cleanup of sensitive structs in
    safexcel_aead_setkey
  crypto: safexcel - zeroize crypto_aes_ctx with
    __cleanup(aes_zeroize_ctx)

 arch/arm/crypto/aes-neonbs-glue.c             |  2 +-
 arch/arm64/crypto/aes-neonbs-glue.c           |  2 +-
 drivers/crypto/aspeed/aspeed-hace-crypto.c    |  3 +-
 .../crypto/inside-secure/eip93/eip93-aead.c   |  2 +-
 .../crypto/inside-secure/eip93/eip93-cipher.c |  2 +-
 .../crypto/inside-secure/safexcel_cipher.c    | 40 +++++++------------
 drivers/crypto/inside-secure/safexcel_hash.c  |  3 +-
 .../crypto/intel/qat/qat_common/qat_algs.c    |  3 +-
 drivers/crypto/padlock-aes.c                  | 22 +++++-----
 drivers/crypto/sa2ul.c                        |  2 +-
 include/crypto/aes.h                          | 13 ++++++
 11 files changed, 46 insertions(+), 48 deletions(-)

-- 
2.55.0

[PATCH v4 01/10] crypto: Provide a wrapper function for zeroizing crypto_aes_ctx

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Several crypto drivers need to zeroize their local crypto_aes_ctx
structures after use to avoid leaking key material on the stack.
Currently some call sites do this with their own memzero_explicit()
call, which is error-prone since it is easy to miss a return path
(what already happened in some drivers). Some other call sites miss
to clear crypto_aes_ctx completely.

Provide an aes_zeroize_ctx() helper that can be used with __cleanup()
to automatically zeroize the context when it goes out of scope.

Acked-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/crypto/aes.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/crypto/aes.h b/include/crypto/aes.h
index 3279cfa546085..eb970b8d623f9 100644
--- a/include/crypto/aes.h
+++ b/include/crypto/aes.h
@@ -159,6 +159,19 @@ static inline int aes_check_keylen(size_t keylen)
 int aes_expandkey(struct crypto_aes_ctx *ctx, const u8 *in_key,
 		  unsigned int key_len);
 
+/**
+ * aes_zeroize_ctx - Clear a crypto_aes_ctx structure
+ * @ctx:	The location of the context that should be zeroized
+ *
+ * Explicitly fills the crypto_aes_ctx with zeroes. This should be done
+ * once the context is not required anymore to avoid that its contents
+ * are leaked on the stack or heap (if not using kfree_sensitive()).
+ */
+static inline void aes_zeroize_ctx(struct crypto_aes_ctx *ctx)
+{
+	memzero_explicit(ctx, sizeof(*ctx));
+}
+
 /*
  * The following functions are temporarily exported for use by the AES mode
  * implementations in arch/$(SRCARCH)/crypto/.  These exports will go away when
-- 
2.55.0

[PATCH v4 02/10] crypto: aspeed - clear the crypto_aes_ctx when done

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Declare the gen_aes_key with __cleanup(aes_zeroize_ctx) to avoid that
its contents could be leaking via the stack when the function returns.
And since it is only required in one branch of the if-statement there,
move it to that block, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 drivers/crypto/aspeed/aspeed-hace-crypto.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/aspeed/aspeed-hace-crypto.c b/drivers/crypto/aspeed/aspeed-hace-crypto.c
index fa201dae1f81b..e7d3f611df05c 100644
--- a/drivers/crypto/aspeed/aspeed-hace-crypto.c
+++ b/drivers/crypto/aspeed/aspeed-hace-crypto.c
@@ -576,7 +576,6 @@ static int aspeed_aes_setkey(struct crypto_skcipher *cipher, const u8 *key,
 {
 	struct aspeed_cipher_ctx *ctx = crypto_skcipher_ctx(cipher);
 	struct aspeed_hace_dev *hace_dev = ctx->hace_dev;
-	struct crypto_aes_ctx gen_aes_key;
 
 	CIPHER_DBG(hace_dev, "keylen: %d bits\n", (keylen * 8));
 
@@ -585,9 +584,9 @@ static int aspeed_aes_setkey(struct crypto_skcipher *cipher, const u8 *key,
 		return -EINVAL;
 
 	if (ctx->hace_dev->version == AST2500_VERSION) {
+		struct crypto_aes_ctx gen_aes_key __cleanup(aes_zeroize_ctx);
 		aes_expandkey(&gen_aes_key, key, keylen);
 		memcpy(ctx->key, gen_aes_key.key_enc, AES_MAX_KEYLENGTH);
-
 	} else {
 		memcpy(ctx->key, key, keylen);
 	}
-- 
2.55.0

[PATCH v4 03/10] crypto: inside-secure/eip93 - clear the crypto_aes_ctx when done

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Clear the crypto_aes_ctx structure via __cleanup(aes_zeroize_ctx)
when we're done with it to avoid that key data could leak on the
stack.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 drivers/crypto/inside-secure/eip93/eip93-aead.c   | 2 +-
 drivers/crypto/inside-secure/eip93/eip93-cipher.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/inside-secure/eip93/eip93-aead.c b/drivers/crypto/inside-secure/eip93/eip93-aead.c
index 2bbd0af7b0e0e..973cebef5df37 100644
--- a/drivers/crypto/inside-secure/eip93/eip93-aead.c
+++ b/drivers/crypto/inside-secure/eip93/eip93-aead.c
@@ -92,7 +92,7 @@ static int eip93_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
 	struct crypto_tfm *tfm = crypto_aead_tfm(ctfm);
 	struct eip93_crypto_ctx *ctx = crypto_tfm_ctx(tfm);
 	struct crypto_authenc_keys keys;
-	struct crypto_aes_ctx aes;
+	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
 	struct sa_record *sa_record = ctx->sa_record;
 	u32 nonce = 0;
 	int ret;
diff --git a/drivers/crypto/inside-secure/eip93/eip93-cipher.c b/drivers/crypto/inside-secure/eip93/eip93-cipher.c
index 4dd7ab7503e85..7051b99ee6235 100644
--- a/drivers/crypto/inside-secure/eip93/eip93-cipher.c
+++ b/drivers/crypto/inside-secure/eip93/eip93-cipher.c
@@ -116,7 +116,7 @@ static int eip93_skcipher_setkey(struct crypto_skcipher *ctfm, const u8 *key,
 	}
 
 	if (flags & EIP93_ALG_AES) {
-		struct crypto_aes_ctx aes;
+		struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
 
 		ctx->blksize = AES_BLOCK_SIZE;
 		ret = aes_expandkey(&aes, key, keylen);
-- 
2.55.0

[PATCH v4 04/10] crypto: padlock-aes - clear the crypto_aes_ctx when done

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Clear the crypto_aes_ctx structure via __cleanup(aes_zeroize_ctx)
when we're done with it to avoid that key data could leak on the
stack.

And since __cleanup() (and __free()) should not be mixed with
"gotos" in the same function, turn the goto statement here
into a proper block of the related if-statement.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 drivers/crypto/padlock-aes.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/padlock-aes.c b/drivers/crypto/padlock-aes.c
index 1be549a07a219..400a889e924d7 100644
--- a/drivers/crypto/padlock-aes.c
+++ b/drivers/crypto/padlock-aes.c
@@ -109,7 +109,7 @@ static int aes_set_key(struct crypto_tfm *tfm, const u8 *in_key,
 {
 	struct aes_ctx *ctx = aes_ctx(tfm);
 	const __le32 *key = (const __le32 *)in_key;
-	struct crypto_aes_ctx gen_aes;
+	struct crypto_aes_ctx gen_aes __cleanup(aes_zeroize_ctx);
 	int cpu;
 
 	if (key_len % 8)
@@ -137,20 +137,18 @@ static int aes_set_key(struct crypto_tfm *tfm, const u8 *in_key,
 	ctx->cword.decrypt.ksize = ctx->cword.encrypt.ksize;
 
 	/* Don't generate extended keys if the hardware can do it. */
-	if (aes_hw_extkey_available(key_len))
-		goto ok;
+	if (!aes_hw_extkey_available(key_len)) {
+		ctx->D = ctx->d_data;
+		ctx->cword.encrypt.keygen = 1;
+		ctx->cword.decrypt.keygen = 1;
 
-	ctx->D = ctx->d_data;
-	ctx->cword.encrypt.keygen = 1;
-	ctx->cword.decrypt.keygen = 1;
+		if (aes_expandkey(&gen_aes, in_key, key_len))
+			return -EINVAL;
 
-	if (aes_expandkey(&gen_aes, in_key, key_len))
-		return -EINVAL;
-
-	memcpy(ctx->E, gen_aes.key_enc, AES_MAX_KEYLENGTH);
-	memcpy(ctx->D, gen_aes.key_dec, AES_MAX_KEYLENGTH);
+		memcpy(ctx->E, gen_aes.key_enc, AES_MAX_KEYLENGTH);
+		memcpy(ctx->D, gen_aes.key_dec, AES_MAX_KEYLENGTH);
+	}
 
-ok:
 	for_each_online_cpu(cpu)
 		if (&ctx->cword.encrypt == per_cpu(paes_last_cword, cpu) ||
 		    &ctx->cword.decrypt == per_cpu(paes_last_cword, cpu))
-- 
2.55.0

[PATCH v4 05/10] crypto: sa2ul - clear the crypto_aes_ctx when done

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Clear the crypto_aes_ctx structure via __cleanup(aes_zeroize_ctx) when
we're done with it to avoid that key data could leak on the stack.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 drivers/crypto/sa2ul.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/sa2ul.c b/drivers/crypto/sa2ul.c
index d865fd4a098cb..f1a7c2cc7a1af 100644
--- a/drivers/crypto/sa2ul.c
+++ b/drivers/crypto/sa2ul.c
@@ -465,7 +465,7 @@ static void sa_prepare_iopads(struct algo_data *data, const u8 *key,
 /* Derive the inverse key used in AES-CBC decryption operation */
 static inline int sa_aes_inv_key(u8 *inv_key, const u8 *key, u16 key_sz)
 {
-	struct crypto_aes_ctx ctx;
+	struct crypto_aes_ctx ctx __cleanup(aes_zeroize_ctx);
 	int key_pos;
 
 	if (aes_expandkey(&ctx, key, key_sz)) {
-- 
2.55.0

[PATCH v4 06/10] crypto: arm/aes-neonbs - clear the crypto_aes_ctx when done

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Clear the crypto_aes_ctx structure via __cleanup(aes_zeroize_ctx) when
we're done with it to avoid that key data could leak on the stack.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 arch/arm/crypto/aes-neonbs-glue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/crypto/aes-neonbs-glue.c b/arch/arm/crypto/aes-neonbs-glue.c
index c49ddafc54f34..f0c8bfd2ac8fc 100644
--- a/arch/arm/crypto/aes-neonbs-glue.c
+++ b/arch/arm/crypto/aes-neonbs-glue.c
@@ -60,7 +60,7 @@ static int aesbs_setkey(struct crypto_skcipher *tfm, const u8 *in_key,
 			unsigned int key_len)
 {
 	struct aesbs_ctx *ctx = crypto_skcipher_ctx(tfm);
-	struct crypto_aes_ctx rk;
+	struct crypto_aes_ctx rk __cleanup(aes_zeroize_ctx);
 	int err;
 
 	err = aes_expandkey(&rk, in_key, key_len);
-- 
2.55.0

[PATCH v4 07/10] crypto: arm64/aes-neonbs - clear the crypto_aes_ctx when done

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Clear the crypto_aes_ctx structure via __cleanup(aes_zeroize_ctx) when
we're done with it to avoid that key data could leak on the stack.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 arch/arm64/crypto/aes-neonbs-glue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/crypto/aes-neonbs-glue.c b/arch/arm64/crypto/aes-neonbs-glue.c
index 5bcbac9798931..c2f3eac0ca661 100644
--- a/arch/arm64/crypto/aes-neonbs-glue.c
+++ b/arch/arm64/crypto/aes-neonbs-glue.c
@@ -247,7 +247,7 @@ static int aesbs_xts_setkey(struct crypto_skcipher *tfm, const u8 *in_key,
 			    unsigned int key_len)
 {
 	struct aesbs_xts_ctx *ctx = crypto_skcipher_ctx(tfm);
-	struct crypto_aes_ctx rk;
+	struct crypto_aes_ctx rk __cleanup(aes_zeroize_ctx);
 	int err;
 
 	err = xts_verify_key(tfm, in_key, key_len);
-- 
2.55.0

[PATCH v4 08/10] crypto: qat - zeroize crypto_aes_ctx with __cleanup(aes_zeroize_ctx)

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

A recent patch by Giovanni Cabiddu already added a memzero_explicit()
for the crypto_aes_ctx in the qat_alg_xts_reverse_key() function,
but since we introduced __cleanup(aes_zeroize_ctx) markers in previous
commits in many spots of the code already, let's use it here now, too,
to have the same code pattern everywhere.

Cc: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 drivers/crypto/intel/qat/qat_common/qat_algs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/intel/qat/qat_common/qat_algs.c b/drivers/crypto/intel/qat/qat_common/qat_algs.c
index 91663805d9e60..cb669fb661625 100644
--- a/drivers/crypto/intel/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/intel/qat/qat_common/qat_algs.c
@@ -388,7 +388,7 @@ static void qat_alg_skcipher_init_enc(struct qat_alg_skcipher_ctx *ctx,
 static void qat_alg_xts_reverse_key(const u8 *key_forward, unsigned int keylen,
 				    u8 *key_reverse)
 {
-	struct crypto_aes_ctx aes_expanded;
+	struct crypto_aes_ctx aes_expanded __cleanup(aes_zeroize_ctx);
 	int nrounds;
 	u8 *key;
 
@@ -405,7 +405,6 @@ static void qat_alg_xts_reverse_key(const u8 *key_forward, unsigned int keylen,
 		memcpy(key_reverse + AES_BLOCK_SIZE, key - AES_BLOCK_SIZE,
 		       AES_BLOCK_SIZE);
 	}
-	memzero_explicit(&aes_expanded, sizeof(aes_expanded));
 }
 
 static void qat_alg_skcipher_init_dec(struct qat_alg_skcipher_ctx *ctx,
-- 
2.55.0

[PATCH v4 09/10] crypto: safexcel - Rework cleanup of sensitive structs in safexcel_aead_setkey

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

The crypto_authenc_keys structure only contains pointers to keys, but not
the key data itself. So explicitly clearing the structure at the end of
safexcel_aead_setkey() is not really necessary, see Eric's recommendation
here:

 https://lore.kernel.org/linux-crypto/20260804185402.GD2904385@google.com/

On the other hand, the crypto_aes_ctx might contain sensitive information,
so this structure should be cleaned up at the end instead. Do this
now via the new __cleanup(aes_zeroize_ctx) marker.

Since __cleanup() and gotos should not be mixed in the same function,
replace the gotos with early return statements, which is fine now that
we dropped the memzero_explicit(&keys, sizeof(keys)) at the end.

Suggested-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .../crypto/inside-secure/safexcel_cipher.c    | 27 ++++++++-----------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index a8349b684693e..c331d81ac8a2d 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -407,17 +407,17 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
 	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
 	struct safexcel_crypto_priv *priv = ctx->base.priv;
 	struct crypto_authenc_keys keys;
-	struct crypto_aes_ctx aes;
-	int err = -EINVAL, i;
+	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
+	int err, i;
 	const char *alg;
 
 	if (unlikely(crypto_authenc_extractkeys(&keys, key, len)))
-		goto badkey;
+		return -EINVAL;
 
 	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) {
 		/* Must have at least space for the nonce here */
 		if (unlikely(keys.enckeylen < CTR_RFC3686_NONCE_SIZE))
-			goto badkey;
+			return -EINVAL;
 		/* last 4 bytes of key are the nonce! */
 		ctx->nonce = *(u32 *)(keys.enckey + keys.enckeylen -
 				      CTR_RFC3686_NONCE_SIZE);
@@ -430,25 +430,25 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
 	case SAFEXCEL_DES:
 		err = verify_aead_des_key(ctfm, keys.enckey, keys.enckeylen);
 		if (unlikely(err))
-			goto badkey;
+			return err;
 		break;
 	case SAFEXCEL_3DES:
 		err = verify_aead_des3_key(ctfm, keys.enckey, keys.enckeylen);
 		if (unlikely(err))
-			goto badkey;
+			return err;
 		break;
 	case SAFEXCEL_AES:
 		err = aes_expandkey(&aes, keys.enckey, keys.enckeylen);
 		if (unlikely(err))
-			goto badkey;
+			return err;
 		break;
 	case SAFEXCEL_SM4:
 		if (unlikely(keys.enckeylen != SM4_KEY_SIZE))
-			goto badkey;
+			return -EINVAL;
 		break;
 	default:
 		dev_err(priv->dev, "aead: unsupported cipher algorithm\n");
-		goto badkey;
+		return -EINVAL;
 	}
 
 	if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma) {
@@ -486,24 +486,19 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
 		break;
 	default:
 		dev_err(priv->dev, "aead: unsupported hash algorithm\n");
-		goto badkey;
+		return -EINVAL;
 	}
 
 	if (safexcel_hmac_setkey(&ctx->base, keys.authkey, keys.authkeylen,
 				 alg, ctx->state_sz))
-		goto badkey;
+		return -EINVAL;
 
 	/* Now copy the keys into the context */
 	for (i = 0; i < keys.enckeylen / sizeof(u32); i++)
 		ctx->key[i] = cpu_to_le32(((u32 *)keys.enckey)[i]);
 	ctx->key_len = keys.enckeylen;
 
-	memzero_explicit(&keys, sizeof(keys));
 	return 0;
-
-badkey:
-	memzero_explicit(&keys, sizeof(keys));
-	return err;
 }
 
 static int safexcel_context_control(struct safexcel_cipher_ctx *ctx,
-- 
2.55.0

[PATCH v4 10/10] crypto: safexcel - zeroize crypto_aes_ctx with __cleanup(aes_zeroize_ctx)

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

The code clears the crypto_aes_ctx in most cases already with
memzero_explicit(), but safexcel_skcipher_aesxts_setkey() runs
aes_expandkey() twice, and in case the second call fails, the
context from the first call is leaked.

To fix this issue and to avoid future similar problems, let's use
the new __cleanup(aes_zeroize_ctx) mechanism to make sure that we
always clear the crypto_aes_ctx in all cases.

Acked-by: Antoine Tenart <atenart@kernel.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 drivers/crypto/inside-secure/safexcel_cipher.c | 13 ++++---------
 drivers/crypto/inside-secure/safexcel_hash.c   |  3 +--
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index c331d81ac8a2d..54ab08aea7536 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -375,7 +375,7 @@ static int safexcel_skcipher_aes_setkey(struct crypto_skcipher *ctfm,
 	struct crypto_tfm *tfm = crypto_skcipher_tfm(ctfm);
 	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
 	struct safexcel_crypto_priv *priv = ctx->base.priv;
-	struct crypto_aes_ctx aes;
+	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
 	int ret, i;
 
 	ret = aes_expandkey(&aes, key, len);
@@ -396,7 +396,6 @@ static int safexcel_skcipher_aes_setkey(struct crypto_skcipher *ctfm,
 
 	ctx->key_len = len;
 
-	memzero_explicit(&aes, sizeof(aes));
 	return 0;
 }
 
@@ -1357,7 +1356,7 @@ static int safexcel_skcipher_aesctr_setkey(struct crypto_skcipher *ctfm,
 	struct crypto_tfm *tfm = crypto_skcipher_tfm(ctfm);
 	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
 	struct safexcel_crypto_priv *priv = ctx->base.priv;
-	struct crypto_aes_ctx aes;
+	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
 	int ret, i;
 	unsigned int keylen;
 
@@ -1383,7 +1382,6 @@ static int safexcel_skcipher_aesctr_setkey(struct crypto_skcipher *ctfm,
 
 	ctx->key_len = keylen;
 
-	memzero_explicit(&aes, sizeof(aes));
 	return 0;
 }
 
@@ -2537,7 +2535,7 @@ static int safexcel_skcipher_aesxts_setkey(struct crypto_skcipher *ctfm,
 	struct crypto_tfm *tfm = crypto_skcipher_tfm(ctfm);
 	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
 	struct safexcel_crypto_priv *priv = ctx->base.priv;
-	struct crypto_aes_ctx aes;
+	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
 	int ret, i;
 	unsigned int keylen;
 
@@ -2585,7 +2583,6 @@ static int safexcel_skcipher_aesxts_setkey(struct crypto_skcipher *ctfm,
 
 	ctx->key_len = keylen << 1;
 
-	memzero_explicit(&aes, sizeof(aes));
 	return 0;
 }
 
@@ -2751,12 +2748,11 @@ static int safexcel_aead_ccm_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_crypto_priv *priv = ctx->base.priv;
-	struct crypto_aes_ctx aes;
+	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
 	int ret, i;
 
 	ret = aes_expandkey(&aes, key, len);
 	if (ret) {
-		memzero_explicit(&aes, sizeof(aes));
 		return ret;
 	}
 
@@ -2785,7 +2781,6 @@ static int safexcel_aead_ccm_setkey(struct crypto_aead *ctfm, const u8 *key,
 	else
 		ctx->hash_alg = CONTEXT_CONTROL_CRYPTO_ALG_XCBC128;
 
-	memzero_explicit(&aes, sizeof(aes));
 	return 0;
 }
 
diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
index 3402e570d045c..20c17eb09495e 100644
--- a/drivers/crypto/inside-secure/safexcel_hash.c
+++ b/drivers/crypto/inside-secure/safexcel_hash.c
@@ -1905,7 +1905,7 @@ static int safexcel_cbcmac_setkey(struct crypto_ahash *tfm, const u8 *key,
 				 unsigned int len)
 {
 	struct safexcel_ahash_ctx *ctx = crypto_tfm_ctx(crypto_ahash_tfm(tfm));
-	struct crypto_aes_ctx aes;
+	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
 	int ret, i;
 
 	ret = aes_expandkey(&aes, key, len);
@@ -1928,7 +1928,6 @@ static int safexcel_cbcmac_setkey(struct crypto_ahash *tfm, const u8 *key,
 	}
 	ctx->cbcmac  = true;
 
-	memzero_explicit(&aes, sizeof(aes));
 	return 0;
 }
 
-- 
2.55.0

Re: [PATCH v4 09/10] crypto: safexcel - Rework cleanup of sensitive structs in safexcel_aead_setkey

[Antoine Tenart]

On Fri, Aug 07, 2026 at 09:06:36AM +0200, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> The crypto_authenc_keys structure only contains pointers to keys, but not
> the key data itself. So explicitly clearing the structure at the end of
> safexcel_aead_setkey() is not really necessary, see Eric's recommendation
> here:
> 
>  https://lore.kernel.org/linux-crypto/20260804185402.GD2904385@google.com/
> 
> On the other hand, the crypto_aes_ctx might contain sensitive information,
> so this structure should be cleaned up at the end instead. Do this
> now via the new __cleanup(aes_zeroize_ctx) marker.
> 
> Since __cleanup() and gotos should not be mixed in the same function,
> replace the gotos with early return statements, which is fine now that
> we dropped the memzero_explicit(&keys, sizeof(keys)) at the end.
> 
> Suggested-by: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Antoine Tenart <atenart@kernel.org>

> ---
>  .../crypto/inside-secure/safexcel_cipher.c    | 27 ++++++++-----------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
> index a8349b684693e..c331d81ac8a2d 100644
> --- a/drivers/crypto/inside-secure/safexcel_cipher.c
> +++ b/drivers/crypto/inside-secure/safexcel_cipher.c
> @@ -407,17 +407,17 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
>  	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
>  	struct safexcel_crypto_priv *priv = ctx->base.priv;
>  	struct crypto_authenc_keys keys;
> -	struct crypto_aes_ctx aes;
> -	int err = -EINVAL, i;
> +	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
> +	int err, i;
>  	const char *alg;
>  
>  	if (unlikely(crypto_authenc_extractkeys(&keys, key, len)))
> -		goto badkey;
> +		return -EINVAL;
>  
>  	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) {
>  		/* Must have at least space for the nonce here */
>  		if (unlikely(keys.enckeylen < CTR_RFC3686_NONCE_SIZE))
> -			goto badkey;
> +			return -EINVAL;
>  		/* last 4 bytes of key are the nonce! */
>  		ctx->nonce = *(u32 *)(keys.enckey + keys.enckeylen -
>  				      CTR_RFC3686_NONCE_SIZE);
> @@ -430,25 +430,25 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
>  	case SAFEXCEL_DES:
>  		err = verify_aead_des_key(ctfm, keys.enckey, keys.enckeylen);
>  		if (unlikely(err))
> -			goto badkey;
> +			return err;
>  		break;
>  	case SAFEXCEL_3DES:
>  		err = verify_aead_des3_key(ctfm, keys.enckey, keys.enckeylen);
>  		if (unlikely(err))
> -			goto badkey;
> +			return err;
>  		break;
>  	case SAFEXCEL_AES:
>  		err = aes_expandkey(&aes, keys.enckey, keys.enckeylen);
>  		if (unlikely(err))
> -			goto badkey;
> +			return err;
>  		break;
>  	case SAFEXCEL_SM4:
>  		if (unlikely(keys.enckeylen != SM4_KEY_SIZE))
> -			goto badkey;
> +			return -EINVAL;
>  		break;
>  	default:
>  		dev_err(priv->dev, "aead: unsupported cipher algorithm\n");
> -		goto badkey;
> +		return -EINVAL;
>  	}
>  
>  	if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma) {
> @@ -486,24 +486,19 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
>  		break;
>  	default:
>  		dev_err(priv->dev, "aead: unsupported hash algorithm\n");
> -		goto badkey;
> +		return -EINVAL;
>  	}
>  
>  	if (safexcel_hmac_setkey(&ctx->base, keys.authkey, keys.authkeylen,
>  				 alg, ctx->state_sz))
> -		goto badkey;
> +		return -EINVAL;
>  
>  	/* Now copy the keys into the context */
>  	for (i = 0; i < keys.enckeylen / sizeof(u32); i++)
>  		ctx->key[i] = cpu_to_le32(((u32 *)keys.enckey)[i]);
>  	ctx->key_len = keys.enckeylen;
>  
> -	memzero_explicit(&keys, sizeof(keys));
>  	return 0;
> -
> -badkey:
> -	memzero_explicit(&keys, sizeof(keys));
> -	return err;
>  }
>  
>  static int safexcel_context_control(struct safexcel_cipher_ctx *ctx,
> -- 
> 2.55.0
> 

Re: [PATCH v4 09/10] crypto: safexcel - Rework cleanup of sensitive structs in safexcel_aead_setkey

[Antoine Tenart]

On Fri, Aug 07, 2026 at 09:40:58AM +0200, Antoine Tenart wrote:
> On Fri, Aug 07, 2026 at 09:06:36AM +0200, Thomas Huth wrote:
> > From: Thomas Huth <thuth@redhat.com>
> > 
> > The crypto_authenc_keys structure only contains pointers to keys, but not
> > the key data itself. So explicitly clearing the structure at the end of
> > safexcel_aead_setkey() is not really necessary, see Eric's recommendation
> > here:
> > 
> >  https://lore.kernel.org/linux-crypto/20260804185402.GD2904385@google.com/
> > 
> > On the other hand, the crypto_aes_ctx might contain sensitive information,
> > so this structure should be cleaned up at the end instead. Do this
> > now via the new __cleanup(aes_zeroize_ctx) marker.
> > 
> > Since __cleanup() and gotos should not be mixed in the same function,
> > replace the gotos with early return statements, which is fine now that
> > we dropped the memzero_explicit(&keys, sizeof(keys)) at the end.
> > 
> > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Reviewed-by: Antoine Tenart <atenart@kernel.org>

Actually I think we could do better here:

The AES key is expanded in safexcel_aead_setkey using aes_expandkey but
the result (the aes variable) is discarded and never used. This seems a
waste of CPU cycles for doing a validity check on the input key, which
can be replaced by a simple aes_check_keylen.

The same applies to the EIP93 patch for both chunks.

> > ---
> >  .../crypto/inside-secure/safexcel_cipher.c    | 27 ++++++++-----------
> >  1 file changed, 11 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
> > index a8349b684693e..c331d81ac8a2d 100644
> > --- a/drivers/crypto/inside-secure/safexcel_cipher.c
> > +++ b/drivers/crypto/inside-secure/safexcel_cipher.c
> > @@ -407,17 +407,17 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
> >  	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
> >  	struct safexcel_crypto_priv *priv = ctx->base.priv;
> >  	struct crypto_authenc_keys keys;
> > -	struct crypto_aes_ctx aes;
> > -	int err = -EINVAL, i;
> > +	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
> > +	int err, i;
> >  	const char *alg;
> >  
> >  	if (unlikely(crypto_authenc_extractkeys(&keys, key, len)))
> > -		goto badkey;
> > +		return -EINVAL;
> >  
> >  	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) {
> >  		/* Must have at least space for the nonce here */
> >  		if (unlikely(keys.enckeylen < CTR_RFC3686_NONCE_SIZE))
> > -			goto badkey;
> > +			return -EINVAL;
> >  		/* last 4 bytes of key are the nonce! */
> >  		ctx->nonce = *(u32 *)(keys.enckey + keys.enckeylen -
> >  				      CTR_RFC3686_NONCE_SIZE);
> > @@ -430,25 +430,25 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
> >  	case SAFEXCEL_DES:
> >  		err = verify_aead_des_key(ctfm, keys.enckey, keys.enckeylen);
> >  		if (unlikely(err))
> > -			goto badkey;
> > +			return err;
> >  		break;
> >  	case SAFEXCEL_3DES:
> >  		err = verify_aead_des3_key(ctfm, keys.enckey, keys.enckeylen);
> >  		if (unlikely(err))
> > -			goto badkey;
> > +			return err;
> >  		break;
> >  	case SAFEXCEL_AES:
> >  		err = aes_expandkey(&aes, keys.enckey, keys.enckeylen);
> >  		if (unlikely(err))
> > -			goto badkey;
> > +			return err;
> >  		break;
> >  	case SAFEXCEL_SM4:
> >  		if (unlikely(keys.enckeylen != SM4_KEY_SIZE))
> > -			goto badkey;
> > +			return -EINVAL;
> >  		break;
> >  	default:
> >  		dev_err(priv->dev, "aead: unsupported cipher algorithm\n");
> > -		goto badkey;
> > +		return -EINVAL;
> >  	}
> >  
> >  	if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma) {
> > @@ -486,24 +486,19 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
> >  		break;
> >  	default:
> >  		dev_err(priv->dev, "aead: unsupported hash algorithm\n");
> > -		goto badkey;
> > +		return -EINVAL;
> >  	}
> >  
> >  	if (safexcel_hmac_setkey(&ctx->base, keys.authkey, keys.authkeylen,
> >  				 alg, ctx->state_sz))
> > -		goto badkey;
> > +		return -EINVAL;
> >  
> >  	/* Now copy the keys into the context */
> >  	for (i = 0; i < keys.enckeylen / sizeof(u32); i++)
> >  		ctx->key[i] = cpu_to_le32(((u32 *)keys.enckey)[i]);
> >  	ctx->key_len = keys.enckeylen;
> >  
> > -	memzero_explicit(&keys, sizeof(keys));
> >  	return 0;
> > -
> > -badkey:
> > -	memzero_explicit(&keys, sizeof(keys));
> > -	return err;
> >  }
> >  
> >  static int safexcel_context_control(struct safexcel_cipher_ctx *ctx,
> > -- 
> > 2.55.0
> > 
>