[PATCH 0/2] Cyrpto: Clean up kmap() use

[PATCH 0/2] Cyrpto: Clean up kmap() use

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

While going through kmap() users the following 2 issues were found via code
inspection.

Ira Weiny (2):
  crypto/ux500: Fix kmap() bug
  crypto: Remove unused async iterators

 crypto/ahash.c                        | 41 +++------------------------
 drivers/crypto/ux500/hash/hash_core.c | 30 ++++++++++++--------
 include/crypto/internal/hash.h        | 13 ---------
 3 files changed, 22 insertions(+), 62 deletions(-)

-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 1/2] crypto/ux500: Fix kmap() bug

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Once the crypto hash walk is started by crypto_hash_walk_first()
returning non-zero, crypto_hash_walk_done() must be called to unmap any
memory which was mapped by *_walk_first().

Ensure crypto_hash_walk_done() is called properly by:

	1) Re-arranging the check for device data to be prior to calling
	   *_walk_first()
	2) on error call crypto_hash_walk_done() with an error code to
	   allow the hash walk code to clean up.

While we are at it clean up the 'out' label to be more meaningful.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
	Not tested.
	Found via code inspection.
---
 drivers/crypto/ux500/hash/hash_core.c | 30 ++++++++++++++++-----------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/ux500/hash/hash_core.c b/drivers/crypto/ux500/hash/hash_core.c
index c24f2db8d5e8..5f407a3420f8 100644
--- a/drivers/crypto/ux500/hash/hash_core.c
+++ b/drivers/crypto/ux500/hash/hash_core.c
@@ -1071,27 +1071,32 @@ int hash_hw_update(struct ahash_request *req)
 	struct hash_ctx *ctx = crypto_ahash_ctx(tfm);
 	struct hash_req_ctx *req_ctx = ahash_request_ctx(req);
 	struct crypto_hash_walk walk;
-	int msg_length = crypto_hash_walk_first(req, &walk);
-
-	/* Empty message ("") is correct indata */
-	if (msg_length == 0)
-		return ret;
+	int msg_length;
 
 	index = req_ctx->state.index;
 	buffer = (u8 *)req_ctx->state.buffer;
 
+	ret = hash_get_device_data(ctx, &device_data);
+	if (ret)
+		return ret;
+
+	msg_length = crypto_hash_walk_first(req, &walk);
+
+	/* Empty message ("") is correct indata */
+	if (msg_length == 0) {
+		ret = 0;
+		goto release_dev;
+	}
+
 	/* Check if ctx->state.length + msg_length
 	   overflows */
 	if (msg_length > (req_ctx->state.length.low_word + msg_length) &&
 	    HASH_HIGH_WORD_MAX_VAL == req_ctx->state.length.high_word) {
 		pr_err("%s: HASH_MSG_LENGTH_OVERFLOW!\n", __func__);
-		return -EPERM;
+		ret = crypto_hash_walk_done(&walk, -EPERM);
+		goto release_dev;
 	}
 
-	ret = hash_get_device_data(ctx, &device_data);
-	if (ret)
-		return ret;
-
 	/* Main loop */
 	while (0 != msg_length) {
 		data_buffer = walk.data;
@@ -1101,7 +1106,8 @@ int hash_hw_update(struct ahash_request *req)
 		if (ret) {
 			dev_err(device_data->dev, "%s: hash_internal_hw_update() failed!\n",
 				__func__);
-			goto out;
+			crypto_hash_walk_done(&walk, ret);
+			goto release_dev;
 		}
 
 		msg_length = crypto_hash_walk_done(&walk, 0);
@@ -1111,7 +1117,7 @@ int hash_hw_update(struct ahash_request *req)
 	dev_dbg(device_data->dev, "%s: indata length=%d, bin=%d\n",
 		__func__, req_ctx->state.index, req_ctx->state.bit_index);
 
-out:
+release_dev:
 	release_hash_device(device_data);
 
 	return ret;
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 2/2] crypto: Remove unused async iterators

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Revert "crypto: hash - Add real ahash walk interface"
This reverts commit 75ecb231ff45b54afa9f4ec9137965c3c00868f4.

The callers of the functions in this commit were removed in ab8085c130ed

Remove these unused calls.

Fixes: ab8085c130ed ("crypto: x86 - remove SHA multibuffer routines and mcryptd")
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 crypto/ahash.c                 | 41 ++++------------------------------
 include/crypto/internal/hash.h | 13 -----------
 2 files changed, 4 insertions(+), 50 deletions(-)

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 68a0f0cb75c4..9c23b606949e 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -10,7 +10,6 @@
 
 #include <crypto/internal/hash.h>
 #include <crypto/scatterwalk.h>
-#include <linux/bug.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -46,10 +45,7 @@ static int hash_walk_next(struct crypto_hash_walk *walk)
 	unsigned int nbytes = min(walk->entrylen,
 				  ((unsigned int)(PAGE_SIZE)) - offset);
 
-	if (walk->flags & CRYPTO_ALG_ASYNC)
-		walk->data = kmap(walk->pg);
-	else
-		walk->data = kmap_atomic(walk->pg);
+	walk->data = kmap_atomic(walk->pg);
 	walk->data += offset;
 
 	if (offset & alignmask) {
@@ -99,16 +95,8 @@ int crypto_hash_walk_done(struct crypto_hash_walk *walk, int err)
 		}
 	}
 
-	if (walk->flags & CRYPTO_ALG_ASYNC)
-		kunmap(walk->pg);
-	else {
-		kunmap_atomic(walk->data);
-		/*
-		 * The may sleep test only makes sense for sync users.
-		 * Async users don't need to sleep here anyway.
-		 */
-		crypto_yield(walk->flags);
-	}
+	kunmap_atomic(walk->data);
+	crypto_yield(walk->flags);
 
 	if (err)
 		return err;
@@ -140,33 +128,12 @@ int crypto_hash_walk_first(struct ahash_request *req,
 
 	walk->alignmask = crypto_ahash_alignmask(crypto_ahash_reqtfm(req));
 	walk->sg = req->src;
-	walk->flags = req->base.flags & CRYPTO_TFM_REQ_MASK;
+	walk->flags = req->base.flags;
 
 	return hash_walk_new_entry(walk);
 }
 EXPORT_SYMBOL_GPL(crypto_hash_walk_first);
 
-int crypto_ahash_walk_first(struct ahash_request *req,
-			    struct crypto_hash_walk *walk)
-{
-	walk->total = req->nbytes;
-
-	if (!walk->total) {
-		walk->entrylen = 0;
-		return 0;
-	}
-
-	walk->alignmask = crypto_ahash_alignmask(crypto_ahash_reqtfm(req));
-	walk->sg = req->src;
-	walk->flags = req->base.flags & CRYPTO_TFM_REQ_MASK;
-	walk->flags |= CRYPTO_ALG_ASYNC;
-
-	BUILD_BUG_ON(CRYPTO_TFM_REQ_MASK & CRYPTO_ALG_ASYNC);
-
-	return hash_walk_new_entry(walk);
-}
-EXPORT_SYMBOL_GPL(crypto_ahash_walk_first);
-
 static int ahash_setkey_unaligned(struct crypto_ahash *tfm, const u8 *key,
 				unsigned int keylen)
 {
diff --git a/include/crypto/internal/hash.h b/include/crypto/internal/hash.h
index 89f6f46ab2b8..6d3ad5ac4d28 100644
--- a/include/crypto/internal/hash.h
+++ b/include/crypto/internal/hash.h
@@ -62,25 +62,12 @@ struct crypto_shash_spawn {
 int crypto_hash_walk_done(struct crypto_hash_walk *walk, int err);
 int crypto_hash_walk_first(struct ahash_request *req,
 			   struct crypto_hash_walk *walk);
-int crypto_ahash_walk_first(struct ahash_request *req,
-			   struct crypto_hash_walk *walk);
-
-static inline int crypto_ahash_walk_done(struct crypto_hash_walk *walk,
-					 int err)
-{
-	return crypto_hash_walk_done(walk, err);
-}
 
 static inline int crypto_hash_walk_last(struct crypto_hash_walk *walk)
 {
 	return !(walk->entrylen | walk->total);
 }
 
-static inline int crypto_ahash_walk_last(struct crypto_hash_walk *walk)
-{
-	return crypto_hash_walk_last(walk);
-}
-
 int crypto_register_ahash(struct ahash_alg *alg);
 void crypto_unregister_ahash(struct ahash_alg *alg);
 int crypto_register_ahashes(struct ahash_alg *algs, int count);
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 0/2] Cyrpto: Clean up kmap() use

[Ira Weiny]

On Mon, Aug 10, 2020 at 05:40:13PM -0700, 'Ira Weiny' wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> While going through kmap() users the following 2 issues were found via code
> inspection.

Any feedback on these patches?  Perhaps I've not included the correct people?
Adding some people to the CC list.

Specifically, Linus Walleij for the ux500 work.  Linus can you comment on the
first patch?

patch1: https://lore.kernel.org/lkml/20200811004015.2800392-2-ira.weiny@intel.com/
patch2: https://lore.kernel.org/lkml/20200811004015.2800392-3-ira.weiny@intel.com/

Thanks,
Ira

> 
> Ira Weiny (2):
>   crypto/ux500: Fix kmap() bug
>   crypto: Remove unused async iterators
> 
>  crypto/ahash.c                        | 41 +++------------------------
>  drivers/crypto/ux500/hash/hash_core.c | 30 ++++++++++++--------
>  include/crypto/internal/hash.h        | 13 ---------
>  3 files changed, 22 insertions(+), 62 deletions(-)
> 
> -- 
> 2.28.0.rc0.12.gb6a658bd00c9
> 

Re: [PATCH 0/2] Cyrpto: Clean up kmap() use

[Herbert Xu]

On Mon, Aug 10, 2020 at 05:40:13PM -0700, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> While going through kmap() users the following 2 issues were found via code
> inspection.
> 
> Ira Weiny (2):
>   crypto/ux500: Fix kmap() bug
>   crypto: Remove unused async iterators
> 
>  crypto/ahash.c                        | 41 +++------------------------
>  drivers/crypto/ux500/hash/hash_core.c | 30 ++++++++++++--------
>  include/crypto/internal/hash.h        | 13 ---------
>  3 files changed, 22 insertions(+), 62 deletions(-)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt