crypto: Use nth_page instead of doing it by hand

v2 removes the krb5 patch and changes the code so that nth_page is only called if HIGHMEM is enabled. Also switch to from page_address to lowmem_page_address.

This patch series is based on top of:

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

Curiously, the Crypto API scatterwalk incremented pages by hand rather than using nth_page. Possibly because scatterwalk predates nth_page (the following commit is from the history tree):

commit 3957f2b34960d85b63e814262a8be7d5ad91444d
Author: James Morris <[email protected]>
Date: Sun Feb 2 07:35:32 2003 -0800

[CRYPTO]: in/out scatterlist support for ciphers.

Fix this by using nth_page.

Herbert Xu (2):
crypto: scatterwalk - Use nth_page instead of doing it by hand
crypto: hash - Use nth_page instead of doing it by hand

 crypto/ahash.c               | 42 +++++++++++++++++++++++-------------
 include/crypto/scatterwalk.h | 21 +++++++++++-------
 2 files changed, 40 insertions(+), 23 deletions(-)

2.39.5

Curiously, the Crypto API scatterwalk incremented pages by hand rather than using nth_page. Possibly because scatterwalk predates nth_page (the following commit is from the history tree):

	commit 3957f2b34960d85b63e814262a8be7d5ad91444d
	Author: James Morris <[email protected]>
	Date:   Sun Feb 2 07:35:32 2003 -0800

	    [CRYPTO]: in/out scatterlist support for ciphers.

Fix this by using nth_page.

---
 include/crypto/scatterwalk.h | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

