thr3ads.net - Linux Virtualization - [PATCH v2 2/2] crypto: Remove unnecessary memzero

If this information is useful, please help other people find it:
Share via:

Christophe Leroy

2020-Apr-14 06:08 UTC

[PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

Le 14/04/2020 ? 00:28, Waiman Long a ?crit?:> Since kfree_sensitive() will do an implicit memzero_explicit(), there
> is no need to call memzero_explicit() before it. Eliminate those
> memzero_explicit() and simplify the call sites. For better correctness,
> the setting of keylen is also moved down after the key pointer check.
> 
> Signed-off-by: Waiman Long <longman at redhat.com>
> ---
>   .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 19 +++++-------------
>   .../allwinner/sun8i-ss/sun8i-ss-cipher.c      | 20 +++++--------------
>   drivers/crypto/amlogic/amlogic-gxl-cipher.c   | 12 +++--------
>   drivers/crypto/inside-secure/safexcel_hash.c  |  3 +--
>   4 files changed, 14 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> index aa4e8fdc2b32..8358fac98719 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_sync_skcipher(op->fallback_tfm);
>   	pm_runtime_put_sync_suspend(op->ce->dev);
>   }
> @@ -391,14 +388,11 @@ 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);
> -	}
> -	op->keylen = keylen;
> +	kfree_sensitive(op->key);
>   	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
>   	if (!op->key)
>   		return -ENOMEM;
> +	op->keylen = keylen;
Does it matter at all to ensure op->keylen is not set when of->key is 
NULL ? I'm not sure.

But if it does, then op->keylen should be set to 0 when freeing op->key.
>   
>   	crypto_sync_skcipher_clear_flags(op->fallback_tfm,
CRYPTO_TFM_REQ_MASK);
>   	crypto_sync_skcipher_set_flags(op->fallback_tfm,
tfm->base.crt_flags & CRYPTO_TFM_REQ_MASK);
> @@ -416,14 +410,11 @@ 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);
> -	}
> -	op->keylen = keylen;
> +	kfree_sensitive(op->key);
>   	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
>   	if (!op->key)
>   		return -ENOMEM;
> +	op->keylen = keylen;
Same comment as above.
>   
>   	crypto_sync_skcipher_clear_flags(op->fallback_tfm,
CRYPTO_TFM_REQ_MASK);
>   	crypto_sync_skcipher_set_flags(op->fallback_tfm,
tfm->base.crt_flags & CRYPTO_TFM_REQ_MASK);
> diff --git a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
> index 5246ef4f5430..0495fbc27fcc 100644
> --- a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
> +++ b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
> @@ -249,7 +249,6 @@ static int sun8i_ss_cipher(struct skcipher_request
*areq)
>   			offset = areq->cryptlen - ivsize;
>   			if (rctx->op_dir & SS_DECRYPTION) {
>   				memcpy(areq->iv, backup_iv, ivsize);
> -				memzero_explicit(backup_iv, ivsize);
>   				kfree_sensitive(backup_iv);
>   			} else {
>   				scatterwalk_map_and_copy(areq->iv, areq->dst, offset,
> @@ -367,10 +366,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_sync_skcipher(op->fallback_tfm);
>   	pm_runtime_put_sync(op->ss->dev);
>   }
> @@ -392,14 +388,11 @@ 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);
> -	}
> -	op->keylen = keylen;
> +	kfree_sensitive(op->key);
>   	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
>   	if (!op->key)
>   		return -ENOMEM;
> +	op->keylen = keylen;
Same comment as above.
>   
>   	crypto_sync_skcipher_clear_flags(op->fallback_tfm,
CRYPTO_TFM_REQ_MASK);
>   	crypto_sync_skcipher_set_flags(op->fallback_tfm,
tfm->base.crt_flags & CRYPTO_TFM_REQ_MASK);
> @@ -418,14 +411,11 @@ 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);
> -	}
> -	op->keylen = keylen;
> +	kfree_sensitive(op->key);
>   	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
>   	if (!op->key)
>   		return -ENOMEM;
> +	op->keylen = keylen;
Same comment as above.
>   
>   	crypto_sync_skcipher_clear_flags(op->fallback_tfm,
CRYPTO_TFM_REQ_MASK);
>   	crypto_sync_skcipher_set_flags(op->fallback_tfm,
tfm->base.crt_flags & CRYPTO_TFM_REQ_MASK);
> diff --git a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
> index fd1269900d67..6aa9ce7bbbd4 100644
> --- a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
> +++ b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
> @@ -341,10 +341,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_sync_skcipher(op->fallback_tfm);
>   }
>   
> @@ -368,14 +365,11 @@ 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);
> -	}
> -	op->keylen = keylen;
> +	kfree_sensitive(op->key);
>   	op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
>   	if (!op->key)
>   		return -ENOMEM;
> +	op->keylen = keylen;
Same comment as above.
>   
>   	return crypto_sync_skcipher_setkey(op->fallback_tfm, key, keylen);
>   }
> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c
b/drivers/crypto/inside-secure/safexcel_hash.c
> index 43962bc709c6..4a2d162914de 100644
> --- a/drivers/crypto/inside-secure/safexcel_hash.c
> +++ b/drivers/crypto/inside-secure/safexcel_hash.c
> @@ -1081,8 +1081,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;
> 

Christophe

Waiman Long

2020-Apr-14 16:24 UTC

head link

[PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

On 4/14/20 2:08 AM, Christophe Leroy wrote:>
>
> Le 14/04/2020 ? 00:28, Waiman Long a ?crit?:
>> Since kfree_sensitive() will do an implicit memzero_explicit(), there
>> is no need to call memzero_explicit() before it. Eliminate those
>> memzero_explicit() and simplify the call sites. For better correctness,
>> the setting of keylen is also moved down after the key pointer check.
>>
>> Signed-off-by: Waiman Long <longman at redhat.com>
>> ---
>> ? .../allwinner/sun8i-ce/sun8i-ce-cipher.c????? | 19 +++++-------------
>> ? .../allwinner/sun8i-ss/sun8i-ss-cipher.c????? | 20
+++++--------------
>> ? drivers/crypto/amlogic/amlogic-gxl-cipher.c?? | 12 +++--------
>> ? drivers/crypto/inside-secure/safexcel_hash.c? |? 3 +--
>> ? 4 files changed, 14 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> index aa4e8fdc2b32..8358fac98719 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_sync_skcipher(op->fallback_tfm);
>> ????? pm_runtime_put_sync_suspend(op->ce->dev);
>> ? }
>> @@ -391,14 +388,11 @@ 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);
>> -??? }
>> -??? op->keylen = keylen;
>> +??? kfree_sensitive(op->key);
>> ????? op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
>> ????? if (!op->key)
>> ????????? return -ENOMEM;
>> +??? op->keylen = keylen;
>
> Does it matter at all to ensure op->keylen is not set when of->key is
> NULL ? I'm not sure.
>
> But if it does, then op->keylen should be set to 0 when freeing
op->key.
My thinking is that if memory allocation fails, we just don't touch
anything and return an error code. I will not explicitly set keylen to 0
in this case unless it is specified in the API documentation.

Cheers,
Longman

Waiman Long

2020-Apr-14 19:37 UTC

head link

[PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

On 4/14/20 3:16 PM, Michal Such?nek wrote:> On Tue, Apr 14, 2020 at 12:24:36PM -0400, Waiman Long wrote:
>> On 4/14/20 2:08 AM, Christophe Leroy wrote:
>>>
>>> Le 14/04/2020 ? 00:28, Waiman Long a ?crit?:
>>>> Since kfree_sensitive() will do an implicit memzero_explicit(),
there
>>>> is no need to call memzero_explicit() before it. Eliminate
those
>>>> memzero_explicit() and simplify the call sites. For better
correctness,
>>>> the setting of keylen is also moved down after the key pointer
check.
>>>>
>>>> Signed-off-by: Waiman Long <longman at redhat.com>
>>>> ---
>>>> ? .../allwinner/sun8i-ce/sun8i-ce-cipher.c????? | 19
+++++-------------
>>>> ? .../allwinner/sun8i-ss/sun8i-ss-cipher.c????? | 20
+++++--------------
>>>> ? drivers/crypto/amlogic/amlogic-gxl-cipher.c?? | 12
+++--------
>>>> ? drivers/crypto/inside-secure/safexcel_hash.c? |? 3 +--
>>>> ? 4 files changed, 14 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git
a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>>>> b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>>>> index aa4e8fdc2b32..8358fac98719 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_sync_skcipher(op->fallback_tfm);
>>>> ????? pm_runtime_put_sync_suspend(op->ce->dev);
>>>> ? }
>>>> @@ -391,14 +388,11 @@ 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);
>>>> -??? }
>>>> -??? op->keylen = keylen;
>>>> +??? kfree_sensitive(op->key);
>>>> ????? op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
>>>> ????? if (!op->key)
>>>> ????????? return -ENOMEM;
>>>> +??? op->keylen = keylen;
>>> Does it matter at all to ensure op->keylen is not set when
of->key is
>>> NULL ? I'm not sure.
>>>
>>> But if it does, then op->keylen should be set to 0 when freeing
op->key.
>> My thinking is that if memory allocation fails, we just don't touch
>> anything and return an error code. I will not explicitly set keylen to
0
>> in this case unless it is specified in the API documentation.
> You already freed the key by now so not touching anything is not
> possible. The key is set to NULL on allocation failure so setting keylen
> to 0 should be redundant. However, setting keylen to 0 is consisent with
> not having a key, and it avoids the possibility of leaking the length
> later should that ever cause any problem.
OK, I can change it to clear the key length when the allocation failed
which isn't likely.

Cheers,
Longman

Joe Perches

2020-Apr-14 19:44 UTC

head link

[PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

On Tue, 2020-04-14 at 15:37 -0400, Waiman Long wrote:> OK, I can change it to clear the key length when the allocation failed
> which isn't likely.

Perhaps:

	kfree_sensitive(op->key);
	op->key = NULL;
	op->keylen = 0;

but I don't know that it impacts any possible state.

Seemingly Similar Threads

Search for more reasonably related threads

Linux Virtualization - Apr 2020 - [PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

[PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

[PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

[PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

[PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

Seemingly Similar Threads