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

2020-04-14 Thread Joe Perches 
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.


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2020-04-14 Thread Waiman Long 
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 
 ---
   .../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


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2020-04-14 Thread Waiman Long 
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 
>> ---
>>   .../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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2020-04-14 Thread Christophe Leroy 



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 
---
  .../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) {

-