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(-)
--
2.26.2
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(-)
diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
index 16a467969d8e..5ffdc1cd5847 100644
--- a/drivers/crypto/inside-secure/safexcel_hash.c
+++ b/drivers/crypto/inside-secure/safexcel_hash.c
@@ -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;
--
2.26.2
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(-)
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index b4d5fea27d20..f996dc3d7dcc 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -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)
--
2.26.2
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(-)
diff --git a/drivers/crypto/amlogic/amlogic-gxl-cipher.c b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
index d93210726697..ee5998af2fe8 100644
--- a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
+++ b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
@@ -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)
--
2.26.2
On Thu, Aug 27, 2020 at 09:44:01AM +0300, Denis Efremov wrote:
> 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
On Thu, Aug 27, 2020 at 09:43:59AM +0300, Denis Efremov wrote:
> 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(-)
>
> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
> index 16a467969d8e..5ffdc1cd5847 100644
> --- a/drivers/crypto/inside-secure/safexcel_hash.c
> +++ b/drivers/crypto/inside-secure/safexcel_hash.c
> @@ -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;
> --
> 2.26.2
>
The maintainer of this driver was not TO/CC.
I Add him.
Regards
Hello Denis,
Quoting Denis Efremov (2020-08-27 08:43:59)
> Use kfree_sensitive() instead of open-coding it.
>
> Signed-off-by: Denis Efremov <[email protected]>
Acked-by: Antoine Tenart <[email protected]>
Thanks!
Antoine
> ---
> drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
> index 16a467969d8e..5ffdc1cd5847 100644
> --- a/drivers/crypto/inside-secure/safexcel_hash.c
> +++ b/drivers/crypto/inside-secure/safexcel_hash.c
> @@ -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;
> --
> 2.26.2
>
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
> -----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(-)
>
> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
> index 16a467969d8e..5ffdc1cd5847 100644
> --- a/drivers/crypto/inside-secure/safexcel_hash.c
> +++ b/drivers/crypto/inside-secure/safexcel_hash.c
> @@ -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?
> 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>
On Thu, Aug 27, 2020 at 09:43:58AM +0300, Denis Efremov wrote:
> 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(-)
All applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi,
On 9/2/20 4:10 PM, Van Leeuwen, Pascal wrote:
>> -----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(-)
>>
>> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
>> index 16a467969d8e..5ffdc1cd5847 100644
>> --- a/drivers/crypto/inside-secure/safexcel_hash.c
>> +++ b/drivers/crypto/inside-secure/safexcel_hash.c
>> @@ -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.
kfree_sensitive() uses memzero_explicit() internally.
> 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