crypto: aead,cipher - zeroize key buffer after use
PATCH v2 2/2 crypto: aead,cipher - zeroize key buffer after use
I.G 9.7.B for FIPS 140-3 specifies that variables temporarily holding cryptographic information should be zeroized once they are no longer needed. Accomplish this by using kfree_sensitive for buffers that previously held the private key.
--- crypto/aead.c | 3 +-- crypto/cipher.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
crypto/aead.c +1 -2
@@ -35,8 +35,7 @@ static int setkey_unaligned(struct crypto_aead *tfm, const u8 *key, alignbuffer = (u8 *)ALIGN ((unsigned long )buffer, alignmask +1 );memcpy (alignbuffer, key, keylen); ret =crypto_aead_alg (tfm)->setkey (tfm, alignbuffer, keylen); -memset (alignbuffer,0 , keylen); -kfree (buffer); +kfree_sensitive (buffer);return ret; }
crypto/cipher.c +1 -2
@@ -34,8 +34,7 @@ static int setkey_unaligned(struct crypto_cipher *tfm, const u8 *key, alignbuffer = (u8 *)ALIGN ((unsigned long )buffer, alignmask +1 );memcpy (alignbuffer, key, keylen); ret = cia->cia_setkey (crypto_cipher_tfm (tfm), alignbuffer, keylen); -memset (alignbuffer,0 , keylen); -kfree (buffer); +kfree_sensitive (buffer);return ret; }
Signature · 3 lines
2.40.1 memsets removed and first patch in series dropped.
Quoted 30 lines · Hailey Mothershead
I.G 9.7.B for FIPS 140-3 specifies that variables temporarily holding cryptographic information should be zeroized once they are no longer needed. Accomplish this by using
kfree_sensitivefor buffers that previously held the private key.Signed-off-by Hailey Mothershead <[email protected]>--- crypto/aead.c | 3 +-- crypto/cipher.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)crypto/aead.c +1
-2@@ -35,8 +35,7 @@ static int setkey_unaligned(struct crypto_aead *tfm, const u8 *key, alignbuffer = (u8 *)ALIGN ((unsigned long )buffer, alignmask +1 );memcpy (alignbuffer, key, keylen); ret =crypto_aead_alg (tfm)->setkey (tfm, alignbuffer, keylen); -memset (alignbuffer,0 , keylen); -kfree (buffer); +kfree_sensitive (buffer);return ret; }crypto/cipher.c +0
-0
crypto/cipher.c static int setkey_unaligned(struct crypto_cipher *tfm, const u8 *key, · line 38
Quoted 7 lines
crypto/cipher.c +1
-2@@ -34,8 +34,7 @@ static int setkey_unaligned(struct crypto_cipher *tfm, const u8 *key, alignbuffer = (u8 *)ALIGN ((unsigned long )buffer, alignmask +1 );memcpy (alignbuffer, key, keylen); ret = cia->cia_setkey (crypto_cipher_tfm (tfm), alignbuffer, keylen); -memset (alignbuffer,0 , keylen); -kfree (buffer); +kfree_sensitive (buffer);return ret;
Well, the memset()s that you're removing already did the zeroization. This patch seems worthwhile as a code simplification, but please don't characterize it as a bug fix, because it's not.
- Eric
Re: [PATCH v2 2/2] crypto: aead, cipher - zeroize key buffer after use
Quoted 47 lines · On 4/15/24, 3:50 PM, "Eric Biggers" <[email protected]…
Quoted 30 lines · Hailey Mothershead
I.G 9.7.B for FIPS 140-3 specifies that variables temporarily holding cryptographic information should be zeroized once they are no longer needed. Accomplish this by using
kfree_sensitivefor buffers that previously held the private key.Signed-off-by Hailey Mothershead <[email protected] <mailto:[email protected]>>---
crypto/aead.c| 3 +--crypto/cipher.c| 3 +--2 files changed, 2 insertions(+), 4 deletions(-)crypto/aead.c +1
-2@@ -35,8 +35,7 @@ static int setkey_unaligned(struct crypto_aead *tfm, const u8 *key, alignbuffer = (u8 *)ALIGN ((unsigned long )buffer, alignmask +1 );memcpy (alignbuffer, key, keylen); ret =crypto_aead_alg (tfm)->setkey (tfm, alignbuffer, keylen); -memset (alignbuffer,0 , keylen); -kfree (buffer); +kfree_sensitive (buffer);return ret; }crypto/cipher.c +0
-0crypto/cipher.c static int setkey_unaligned(struct crypto_cipher *tfm, const u8 *key, · line 38
Quoted 7 lines
crypto/cipher.c +1
-2@@ -34,8 +34,7 @@ static int setkey_unaligned(struct crypto_cipher *tfm, const u8 *key, alignbuffer = (u8 *)ALIGN ((unsigned long )buffer, alignmask +1 );memcpy (alignbuffer, key, keylen); ret = cia->cia_setkey (crypto_cipher_tfm (tfm), alignbuffer, keylen); -memset (alignbuffer,0 , keylen); -kfree (buffer); +kfree_sensitive (buffer);return ret;Well, the memset()s that you're removing already did the zeroization. This patch seems worthwhile as a code simplification, but please don't characterize it as a bug fix, because it's not.
Quoted 1 line
- Eric
kfree_sensitive uses memzero_explicit() instead of the memset()s used above. The memzero_explicit header states -
* Note: usually using memset() is just fine (!), but in cases
* where clearing out _local_ data at the end of a scope is
* necessary, memzero_explicit() should be used instead in
* order to prevent the compiler from optimising away zeroing.
It accomplishes this by calling memset() and then adding a barrier. Since FIPS requires this data be zeroed out, and the current memset() and kfree() don't guarantee this, I do not think the commit message is misleading. I can clarify the message with the above information if that is preferred.
Re: [PATCH v2 2/2] crypto: aead, cipher - zeroize key buffer after use
Quoted 57 lines · Hailey
Quoted 47 lines · On 4/15/24, 3:50 PM, "Eric Biggers" <[email protected]…
Quoted 30 lines · Hailey Mothershead
I.G 9.7.B for FIPS 140-3 specifies that variables temporarily holding cryptographic information should be zeroized once they are no longer needed. Accomplish this by using
kfree_sensitivefor buffers that previously held the private key.Signed-off-by Hailey Mothershead <[email protected] <mailto:[email protected]>>---
crypto/aead.c| 3 +--crypto/cipher.c| 3 +--2 files changed, 2 insertions(+), 4 deletions(-)crypto/aead.c +1
-2@@ -35,8 +35,7 @@ static int setkey_unaligned(struct crypto_aead *tfm, const u8 *key, alignbuffer = (u8 *)ALIGN ((unsigned long )buffer, alignmask +1 );memcpy (alignbuffer, key, keylen); ret =crypto_aead_alg (tfm)->setkey (tfm, alignbuffer, keylen); -memset (alignbuffer,0 , keylen); -kfree (buffer); +kfree_sensitive (buffer);return ret; }crypto/cipher.c +0
-0crypto/cipher.c static int setkey_unaligned(struct crypto_cipher *tfm, const u8 *key, · line 38
Quoted 7 lines
crypto/cipher.c +1
-2@@ -34,8 +34,7 @@ static int setkey_unaligned(struct crypto_cipher *tfm, const u8 *key, alignbuffer = (u8 *)ALIGN ((unsigned long )buffer, alignmask +1 );memcpy (alignbuffer, key, keylen); ret = cia->cia_setkey (crypto_cipher_tfm (tfm), alignbuffer, keylen); -memset (alignbuffer,0 , keylen); -kfree (buffer); +kfree_sensitive (buffer);return ret;Well, the memset()s that you're removing already did the zeroization. This patch seems worthwhile as a code simplification, but please don't characterize it as a bug fix, because it's not.
Quoted 1 line
- Eric
kfree_sensitiveusesmemzero_explicit()instead of the memset()s used above. Thememzero_explicitheader states -* Note: usually using
memset()is just fine (!), but in cases
* where clearing out _local_ data at the end of a scope is
* necessary,memzero_explicit()should be used instead in
* order to prevent the compiler from optimising away zeroing.
Quoted 6 lines
It accomplishes this by calling
memset()and then adding a barrier. Since FIPS requires this data be zeroed out, and the currentmemset()andkfree()don't guarantee this, I do not think the commit message is misleading. I can clarify the message with the above information if that is preferred.
It's heap data, not local data. So no, memzero_explicit() isn't actually needed here. It would convey the intent better, but it's not actually needed.
- Eric
Herbert Xu
maintainerQuoted 10 lines · Hailey Mothershead
I.G 9.7.B for FIPS 140-3 specifies that variables temporarily holding cryptographic information should be zeroized once they are no longer needed. Accomplish this by using
kfree_sensitivefor buffers that previously held the private key.Signed-off-by Hailey Mothershead <[email protected]>--- crypto/aead.c | 3 +-- crypto/cipher.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
Patch applied. Thanks.
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt