Skip to contentSkip to conversation

crypto: Hash scatterwalk fixes

PATCH In review Herbert Xu 2 participants 6 messages Mar 13, 2025 – Mar 16, 2025

Subsystems (2)
lore
Share & tools
lore

Herbert Xu

author maintainer
#0 lore raw

v3 cover crypto: Hash scatterwalk fixes

v3 restores the krb5 patch and adds kmap_local support to sg miter. Also remove nth_page since it appears to be unnecessary.

This patch series is based on top of:

https://patchwork.kernel.org/project/linux-crypto/patch/[email protected]/

This was prompted by using nth_page instead of incrementing struct page by hand. However, that turned out to be unnecessary so what remains are simply two bug fixes.

Herbert Xu (3):
lib/scatterlist: Add SG_MITER_LOCAL and use it
crypto: krb5 - Use SG miter instead of doing it by hand
crypto: hash - Fix test underflow in shash_ahash_digest

 crypto/ahash.c                   | 40 +++++++++++++++++++++-----------
 crypto/krb5/rfc3961_simplified.c | 35 ++++++++++++----------------
 include/linux/scatterlist.h      |  1 +
 lib/scatterlist.c                | 12 ++++++----
 4 files changed, 50 insertions(+), 38 deletions(-)
Signature · 1 line
2.39.5
author maintainer
#1 lore raw

PATCH v3 1/3 lib/scatterlist: Add SG_MITER_LOCAL and use it

Add kmap_local support to the scatterlist iterator. Use it for all the helper functions in lib/scatterlist.

---
 include/linux/scatterlist.h |  1 +
 lib/scatterlist.c           | 12 ++++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)
include/linux/scatterlist.h +1 -0
index d836e7440ee8..138e2f1bd08f 100644
@@ -671,6 +671,7 @@ sg_page_iter_dma_address(struct sg_dma_page_iter *dma_iter)
 #define SG_MITER_ATOMIC		(1 << 0)	 /* use kmap_atomic */
 #define SG_MITER_TO_SG		(1 << 1)	/* flush back to phys on unmap */
 #define SG_MITER_FROM_SG	(1 << 2)	/* nop */
+#define SG_MITER_LOCAL		(1 << 3)	 /* use kmap_local */
 
 struct sg_mapping_iter {
 	/* the following three fields can be accessed directly */
lib/scatterlist.c +8 -4
index 5bb6b8aff232..b58d5ef1a34b 100644
@@ -879,7 +879,7 @@ EXPORT_SYMBOL(sg_miter_skip);
  *   @miter->addr and @miter->length point to the current mapping.
  *
  * Context:
- *   May sleep if !SG_MITER_ATOMIC.
+ *   May sleep if !SG_MITER_ATOMIC && !SG_MITER_LOCAL.
  *
  * Returns:
  *   true if @miter contains the next mapping.  false if end of sg
@@ -901,6 +901,8 @@ bool sg_miter_next(struct sg_mapping_iter *miter)
 
 	if (miter->__flags & SG_MITER_ATOMIC)
 		miter->addr = kmap_atomic(miter->page) + miter->__offset;
+	else if (miter->__flags & SG_MITER_LOCAL)
+		miter->addr = kmap_local_page(miter->page) + miter->__offset;
 	else
 		miter->addr = kmap(miter->page) + miter->__offset;
 
@@ -936,7 +938,9 @@ void sg_miter_stop(struct sg_mapping_iter *miter)
 		if (miter->__flags & SG_MITER_ATOMIC) {
 			WARN_ON_ONCE(!pagefault_disabled());
 			kunmap_atomic(miter->addr);
-		} else
+		} else if (miter->__flags & SG_MITER_LOCAL)
+			kunmap_local(miter->addr);
+		else
 			kunmap(miter->page);
 
 		miter->page = NULL;
@@ -965,7 +969,7 @@ size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf,
 {
 	unsigned int offset = 0;
 	struct sg_mapping_iter miter;
-	unsigned int sg_flags = SG_MITER_ATOMIC;
+	unsigned int sg_flags = SG_MITER_LOCAL;
 
 	if (to_buffer)
 		sg_flags |= SG_MITER_FROM_SG;
@@ -1080,7 +1084,7 @@ size_t sg_zero_buffer(struct scatterlist *sgl, unsigned int nents,
 {
 	unsigned int offset = 0;
 	struct sg_mapping_iter miter;
-	unsigned int sg_flags = SG_MITER_ATOMIC | SG_MITER_TO_SG;
+	unsigned int sg_flags = SG_MITER_LOCAL | SG_MITER_TO_SG;
 
 	sg_miter_start(&miter, sgl, nents, sg_flags);
 
Signature · 1 line
2.39.5
#2 lore raw

Re: [v3 PATCH 1/3] lib/scatterlist: Add SG_MITER_LOCAL and use it

Quoted 3 lines · Herbert Xu

* Context:
- * May sleep if !SG_MITER_ATOMIC.
+ * May sleep if !SG_MITER_ATOMIC && !SG_MITER_LOCAL.

This is incorrect. kmap_local_page() does not disable preemption.

- Eric

author maintainer
#3 lore raw

Re: [v3 PATCH 1/3] lib/scatterlist: Add SG_MITER_LOCAL and use it

Quoted 6 lines · Eric Biggers
Quoted 3 lines · Herbert Xu

* Context:
- * May sleep if !SG_MITER_ATOMIC.
+ * May sleep if !SG_MITER_ATOMIC && !SG_MITER_LOCAL.

This is incorrect. kmap_local_page() does not disable preemption.

I thought it was talking about the function itself, i.e., that kmap may sleep. But on a second look yes this is ambiguous and it could be made clearer.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author maintainer
#4 lore raw

PATCH v3 2/3 crypto: krb5 - Use SG miter instead of doing it by hand

In reply to Herbert Xu

The function crypto_shash_update_sg iterates through an SG by hand. It fails to handle corner cases such as SG entries longer than a page. Fix this by using the SG iterator.

Fixes 348f5669d1f6 ("crypto/krb5: Implement the Kerberos5 rfc3961 get_mic and verify_mic")
---
 crypto/krb5/rfc3961_simplified.c | 35 ++++++++++++++------------------
 1 file changed, 15 insertions(+), 20 deletions(-)
crypto/krb5/rfc3961_simplified.c +15 -20
index c1dcb0dd3a00..79180d28baa9 100644
@@ -67,9 +67,9 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/random.h>
+#include <linux/scatterlist.h>
 #include <linux/skbuff.h>
 #include <linux/slab.h>
-#include <linux/highmem.h>
 #include <linux/lcm.h>
 #include <linux/rtnetlink.h>
 #include <crypto/authenc.h>
@@ -83,26 +83,21 @@
 int crypto_shash_update_sg(struct shash_desc *desc, struct scatterlist *sg,
 			   size_t offset, size_t len)
 {
-	do {
-		int ret;
+	struct sg_mapping_iter miter;
+	size_t i, n;
+	int ret = 0;
 
-		if (offset < sg->length) {
-			struct page *page = sg_page(sg);
-			void *p = kmap_local_page(page);
-			void *q = p + sg->offset + offset;
-			size_t seg = min_t(size_t, len, sg->length - offset);
-
-			ret = crypto_shash_update(desc, q, seg);
-			kunmap_local(p);
-			if (ret < 0)
-				return ret;
-			len -= seg;
-			offset = 0;
-		} else {
-			offset -= sg->length;
-		}
-	} while (len > 0 && (sg = sg_next(sg)));
-	return 0;
+	sg_miter_start(&miter, sg, sg_nents(sg),
+		       SG_MITER_FROM_SG | SG_MITER_LOCAL);
+	for (i = 0; i < len; i += n) {
+		sg_miter_next(&miter);
+		n = min(miter.length, len - i);
+		ret = crypto_shash_update(desc, miter.addr, n);
+		if (ret < 0)
+			break;
+	}
+	sg_miter_stop(&miter);
+	return ret;
 }
 
 static int rfc3961_do_encrypt(struct crypto_sync_skcipher *tfm, void *iv,
Signature · 1 line
2.39.5
author maintainer
#5 lore raw

PATCH v3 3/3 crypto: hash - Fix test underflow in shash_ahash_digest

In reply to Herbert Xu

The test on PAGE_SIZE - offset in shash_ahash_digest can underflow, leading to execution of the fast path even if the data cannot be mapped into a single page.

Fix this by splitting the test into four cases:

1) nbytes > sg->length: More than one SG entry, slow path.
2) !IS_ENABLED(CONFIG_HIGHMEM): fast path.
3) nbytes > (unsigned int)PAGE_SIZE - offset: Two highmem pages, slow path.
4) Highmem fast path.

Fixes 5f7082ed4f48 ("crypto: hash - Export shash through hash")
---
 crypto/ahash.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)
crypto/ahash.c +26 -14
index 9c26175c21a8..1fe594880295 100644
@@ -16,6 +16,7 @@
 #include <linux/cryptouser.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -201,25 +202,36 @@ int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
 	unsigned int nbytes = req->nbytes;
 	struct scatterlist *sg;
 	unsigned int offset;
+	struct page *page;
+	const u8 *data;
 	int err;
 
-	if (ahash_request_isvirt(req))
-		return crypto_shash_digest(desc, req->svirt, nbytes,
-					   req->result);
+	data = req->svirt;
+	if (!nbytes || ahash_request_isvirt(req))
+		return crypto_shash_digest(desc, data, nbytes, req->result);
 
-	if (nbytes &&
-	    (sg = req->src, offset = sg->offset,
-	     nbytes <= min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset))) {
-		void *data;
+	sg = req->src;
+	if (nbytes > sg->length)
+		return crypto_shash_init(desc) ?:
+		       shash_ahash_finup(req, desc);
 
-		data = kmap_local_page(sg_page(sg));
-		err = crypto_shash_digest(desc, data + offset, nbytes,
-					  req->result);
-		kunmap_local(data);
-	} else
-		err = crypto_shash_init(desc) ?:
-		      shash_ahash_finup(req, desc);
+	page = sg_page(sg);
+	offset = sg->offset;
+	data = lowmem_page_address(page) + offset;
+	if (!IS_ENABLED(CONFIG_HIGHMEM))
+		return crypto_shash_digest(desc, data, nbytes, req->result);
 
+	page += offset >> PAGE_SHIFT;
+	offset = offset_in_page(offset);
+
+	if (nbytes > (unsigned int)PAGE_SIZE - offset)
+		return crypto_shash_init(desc) ?:
+		       shash_ahash_finup(req, desc);
+
+	data = kmap_local_page(page);
+	err = crypto_shash_digest(desc, data + offset, nbytes,
+				  req->result);
+	kunmap_local(data);
 	return err;
 }
 EXPORT_SYMBOL_GPL(shash_ahash_digest);
Signature · 1 line
2.39.5