crypto: use kfree_sensitive()

PATCH In review Denis Efremov 5 participants 14 messages Aug 27, 2020 – Sep 4, 2020

Subsystems (5)

Crypto API Arm/Allwinner sunXi SoC support Inside secure crypto driver Allwinner crypto drivers +1 more subsystems

lore

Share & tools

lore

Denis Efremov

author

2020-08-27 06:44 UTC #0 lore raw

v2 cover crypto: use kfree_sensitive()

kfree_sensitive() is introduced in commit 453431a54934 ("mm, treewide: rename kzfree() to kfree_sensitive()") and uses memzero_explicit() internally. Thus, we can switch to this API instead of open-coding memzero_explicit() && kfree().

Changes in v2:
- if (op->len) check removed

Denis Efremov (4):
crypto: inside-secure - use kfree_sensitive()
crypto: amlogic - use kfree_sensitive()
crypto: sun8i-ce - use kfree_sensitive()
crypto: sun8i-ss - use kfree_sensitive()

 .../crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c   | 15 +++------------
 .../crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c   | 15 +++------------
 drivers/crypto/amlogic/amlogic-gxl-cipher.c       | 10 ++--------
 drivers/crypto/inside-secure/safexcel_hash.c      |  3 +--
 4 files changed, 9 insertions(+), 34 deletions(-)

Signature · 1 line

2.26.2

Denis Efremov

author

2020-08-27 06:44 UTC #1 lore raw

PATCH v2 1/4 crypto: inside-secure - use kfree_sensitive()

Use kfree_sensitive() instead of open-coding it.

Signed-off-by Denis Efremov <[email protected]>

---
 drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/crypto/inside-secure/safexcel_hash.c +1 -2

index 16a467969d8e..5ffdc1cd5847 100644
@@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq,
 		}
 
 		/* Avoid leaking */
-		memzero_explicit(keydup, keylen);
-		kfree(keydup);
+		kfree_sensitive(keydup);
 
 		if (ret)
 			return ret;

Signature · 1 line

2.26.2

Denis Efremov

author

2020-08-27 06:45 UTC #7 lore raw

PATCH v2 2/4 crypto: amlogic - use kfree_sensitive()

In reply to Denis Efremov

Use kfree_sensitive() instead of open-coding it.

Signed-off-by Denis Efremov <[email protected]>

---
 drivers/crypto/amlogic/amlogic-gxl-cipher.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

drivers/crypto/amlogic/amlogic-gxl-cipher.c +2 -8

index d93210726697..ee5998af2fe8 100644
@@ -340,10 +340,7 @@ void meson_cipher_exit(struct crypto_tfm *tfm)
 {
 	struct meson_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
 
-	if (op->key) {
-		memzero_explicit(op->key, op->keylen);
-		kfree(op->key);
-	}
+	kfree_sensitive(op->key);
 	crypto_free_skcipher(op->fallback_tfm);
 }

@@ -367,10 +364,7 @@ int meson_aes_setkey(struct crypto_skcipher *tfm, const u8 *key,
 		dev_dbg(mc->dev, "ERROR: Invalid keylen %u\n", keylen);
 		return -EINVAL;
 	}
-	if (op->key) {
-		memzero_explicit(op->key, op->keylen);
-		kfree(op->key);
-	}
+	kfree_sensitive(op->key);
 	op->keylen = keylen;
 	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
 	if (!op->key)

Signature · 1 line

2.26.2

Denis Efremov

author

2020-08-27 06:45 UTC #9 lore raw

PATCH v2 3/4 crypto: sun8i-ce - use kfree_sensitive()

In reply to Denis Efremov

Use kfree_sensitive() instead of open-coding it.

Signed-off-by Denis Efremov <[email protected]>

---
 .../crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c   | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c +3 ~~-12~~

index b4d5fea27d20..f996dc3d7dcc 100644
@@ -366,10 +366,7 @@ void sun8i_ce_cipher_exit(struct crypto_tfm *tfm)
 {
 	struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
 
-	if (op->key) {
-		memzero_explicit(op->key, op->keylen);
-		kfree(op->key);
-	}
+	kfree_sensitive(op->key);
 	crypto_free_skcipher(op->fallback_tfm);
 	pm_runtime_put_sync_suspend(op->ce->dev);
 }

@@ -391,10 +388,7 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm, const u8 *key,
 		dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
 		return -EINVAL;
 	}
-	if (op->key) {
-		memzero_explicit(op->key, op->keylen);
-		kfree(op->key);
-	}
+	kfree_sensitive(op->key);
 	op->keylen = keylen;
 	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
 	if (!op->key)

@@ -416,10 +410,7 @@ int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm, const u8 *key,
 	if (err)
 		return err;
 
-	if (op->key) {
-		memzero_explicit(op->key, op->keylen);
-		kfree(op->key);
-	}
+	kfree_sensitive(op->key);
 	op->keylen = keylen;
 	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
 	if (!op->key)

Signature · 1 line

2.26.2

Denis Efremov

author

2020-08-27 06:45 UTC #11 lore raw

PATCH v2 4/4 crypto: sun8i-ss - use kfree_sensitive()

In reply to Denis Efremov

Use kfree_sensitive() instead of open-coding it.

Signed-off-by Denis Efremov <[email protected]>

---
 .../crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c   | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c +3 ~~-12~~

index 7b39b4495571..deb8b39a86db 100644
@@ -368,10 +368,7 @@ void sun8i_ss_cipher_exit(struct crypto_tfm *tfm)
 {
 	struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
 
-	if (op->key) {
-		memzero_explicit(op->key, op->keylen);
-		kfree(op->key);
-	}
+	kfree_sensitive(op->key);
 	crypto_free_skcipher(op->fallback_tfm);
 	pm_runtime_put_sync(op->ss->dev);
 }

@@ -393,10 +390,7 @@ int sun8i_ss_aes_setkey(struct crypto_skcipher *tfm, const u8 *key,
 		dev_dbg(ss->dev, "ERROR: Invalid keylen %u\n", keylen);
 		return -EINVAL;
 	}
-	if (op->key) {
-		memzero_explicit(op->key, op->keylen);
-		kfree(op->key);
-	}
+	kfree_sensitive(op->key);
 	op->keylen = keylen;
 	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
 	if (!op->key)

@@ -419,10 +413,7 @@ int sun8i_ss_des3_setkey(struct crypto_skcipher *tfm, const u8 *key,
 		return -EINVAL;
 	}
 
-	if (op->key) {
-		memzero_explicit(op->key, op->keylen);
-		kfree(op->key);
-	}
+	kfree_sensitive(op->key);
 	op->keylen = keylen;
 	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
 	if (!op->key)

Signature · 1 line

2.26.2

Corentin Labbe

maintainer Acked-by Tested-by on v2 3/4

2020-08-27 14:41 UTC #10 lore raw

Re: [PATCH v2 3/4] crypto: sun8i-ce - use kfree_sensitive()

In reply to Denis Efremov

Quoted 7 lines · Denis Efremov

Use kfree_sensitive() instead of open-coding it.
Signed-off-by Denis Efremov <[email protected]>
---
 .../crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c   | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

Acked-by Corentin Labbe <[email protected]>

Tested-by Corentin Labbe <[email protected]>

Thanks

Corentin Labbe

maintainer Acked-by Tested-by on v2 4/4

2020-08-27 14:41 UTC #12 lore raw

Re: [PATCH v2 4/4] crypto: sun8i-ss - use kfree_sensitive()

In reply to Denis Efremov

Quoted 7 lines · Denis Efremov

Use kfree_sensitive() instead of open-coding it.
Signed-off-by Denis Efremov <[email protected]>
---
 .../crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c   | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

Acked-by Corentin Labbe <[email protected]>

Tested-by Corentin Labbe <[email protected]>

Thanks

Corentin Labbe

maintainer Tested-by Acked-by on v2 2/4

2020-08-27 14:50 UTC #8 lore raw

Re: [PATCH v2 2/4] crypto: amlogic - use kfree_sensitive()

In reply to Denis Efremov

Quoted 7 lines · Denis Efremov

Use kfree_sensitive() instead of open-coding it.
Signed-off-by Denis Efremov <[email protected]>
---
 drivers/crypto/amlogic/amlogic-gxl-cipher.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

For the whole serie you didnt use getmaintainers, so nor sunxi and amlogic maintainers where CC. And my baylibre address which is the address for this driver.

Anyway, for this case the patch is trivial enough.

Tested-by Corentin Labbe <[email protected]>

Acked-by Corentin Labbe <[email protected]>

Regards

Corentin Labbe

maintainer

2020-08-27 14:52 UTC #2 lore raw

Re: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()

In reply to Denis Efremov

Quoted 24 lines · Denis Efremov

Use kfree_sensitive() instead of open-coding it.

Signed-off-by Denis Efremov <[email protected]>

---
 drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/crypto/inside-secure/safexcel_hash.c +1 -2

index 16a467969d8e..5ffdc1cd5847 100644
@@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq,
 		}
 
 		/* Avoid leaking */
-		memzero_explicit(keydup, keylen);
-		kfree(keydup);
+		kfree_sensitive(keydup);
 
 		if (ret)
 			return ret;

Signature · 1 line

2.26.2

The maintainer of this driver was not TO/CC.

I Add him.

Regards

Antoine Tenart

Acked-by on v2 1/4

2020-09-02 09:02 UTC #3 lore raw

Re: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()

In reply to Denis Efremov

Hello Denis,

Quoting Denis Efremov (2020-08-27 08:43:59)

Quoted 3 lines

Use kfree_sensitive() instead of open-coding it.
Signed-off-by Denis Efremov <[email protected]>

Acked-by Antoine Tenart <[email protected]>

Thanks!
Antoine

Quoted 21 lines

---
 drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/crypto/inside-secure/safexcel_hash.c +1 -2

index 16a467969d8e..5ffdc1cd5847 100644
@@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq,
                }
 
                /* Avoid leaking */
-               memzero_explicit(keydup, keylen);
-               kfree(keydup);
+               kfree_sensitive(keydup);
 
                if (ret)
                        return ret;

Signature · 1 line

2.26.2

Signature · 3 lines

Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Van Leeuwen, Pascal

2020-09-02 13:12 UTC #4 lore raw

RE: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()

In reply to Denis Efremov

Quoted 21 lines

-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Denis Efremov
Sent: Thursday, August 27, 2020 8:44 AM
To: [email protected]
Cc: Denis Efremov <[email protected]>; Corentin Labbe <[email protected]>; Herbert Xu <[email protected]>; [email protected]
Subject: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()
<<< External Email >>>
Use kfree_sensitive() instead of open-coding it.
Signed-off-by Denis Efremov <[email protected]>
---
 drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/crypto/inside-secure/safexcel_hash.c +0 -0
index 16a467969d8e..5ffdc1cd5847 100644

drivers/crypto/inside-secure/safexcel_hash.c static int safexcel_hmac_init_pad(struct ahash_request *areq, · line 1086

Quoted 7 lines

drivers/crypto/inside-secure/safexcel_hash.c +1 -2

@@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq,
 }
 
 /* Avoid leaking */
-memzero_explicit(keydup, keylen);
-kfree(keydup);
+kfree_sensitive(keydup);

I'm not sure here ... I verified it does not break the driver (not a big surprise), but ...

memzero_explicit guarantees that it will not get optimized away and the keydata _always_ gets overwritten. Does kfree_sensitive also come with such a guarantee? I could not find a hard statement on that in its documentation. Although the "sensitive" part surely suggests it.

Additionally, this remark is made in the documentation for kfree_sensitive: "this function zeroes the whole allocated buffer which can be a good deal bigger than the requested buffer size passed to kmalloc(). So be careful when using this function in performance sensitive code"

While the memzero_explicit does not zeroize anything beyond keylen.
Which is all you really need here, so why would you want to zeroize potentially a lot more? In any case the two are not fully equivalent.

Any opinions?

Quoted 4 lines

drivers/crypto/inside-secure/safexcel_hash.c +0 -1
 if (ret)
 return ret;
--
2.26.2

Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security Rambus ROTW Holding BV
+31-73 6581953

Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus. Please be so kind to update your e-mail address book with my new e-mail address.

** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **

Rambus Inc.<http://www.rambus.com>

Herbert Xu

maintainer

2020-09-04 08:28 UTC #13 lore raw

In reply to Denis Efremov

Quoted 14 lines · Denis Efremov

kfree_sensitive() is introduced in commit 453431a54934 ("mm, treewide: rename kzfree() to kfree_sensitive()") and uses memzero_explicit() internally. Thus, we can switch to this API instead of open-coding memzero_explicit() && kfree().
Changes in v2:
- if (op->len) check removed
Denis Efremov (4):
crypto: inside-secure - use kfree_sensitive()
crypto: amlogic - use kfree_sensitive()
crypto: sun8i-ce - use kfree_sensitive()
crypto: sun8i-ss - use kfree_sensitive()

Quoted 5 lines

 .../crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c   | 15 +++------------
 .../crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c   | 15 +++------------
 drivers/crypto/amlogic/amlogic-gxl-cipher.c       | 10 ++--------
 drivers/crypto/inside-secure/safexcel_hash.c      |  3 +--
 4 files changed, 9 insertions(+), 34 deletions(-)

All 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

Denis Efremov

author

2020-09-04 08:55 UTC #5 lore raw

Re: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()

In reply to Van Leeuwen, Pascal

Hi,

Quoted 30 lines · Pascal

Quoted 21 lines
-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Denis Efremov
Sent: Thursday, August 27, 2020 8:44 AM
To: [email protected]
Cc: Denis Efremov <[email protected]>; Corentin Labbe <[email protected]>; Herbert Xu <[email protected]>; [email protected]
Subject: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()
<<< External Email >>>
Use kfree_sensitive() instead of open-coding it.
Signed-off-by Denis Efremov <[email protected]>
---
 drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/crypto/inside-secure/safexcel_hash.c +0 -0
index 16a467969d8e..5ffdc1cd5847 100644
drivers/crypto/inside-secure/safexcel_hash.c static int safexcel_hmac_init_pad(struct ahash_request *areq, · line 1086
Quoted 7 lines
drivers/crypto/inside-secure/safexcel_hash.c +1 -2
@@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq,
 }
 
 /* Avoid leaking */
-memzero_explicit(keydup, keylen);
-kfree(keydup);
+kfree_sensitive(keydup);
 
I'm not sure here ... I verified it does not break the driver (not a big surprise), but ...

Quoted 4 lines

memzero_explicit guarantees that it will not get optimized away and the keydata _always_ gets overwritten. Does kfree_sensitive also come with such a guarantee? I could not find a hard statement on that in its documentation. Although the "sensitive" part surely suggests it.

kfree_sensitive() uses memzero_explicit() internally.

Quoted 8 lines

Additionally, this remark is made in the documentation for kfree_sensitive: "this function zeroes the whole allocated buffer which can be a good deal bigger than the requested buffer size passed to kmalloc(). So be careful when using this function in performance sensitive code"
While the memzero_explicit does not zeroize anything beyond keylen.
Which is all you really need here, so why would you want to zeroize potentially a lot more? In any case the two are not fully equivalent.

There are a number of predefined allocation sizes (power of 2) for faster alloc,
i.e. https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L349
and it looks like that keys we free in this patches are in bounds of these sizes.
As far as I understand, if a key is not a power of 2 len, the buffer will be zeroed to the closest power of 2 size. For small sizes like these, performance difference should be unnoticeable because of cache lines and how arch-optimized memzero() works. Key freeing doesn't look like a frequent event.