index b7e617ae4442..0ba5be363abf 100644
@@ -100,11 +100,15 @@ static inline void scatterwalk_get_sglist(struct scatter_walk *walk,
 static inline void scatterwalk_map(struct scatter_walk *walk)
 {
 	struct page *base_page = sg_page(walk->sg);
+	unsigned int offset = walk->offset;
+	void *addr;
 
 	if (IS_ENABLED(CONFIG_HIGHMEM)) {
-		walk->__addr = kmap_local_page(base_page +
-					       (walk->offset >> PAGE_SHIFT)) +
-			       offset_in_page(walk->offset);
+		struct page *page;
+
+		page = nth_page(base_page, offset >> PAGE_SHIFT);
+		offset = offset_in_page(offset);
+		addr = kmap_local_page(page) + offset;
 	} else {
 		/*
 		 * When !HIGHMEM we allow the walker to return segments that

@@ -112,13 +116,14 @@ static inline void scatterwalk_map(struct scatter_walk *walk)
 		 * clear that in this case we're working in the linear buffer of
 		 * the whole sg entry in the kernel's direct map rather than
 		 * within the mapped buffer of a single page, compute the
-		 * address as an offset from the page_address() of the first
-		 * page of the sg entry.  Either way the result is the address
-		 * in the direct map, but this makes it clearer what is really
-		 * going on.
+		 * address as an offset from the lowmem_page_address() of
+		 * the first page of the sg entry.  Either way the result
+		 * is the address in the direct map, but this makes it clearer
+		 * what is really going on.
 		 */
-		walk->__addr = page_address(base_page) + walk->offset;
+		addr = lowmem_page_address(base_page) + offset;
 	}
+	walk->__addr = addr;
 }
 
 /**

2.39.5

Well, also because it's never been clearly defined what a scatterlist even is.

This needs an explanation of what this is actually fixing.

I think it is: on a system with SPARSEMEM && !SPARSEMEM_VMEMMAP, if the caller provided a scatterlist element that spanned physical memory sections that are physically contiguous (but have separate vmemmaps), then the struct page(s) calculated for any non-first sections would be invalid. But then that invalid struct page just gets passed to kmap_local_page(), so it would only matter if also HIGHMEM || WANT_PAGE_VIRTUAL which are the cases where the struct page actually gets used, I think?

All the edge cases like this need to be fixed of course, though fortunately it sounds like this one was super rare.

		struct page *page;

		page = nth_page(base_page, offset >> PAGE_SHIFT);
		walk->__addr = kmap_local_page(page) + offset_in_page(offset);

lowmem_page_address is obviously linear, while page_address is not. For example, arch/loongarch does something funky with kfence and page->virtual so that page_address is non-linear even on 64-bit without HIGHMEM.

Sigh, it seems that they've overridden page_to_virt too so even lowmem_page_address is non-linear. But that's probably just a result of people abusing page_to_virt.

Cheers,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Well, it looks like the intention of !HIGHMEM && WANT_PAGE_VIRTUAL was for page->virtual to contain the linear address, as an optimization for architectures where multiplication and division are slow and sizeof(struct page) is not a power of 2. So the resulting address would be the same, just gotten in a different way. __init_single_page() in mm/mm_init.c indeed sets page->virtual that way. But if there are now cases where it does not actually contain the linear address as expected, then such configurations need to be excluded from the linear sg entry optimization...

- Eric

Use nth_page instead of adding n to the page pointer.

This also fixes a real bug in shash_ahash_digest where the the check for continguous hash data may be incorrect in the presence of highmem. This could result in an incorrect hash or worse.

---
 crypto/ahash.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

index 9c26175c21a8..aff0d3387f3a 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>

@@ -79,7 +80,7 @@ static int hash_walk_new_entry(struct crypto_hash_walk *walk)
 
 	sg = walk->sg;
 	walk->offset = sg->offset;
-	walk->pg = sg_page(walk->sg) + (walk->offset >> PAGE_SHIFT);
+	walk->pg = nth_page(sg_page(walk->sg), walk->offset >> PAGE_SHIFT);
 	walk->offset = offset_in_page(walk->offset);
 	walk->entrylen = sg->length;
 

@@ -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);
+	data = lowmem_page_address(page) + offset;
+	if (!IS_ENABLED(CONFIG_HIGHMEM))
+		return crypto_shash_digest(desc, data, nbytes, req->result);
 
+	offset = sg->offset;
+	page = nth_page(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);

2.39.5

Again, you need to explain this properly.

It seems that the "real bug" mentioned above is the case of scatterlist::offset > PAGE_SIZE. That's unrelated to the nth_page() fix, which seems to be for scatterlist elements that span physical memory sections. Also, unlike the case of scatterlist elements that span physical memory sections, scatterlist::offset > PAGE_SIZE can be easily tested. The self-tests need to be updated to test that case, if it's indeed now established that it's allowed.

Note that there is also page arithmetic being done in scatterwalk_done_dst() and scomp_acomp_comp_decomp(). Those presumably need the nth_page() fix too.

scomp_acomp_comp_decomp() also assumes that if the first page in a given scatterlist element is lowmem, then any additional pages are lowmem too. That sounds like another potentially wrong assumption. Can scatterlist elements span memory zones? Or just physical memory sections?

Is there actually going to be a clear specification of the scatterlist based crypto APIs, or just random broken and incomplete fixes?

Alright I'll try to split it up.

Thanks, I had missed the flushing code in scatterwalk.

As for scomp yes that's already fixed in my acomp series which I will repost soon.

Yes I've fixed that by changing it to test the last page rather than the first, assuming that highmem indeed comes after lowmem.

Theoretically it can cross anything. Check out the block merging code in __blk_rq_map_sg, it tries to merge any physically contiguous page.

Have you considered working in the mm subsystem? :)

But yes folios seem to be a lot nicer than a scatterlist. At least it's guaranteed to not cross any boundaries and is always a power of 2.

So I've added folio support to acomp and I'll be doing the same to the other interfaces.

Of course folios don't support the kind of non-linear memory used in sk_buff so we'll still need scatterlists for that.

Thanks for catching this. I hadn't noticed Linus turning off -Wmaybe-uninitialized. For all this time I'd been thinking that gcc has suddenly got a lot smarter.

Cheers,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Actually the block layer avoids this edge case. See the comment above struct bio_vec, and the corresponding code in bvec_try_merge_page():

        if (bv->bv_page + bv_end / PAGE_SIZE != page + off / PAGE_SIZE)
                return false;

- Eric

Well spotted. It looks like block scatterlists don't need nth_page:

commit 52d52d1c98a90cfe860b83498e4b6074aad95c15

Author: Christoph Hellwig <[email protected]>
Date:   Thu Apr 11 08:23:31 2019 +0200

block: only allow contiguous page structs in a bio_vec

That still leaves the question as to why folios are using nth_page for folio_page. It seems that the only reason is that the same function is also used by folio_next, which can cross boundaries.

So I will get rid of the nth_page changes, and steer clear of folio_page too.

Thanks,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt