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

[PATCH v3 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.

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 v3 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 a driver). 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.
+ */
+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 v3 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 v3 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 v3 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 v3 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 v3 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 v3 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 v3 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 v3 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.

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..e94686490bb27 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 err;
 		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 v3 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.

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 e94686490bb27..50e8792b399de 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 v3 00/10] crypto: Provide a function for zeroizing crypto_aes_ctx

[Eric Biggers]

On Wed, Aug 05, 2026 at 01:57:38PM +0200, Thomas Huth wrote:
> 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.
> 
> 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

I'll assume that Herbert will take this series via cryptodev/master,
since it mostly deals with drivers/crypto/.  And the new library APIs
don't use 'struct crypto_aes_ctx'.  It's just going to stay around for a
while to serve drivers that call aes_expandkey().

I'll take the AES-CMAC one.

- Eric

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

[Antoine Tenart]

On Wed, Aug 05, 2026 at 01:57:47PM +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.
> 
> 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.

Looking at other crypto drivers it seems zeroing the key pointers was
explicitly added (sometimes later) and my impression is a good chunk of
the users are zeroing it. I don't know whether removing that is fine or
not, my limited understanding is that provides in-depth defense against
leaking were the key reside in memory. Would love to see an explicit
statement from someone with that knowledge.

(On the other hand mixing gotos and __cleanup is not advised but is that
an issue here? Or if zeroing crypto_authenc_keys is actually important
can we use __cleanup too?).

> --- 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;

> @@ -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 err;

'err' is uninitialized here. You can use '-EINVAL' instead.

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

[Antoine Tenart]

On Wed, Aug 05, 2026 at 01:57:48PM +0200, Thomas Huth wrote:
> 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.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

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

> ---
>  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 e94686490bb27..50e8792b399de 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 v3 09/10] crypto: safexcel - Rework cleanup of sensitive structs in safexcel_aead_setkey

[Thomas Huth]

On 06/08/2026 10.10, Antoine Tenart wrote:
> On Wed, Aug 05, 2026 at 01:57:47PM +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.
>>
>> 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.
> 
> Looking at other crypto drivers it seems zeroing the key pointers was
> explicitly added (sometimes later) and my impression is a good chunk of
> the users are zeroing it. I don't know whether removing that is fine or
> not, my limited understanding is that provides in-depth defense against
> leaking were the key reside in memory. Would love to see an explicit
> statement from someone with that knowledge.

It has been suggested by Eric here (unless I got him wrong):

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

... I should have maybe added that link to this patch description ...

> (On the other hand mixing gotos and __cleanup is not advised but is that
> an issue here? Or if zeroing crypto_authenc_keys is actually important
> can we use __cleanup too?).
> 
>> --- 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;
> 
>> @@ -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 err;
> 
> 'err' is uninitialized here. You can use '-EINVAL' instead.
Oops, good catch, thanks! I will fix it in the next version (assuming that 
removing the memzero_explicit is ok and we'll keep this patch...)

  Thomas

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

[Antoine Tenart]

On Thu, Aug 06, 2026 at 10:18:12AM +0200, Thomas Huth wrote:
> On 06/08/2026 10.10, Antoine Tenart wrote:
> > On Wed, Aug 05, 2026 at 01:57:47PM +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.
> >>
> >> 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.
> > 
> > Looking at other crypto drivers it seems zeroing the key pointers was
> > explicitly added (sometimes later) and my impression is a good chunk of
> > the users are zeroing it. I don't know whether removing that is fine or
> > not, my limited understanding is that provides in-depth defense against
> > leaking were the key reside in memory. Would love to see an explicit
> > statement from someone with that knowledge.
> 
> It has been suggested by Eric here (unless I got him wrong):
> 
>   https://lore.kernel.org/linux-crypto/20260804185402.GD2904385@google.com/
> 
> ... I should have maybe added that link to this patch description ...

Thanks for the link! Feel free to add it or not in the next revision.

> > (On the other hand mixing gotos and __cleanup is not advised but is that
> > an issue here? Or if zeroing crypto_authenc_keys is actually important
> > can we use __cleanup too?).
> > 
> >> --- 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;
> > 
> >> @@ -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 err;
> > 
> > 'err' is uninitialized here. You can use '-EINVAL' instead.
> Oops, good catch, thanks! I will fix it in the next version (assuming that 
> removing the memzero_explicit is ok and we'll keep this patch...)