On 4/14/20 2:08 AM, Christophe Leroy wrote:

Le 14/04/2020 à 00:28, Waiman Long a écrit :

quoted

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@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