Thanks,
Denis

Van Leeuwen, Pascal

Tested-by

2020-09-04 09:44 UTC #6 lore raw

RE: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()

Quoted 41 lines

-----Original Message-----
From: Denis Efremov <[email protected]>
Sent: Friday, September 4, 2020 10:55 AM
To: Van Leeuwen, Pascal <[email protected]>; [email protected]
Cc: Corentin Labbe <[email protected]>; Herbert Xu <[email protected]>; [email protected]
Subject: Re: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()
<<< External Email >>>
Hi,
Quoted 30 lines · Pascal
Quoted 21 lines
-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Denis Efremov
Sent: Thursday, August 27, 2020 8:44 AM
To: [email protected]
Cc: Denis Efremov <[email protected]>; Corentin Labbe <[email protected]>; Herbert Xu <[email protected]>; [email protected]
Subject: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()
<<< External Email >>>
Use kfree_sensitive() instead of open-coding it.
Signed-off-by Denis Efremov <[email protected]>
---
 drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/crypto/inside-secure/safexcel_hash.c +0 -0
index 16a467969d8e..5ffdc1cd5847 100644
drivers/crypto/inside-secure/safexcel_hash.c static int safexcel_hmac_init_pad(struct ahash_request *areq, · line 1086
Quoted 7 lines
drivers/crypto/inside-secure/safexcel_hash.c +1 -2
@@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq,
 }
 
 /* Avoid leaking */
-memzero_explicit(keydup, keylen);
-kfree(keydup);
+kfree_sensitive(keydup);
 
I'm not sure here ... I verified it does not break the driver (not a big surprise), but ...

Quoted 7 lines

Quoted 4 lines
memzero_explicit guarantees that it will not get optimized away and the keydata _always_ gets overwritten. Does kfree_sensitive also come with such a guarantee? I could not find a hard statement on that in its documentation. Although the "sensitive" part surely suggests it.
kfree_sensitive() uses memzero_explicit() internally.

Ok. Although formally that's still only _current_ implementation.
But given the function name, I guess it's a fair assumption that the intention is to maintain this behavior going forward.

Quoted 9 lines

Quoted 8 lines
Additionally, this remark is made in the documentation for kfree_sensitive: "this function zeroes the whole allocated buffer which can be a good deal bigger than the requested buffer size passed to kmalloc(). So be careful when using this function in performance sensitive code"
While the memzero_explicit does not zeroize anything beyond keylen.
Which is all you really need here, so why would you want to zeroize potentially a lot more? In any case the two are not fully equivalent.

Quoted 6 lines

There are a number of predefined allocation sizes (power of 2) for faster alloc, i.e. https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L349
and it looks like that keys we free in this patches are in bounds of these sizes. As far as I understand, if a key is not a power of 2 len, the buffer will be zeroed to the closest power of 2 size.

This path is for hash keys that are larger than the hash block size. Potentially, there is no upper bound on the size of such a hash key, so it doesn't need to be in that range hence zeroizing to the next power of 2 boundary could be expensive.
OTOH, I don't expect this path to be frequently used, and the key processing itself already costs a lot of time, so it's probably not that relevant. Never mind.

I guess was more about whether using kfree_sensitive() is a good replacement _in general_. For that, there should be some guaranteed upper bound on how much extra will be zeroized.

Given the above considerations (and after testing this on my hardware):

Tested-by Pascal van Leeuwen <[email protected]>

Quoted 6 lines

For small sizes like these, performance difference should be unnoticeable because
of cache lines and how arch-optimized memzero() works. Key freeing doesn't look like a frequent event.
Thanks,
Denis

Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security Rambus ROTW Holding BV
+31-73 6581953

Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus. Please be so kind to update your e-mail address book with my new e-mail address.

Rambus Inc.<http://www.rambus.com>