Skip to contentSkip to conversation

crypto: Fix memcpy_sglist()

PATCH Superseded Eric Biggers 1 participant 4 messages Nov 14, 2025 – Nov 15, 2025

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover crypto: Fix memcpy_sglist()

This series rewrites memcpy_sglist() to fix the bug where it called functions that could fail and ignored errors.

This series is targeting crypto/master.

Eric Biggers (2):
crypto: scatterwalk - Fix memcpy_sglist() to always succeed Revert "crypto: scatterwalk - Move skcipher walk and use it for
memcpy_sglist"

 crypto/scatterwalk.c               | 351 ++++++++---------------------
 crypto/skcipher.c                  | 261 ++++++++++++++++++++-
 include/crypto/algapi.h            |  12 +
 include/crypto/internal/skcipher.h |  48 +++-
 include/crypto/scatterwalk.h       | 117 +++-------
 5 files changed, 437 insertions(+), 352 deletions(-)

base-commit: 59b0afd01b2ce353ab422ea9c8375b03db313a21

Signature · 1 line
2.51.2
#1 lore raw

PATCH 1/2 crypto: scatterwalk - Fix memcpy_sglist() to always succeed

The original implementation of memcpy_sglist() was broken because it didn't handle overlapping scatterlists. The current implementation is broken too because it calls the skcipher_walk functions which can fail. It ignores any errors from those functions.

Fix it by replacing it with a new implementation written from scratch. It always succeeds. It's also a bit faster, since it avoids the overhead of skcipher_walk. skcipher_walk includes a lot of functionality (such as alignmask handling) that's irrelevant here.

Fixes 131bdceca1f0 ("crypto: scatterwalk - Add memcpy_sglist")
Fixes 0f8d42bf128d ("crypto: scatterwalk - Move skcipher walk and use it for memcpy_sglist")
---
 crypto/scatterwalk.c         | 103 ++++++++++++++++++++++++++++++-----
 include/crypto/scatterwalk.h |  52 +++++++++++-------
 2 files changed, 121 insertions(+), 34 deletions(-)
crypto/scatterwalk.c +90 -13
index 1d010e2a1b1a..af6c17bfcadb 100644
@@ -99,30 +99,107 @@ void memcpy_to_sglist(struct scatterlist *sg, unsigned int start,
 	scatterwalk_start_at_pos(&walk, sg, start);
 	memcpy_to_scatterwalk(&walk, buf, nbytes);
 }
 EXPORT_SYMBOL_GPL(memcpy_to_sglist);
 
+/**
+ * memcpy_sglist() - Copy data from one scatterlist to another
+ * @dst: The destination scatterlist.  Can be NULL if @nbytes == 0.
+ * @src: The source scatterlist.  Can be NULL if @nbytes == 0.
+ * @nbytes: Number of bytes to copy
+ *
+ * The scatterlists can overlap.  Hence this really acts like memmove(), not
+ * memcpy().
+ *
+ * Context: Any context
+ */
 void memcpy_sglist(struct scatterlist *dst, struct scatterlist *src,
 		   unsigned int nbytes)
 {
-	struct skcipher_walk walk = {};
+	unsigned int src_offset, dst_offset;
 
-	if (unlikely(nbytes == 0)) /* in case sg == NULL */
+	if (unlikely(nbytes == 0)) /* in case src and/or dst is NULL */
 		return;
 
-	walk.total = nbytes;
-
-	scatterwalk_start(&walk.in, src);
-	scatterwalk_start(&walk.out, dst);
+	src_offset = src->offset;
+	dst_offset = dst->offset;
+	for (;;) {
+		/* Compute the length to copy this step. */
+		unsigned int len = min3(src->offset + src->length - src_offset,
+					dst->offset + dst->length - dst_offset,
+					nbytes);
+		struct page *src_page = sg_page(src);
+		struct page *dst_page = sg_page(dst);
+		const void *src_virt;
+		void *dst_virt;
+
+		if (IS_ENABLED(CONFIG_HIGHMEM)) {
+			/* HIGHMEM: we may have to actually map the pages. */
+			const unsigned int src_oip = offset_in_page(src_offset);
+			const unsigned int dst_oip = offset_in_page(dst_offset);
+			const unsigned int limit = PAGE_SIZE;
+
+			/* Further limit len to not cross a page boundary. */
+			len = min3(len, limit - src_oip, limit - dst_oip);
+
+			/* Compute the source and destination pages. */
+			src_page += src_offset / PAGE_SIZE;
+			dst_page += dst_offset / PAGE_SIZE;
+
+			if (src_page != dst_page) {
+				/*
+				 * Copy between different pages.
+				 * No need for memmove(), as the pages differ.
+				 */
+				src_virt = kmap_local_page(src_page);
+				dst_virt = kmap_local_page(dst_page);
+				memcpy(dst_virt + dst_oip, src_virt + src_oip,
+				       len);
+				flush_dcache_page(dst_page);
+				kunmap_local(dst_virt);
+				kunmap_local(src_virt);
+			} else if (src_oip != dst_oip) {
+				/* Copy between different parts of same page */
+				dst_virt = kmap_local_page(dst_page);
+				memmove(dst_virt + dst_oip, dst_virt + src_oip,
+					len);
+				flush_dcache_page(dst_page);
+				kunmap_local(dst_virt);
+			} /* Exact overlap.  No action needed. */
+		} else {
+			/*
+			 * !HIGHMEM: no mapping needed.  Just work in the linear
+			 * buffer of each sg entry.
+			 */
+			src_virt = page_address(src_page) + src_offset;
+			dst_virt = page_address(dst_page) + dst_offset;
+			if (src_virt != dst_virt) {
+				memmove(dst_virt, src_virt, len);
+				if (ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE)
+					__scatterwalk_flush_dcache_pages(
+						dst_page, dst_offset, len);
+			}
+		}
+		nbytes -= len;
+		if (nbytes == 0) /* No more to copy? */
+			break;
 
-	skcipher_walk_first(&walk, true);
-	do {
-		if (walk.src.virt.addr != walk.dst.virt.addr)
-			memcpy(walk.dst.virt.addr, walk.src.virt.addr,
-			       walk.nbytes);
-		skcipher_walk_done(&walk, 0);
-	} while (walk.nbytes);
+		/*
+		 * There's more to copy.  Advance the offsets by the length
+		 * copied this step, and advance the sg entries as needed.
+		 */
+		src_offset += len;
+		if (src_offset >= src->offset + src->length) {
+			src = sg_next(src);
+			src_offset = src->offset;
+		}
+		dst_offset += len;
+		if (dst_offset >= dst->offset + dst->length) {
+			dst = sg_next(dst);
+			dst_offset = dst->offset;
+		}
+	}
 }
 EXPORT_SYMBOL_GPL(memcpy_sglist);
 
 struct scatterlist *scatterwalk_ffwd(struct scatterlist dst[2],
 				     struct scatterlist *src,
include/crypto/scatterwalk.h +31 -21

72 lines not on this page · raw message

Signature · 1 line
2.51.2
#2 lore raw

Re: [PATCH 1/2] crypto: scatterwalk - Fix memcpy_sglist() to always succeed

Quoted 11 lines · Eric Biggers

+/**
+ * memcpy_sglist() - Copy data from one scatterlist to another
+ * @dst: The destination scatterlist. Can be NULL if @nbytes == 0.
+ * @src: The source scatterlist. Can be NULL if @nbytes == 0.
+ * @nbytes: Number of bytes to copy
+ *
+ * The scatterlists can overlap. Hence this really acts like memmove(), not
+ * memcpy().
+ *
+ * Context: Any context
+ */

[...]

crypto/scatterwalk.c void memcpy_to_sglist(struct scatterlist *sg, unsigned int start, · line 180

Quoted 6 lines
crypto/scatterwalk.c +6 -0
+			if (src_virt != dst_virt) {
+				memmove(dst_virt, src_virt, len);
+				if (ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE)
+					__scatterwalk_flush_dcache_pages(
+						dst_page, dst_offset, len);
+			}

I realized that this doesn't correctly handle arbitrary overlaps in the case where there are multiple copy steps. So, my idea to make this function more robust by allowing arbitrary overlaps won't easily work. I'll send a revised version that only claims to support exact overlaps, which is consistent with the current code.

- Eric

#3 lore raw

PATCH 2/2 Revert "crypto: scatterwalk - Move skcipher walk and use it for memcpy_sglist"

In reply to Eric Biggers

This reverts commit 0f8d42bf128d349ad490e87d5574d211245e40f1, with the memcpy_sglist() part dropped.

Now that memcpy_sglist() no longer uses the skcipher_walk code, the skcipher_walk code can be moved back where it belongs.

---
 crypto/scatterwalk.c               | 248 ---------------------------
 crypto/skcipher.c                  | 261 ++++++++++++++++++++++++++++-
 include/crypto/algapi.h            |  12 ++
 include/crypto/internal/skcipher.h |  48 +++++-
 include/crypto/scatterwalk.h       |  65 +------
 5 files changed, 316 insertions(+), 318 deletions(-)
crypto/scatterwalk.c +0 -248
index af6c17bfcadb..bf120edfa3ac 100644
@@ -8,29 +8,14 @@
  *               2002 Adam J. Richter <[email protected]>
  *               2004 Jean-Luc Cooke <[email protected]>
  */
 
 #include <crypto/scatterwalk.h>
-#include <linux/crypto.h>
-#include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/scatterlist.h>
-#include <linux/slab.h>
-
-enum {
-	SKCIPHER_WALK_SLOW = 1 << 0,
-	SKCIPHER_WALK_COPY = 1 << 1,
-	SKCIPHER_WALK_DIFF = 1 << 2,
-	SKCIPHER_WALK_SLEEP = 1 << 3,
-};
-
-static inline gfp_t skcipher_walk_gfp(struct skcipher_walk *walk)
-{
-	return walk->flags & SKCIPHER_WALK_SLEEP ? GFP_KERNEL : GFP_ATOMIC;
-}
 
 void scatterwalk_skip(struct scatter_walk *walk, unsigned int nbytes)
 {
 	struct scatterlist *sg = walk->sg;
 
@@ -221,238 +206,5 @@ struct scatterlist *scatterwalk_ffwd(struct scatterlist dst[2],
 	scatterwalk_crypto_chain(dst, sg_next(src), 2);
 
 	return dst;
 }
 EXPORT_SYMBOL_GPL(scatterwalk_ffwd);
-
-static int skcipher_next_slow(struct skcipher_walk *walk, unsigned int bsize)
-{
-	unsigned alignmask = walk->alignmask;
-	unsigned n;
-	void *buffer;
-
-	if (!walk->buffer)
-		walk->buffer = walk->page;
-	buffer = walk->buffer;
-	if (!buffer) {
-		/* Min size for a buffer of bsize bytes aligned to alignmask */
-		n = bsize + (alignmask & ~(crypto_tfm_ctx_alignment() - 1));
-
-		buffer = kzalloc(n, skcipher_walk_gfp(walk));
-		if (!buffer)
-			return skcipher_walk_done(walk, -ENOMEM);
-		walk->buffer = buffer;
-	}
-
-	buffer = PTR_ALIGN(buffer, alignmask + 1);
-	memcpy_from_scatterwalk(buffer, &walk->in, bsize);
-	walk->out.__addr = buffer;
-	walk->in.__addr = walk->out.addr;
-
-	walk->nbytes = bsize;
-	walk->flags |= SKCIPHER_WALK_SLOW;
-
-	return 0;
-}
-
-static int skcipher_next_copy(struct skcipher_walk *walk)
-{
-	void *tmp = walk->page;
-
-	scatterwalk_map(&walk->in);
-	memcpy(tmp, walk->in.addr, walk->nbytes);
-	scatterwalk_unmap(&walk->in);
-	/*
-	 * walk->in is advanced later when the number of bytes actually
-	 * processed (which might be less than walk->nbytes) is known.
-	 */
-
-	walk->in.__addr = tmp;
-	walk->out.__addr = tmp;
-	return 0;
-}
-
-static int skcipher_next_fast(struct skcipher_walk *walk)
-{
-	unsigned long diff;
-
-	diff = offset_in_page(walk->in.offset) -
-	       offset_in_page(walk->out.offset);
-	diff |= (u8 *)(sg_page(walk->in.sg) + (walk->in.offset >> PAGE_SHIFT)) -
-		(u8 *)(sg_page(walk->out.sg) + (walk->out.offset >> PAGE_SHIFT));
-
-	scatterwalk_map(&walk->out);
-	walk->in.__addr = walk->out.__addr;
-
-	if (diff) {
-		walk->flags |= SKCIPHER_WALK_DIFF;
-		scatterwalk_map(&walk->in);
-	}
-
-	return 0;
-}
-
-static int skcipher_walk_next(struct skcipher_walk *walk)
-{
-	unsigned int bsize;
-	unsigned int n;
-
-	n = walk->total;
-	bsize = min(walk->stride, max(n, walk->blocksize));
-	n = scatterwalk_clamp(&walk->in, n);
-	n = scatterwalk_clamp(&walk->out, n);
-
-	if (unlikely(n < bsize)) {
-		if (unlikely(walk->total < walk->blocksize))
-			return skcipher_walk_done(walk, -EINVAL);
-
-slow_path:
-		return skcipher_next_slow(walk, bsize);
-	}
-	walk->nbytes = n;
-
-	if (unlikely((walk->in.offset | walk->out.offset) & walk->alignmask)) {
-		if (!walk->page) {
-			gfp_t gfp = skcipher_walk_gfp(walk);
-
-			walk->page = (void *)__get_free_page(gfp);
-			if (!walk->page)
-				goto slow_path;
-		}
-		walk->flags |= SKCIPHER_WALK_COPY;
-		return skcipher_next_copy(walk);
-	}
-
-	return skcipher_next_fast(walk);
-}
-
-static int skcipher_copy_iv(struct skcipher_walk *walk)
-{
-	unsigned alignmask = walk->alignmask;
-	unsigned ivsize = walk->ivsize;
-	unsigned aligned_stride = ALIGN(walk->stride, alignmask + 1);
-	unsigned size;
-	u8 *iv;
-
-	/* Min size for a buffer of stride + ivsize, aligned to alignmask */
-	size = aligned_stride + ivsize +
-	       (alignmask & ~(crypto_tfm_ctx_alignment() - 1));
-
-	walk->buffer = kmalloc(size, skcipher_walk_gfp(walk));
-	if (!walk->buffer)
-		return -ENOMEM;
-
-	iv = PTR_ALIGN(walk->buffer, alignmask + 1) + aligned_stride;
-
-	walk->iv = memcpy(iv, walk->iv, walk->ivsize);
-	return 0;
-}
-
-int skcipher_walk_first(struct skcipher_walk *walk, bool atomic)
-{
-	if (WARN_ON_ONCE(in_hardirq()))
-		return -EDEADLK;
-
-	walk->flags = atomic ? 0 : SKCIPHER_WALK_SLEEP;
-
-	walk->buffer = NULL;
-	if (unlikely(((unsigned long)walk->iv & walk->alignmask))) {
-		int err = skcipher_copy_iv(walk);
-		if (err)
-			return err;
-	}
-
-	walk->page = NULL;
-
-	return skcipher_walk_next(walk);
-}
-EXPORT_SYMBOL_GPL(skcipher_walk_first);
-
-/**
- * skcipher_walk_done() - finish one step of a skcipher_walk
- * @walk: the skcipher_walk
- * @res: number of bytes *not* processed (>= 0) from walk->nbytes,
- *	 or a -errno value to terminate the walk due to an error
- *
- * This function cleans up after one step of walking through the source and
- * destination scatterlists, and advances to the next step if applicable.
- * walk->nbytes is set to the number of bytes available in the next step,
- * walk->total is set to the new total number of bytes remaining, and
- * walk->{src,dst}.virt.addr is set to the next pair of data pointers.  If there
- * is no more data, or if an error occurred (i.e. -errno return), then
- * walk->nbytes and walk->total are set to 0 and all resources owned by the
- * skcipher_walk are freed.
- *
- * Return: 0 or a -errno value.  If @res was a -errno value then it will be
- *	   returned, but other errors may occur too.
- */
-int skcipher_walk_done(struct skcipher_walk *walk, int res)
-{
-	unsigned int n = walk->nbytes; /* num bytes processed this step */
-	unsigned int total = 0; /* new total remaining */
-
-	if (!n)
-		goto finish;
-
-	if (likely(res >= 0)) {
-		n -= res; /* subtract num bytes *not* processed */
-		total = walk->total - n;
-	}
-
-	if (likely(!(walk->flags & (SKCIPHER_WALK_SLOW |
-				    SKCIPHER_WALK_COPY |
-				    SKCIPHER_WALK_DIFF)))) {
-		scatterwalk_advance(&walk->in, n);
-	} else if (walk->flags & SKCIPHER_WALK_DIFF) {
-		scatterwalk_done_src(&walk->in, n);
-	} else if (walk->flags & SKCIPHER_WALK_COPY) {
-		scatterwalk_advance(&walk->in, n);
-		scatterwalk_map(&walk->out);
-		memcpy(walk->out.addr, walk->page, n);
-	} else { /* SKCIPHER_WALK_SLOW */
-		if (res > 0) {
-			/*
-			 * Didn't process all bytes.  Either the algorithm is
-			 * broken, or this was the last step and it turned out
-			 * the message wasn't evenly divisible into blocks but
-			 * the algorithm requires it.
-			 */
-			res = -EINVAL;
-			total = 0;
-		} else
-			memcpy_to_scatterwalk(&walk->out, walk->out.addr, n);
-		goto dst_done;
-	}
-
-	scatterwalk_done_dst(&walk->out, n);
-dst_done:
-
-	if (res > 0)
-		res = 0;
-
-	walk->total = total;
-	walk->nbytes = 0;
-
-	if (total) {
-		if (walk->flags & SKCIPHER_WALK_SLEEP)
-			cond_resched();
-		walk->flags &= ~(SKCIPHER_WALK_SLOW | SKCIPHER_WALK_COPY |
-				 SKCIPHER_WALK_DIFF);
-		return skcipher_walk_next(walk);
-	}
-
-finish:
-	/* Short-circuit for the common/fast path. */
-	if (!((unsigned long)walk->buffer | (unsigned long)walk->page))
-		goto out;
-
-	if (walk->iv != walk->oiv)
-		memcpy(walk->oiv, walk->iv, walk->ivsize);
-	if (walk->buffer != walk->page)
-		kfree(walk->buffer);
-	if (walk->page)
-		free_page((unsigned long)walk->page);
-
-out:
-	return res;
-}
-EXPORT_SYMBOL_GPL(skcipher_walk_done);
crypto/skcipher.c +255 -6

329 lines not on this page · raw message

include/crypto/algapi.h +12 -0
index fc4574940636..05deea9dac5e 100644
@@ -105,10 +105,22 @@ struct crypto_queue {
 
 	unsigned int qlen;
 	unsigned int max_qlen;
 };
 
+struct scatter_walk {
+	/* Must be the first member, see struct skcipher_walk. */
+	union {
+		void *const addr;
+
+		/* Private API field, do not touch. */
+		union crypto_no_such_thing *__addr;
+	};
+	struct scatterlist *sg;
+	unsigned int offset;
+};
+
 struct crypto_attr_alg {
 	char name[CRYPTO_MAX_ALG_NAME];
 };
 
 struct crypto_attr_type {
include/crypto/internal/skcipher.h +47 -1

88 lines not on this page · raw message

include/crypto/scatterwalk.h +2 -63

84 lines not on this page · raw message

Signature · 1 line
2.51.2