[v2 PATCH 0/2] crypto: Use nth_page instead of doing it by hand

[v2 PATCH 0/2] crypto: Use nth_page instead of doing it by hand

[Herbert Xu]

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/20250310172016.153423-1-ebiggers@kernel.org/

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 <jmorris@intercode.com.au>
        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

[v2 PATCH 1/2] crypto: scatterwalk - Use nth_page instead of doing it by hand

[Herbert Xu]

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 <jmorris@intercode.com.au>
	Date:   Sun Feb 2 07:35:32 2003 -0800

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

Fix this by using nth_page.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 include/crypto/scatterwalk.h | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h
index b7e617ae4442..0ba5be363abf 100644
--- a/include/crypto/scatterwalk.h
+++ b/include/crypto/scatterwalk.h
@@ -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

[v2 PATCH 2/2] crypto: hash - Use nth_page instead of doing it by hand

[Herbert Xu]

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.

Fixes: 5f7082ed4f48 ("crypto: hash - Export shash through hash")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/ahash.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 9c26175c21a8..aff0d3387f3a 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -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

Re: [v2 PATCH 1/2] crypto: scatterwalk - Use nth_page instead of doing it by hand

[Eric Biggers]

On Wed, Mar 12, 2025 at 12:29:57PM +0800, Herbert Xu wrote:
> 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):

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

> 	commit 3957f2b34960d85b63e814262a8be7d5ad91444d
> 	Author: James Morris <jmorris@intercode.com.au>
> 	Date:   Sun Feb 2 07:35:32 2003 -0800
> 
> 	    [CRYPTO]: in/out scatterlist support for ciphers.
> 
> Fix this by using nth_page.

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.

> diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h
> index b7e617ae4442..0ba5be363abf 100644
> --- a/include/crypto/scatterwalk.h
> +++ b/include/crypto/scatterwalk.h
> @@ -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;

It would be cleaner as:

		struct page *page;

		page = nth_page(base_page, offset >> PAGE_SHIFT);
		walk->__addr = kmap_local_page(page) + offset_in_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;

This change is unrelated and seems incorrect.  lowmem_page_address() is a mm
implementation detail.  page_address() is the right one to use.

- Eric

Re: [v2 PATCH 2/2] crypto: hash - Use nth_page instead of doing it by hand

[Eric Biggers]

On Wed, Mar 12, 2025 at 12:30:00PM +0800, Herbert Xu wrote:
> 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.

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?

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

This is clearly untested, so not much point in reviewing it.

- Eric

Re: [v2 PATCH 1/2] crypto: scatterwalk - Use nth_page instead of doing it by hand

[Herbert Xu]

On Wed, Mar 12, 2025 at 12:56:22PM -0700, Eric Biggers wrote:
>
> This change is unrelated and seems incorrect.  lowmem_page_address() is a mm
> implementation detail.  page_address() is the right one to use.

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v2 PATCH 2/2] crypto: hash - Use nth_page instead of doing it by hand

[Herbert Xu]

On Wed, Mar 12, 2025 at 01:09:08PM -0700, Eric Biggers wrote:
>
> 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,

Alright I'll try to split it up.

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

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.

> 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

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

> sounds like another potentially wrong assumption.  Can scatterlist elements span
> memory zones?  Or just physical memory sections?

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

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

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.
 
> This is clearly untested, so not much point in reviewing it.

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v2 PATCH 2/2] crypto: hash - Use nth_page instead of doing it by hand

[Eric Biggers]

On Thu, Mar 13, 2025 at 10:36:44AM +0800, Herbert Xu wrote:
> On Wed, Mar 12, 2025 at 01:09:08PM -0700, Eric Biggers wrote:
> >
> > 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,
> 
> Alright I'll try to split it up.
> 
> > 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.
> 
> 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.
> 
> > 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
> 
> Yes I've fixed that by changing it to test the last page rather than
> the first, assuming that highmem indeed comes after lowmem.
> 
> > sounds like another potentially wrong assumption.  Can scatterlist elements span
> > memory zones?  Or just physical memory sections?
> 
> Theoretically it can cross anything.  Check out the block merging
> code in __blk_rq_map_sg, it tries to merge any physically contiguous
> page.

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

Re: [v2 PATCH 1/2] crypto: scatterwalk - Use nth_page instead of doing it by hand

[Eric Biggers]

On Thu, Mar 13, 2025 at 09:43:48AM +0800, Herbert Xu wrote:
> On Wed, Mar 12, 2025 at 12:56:22PM -0700, Eric Biggers wrote:
> >
> > This change is unrelated and seems incorrect.  lowmem_page_address() is a mm
> > implementation detail.  page_address() is the right one to use.
> 
> 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.

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

Re: [v2 PATCH 2/2] crypto: hash - Use nth_page instead of doing it by hand

[Herbert Xu]

On Thu, Mar 13, 2025 at 03:07:41AM +0000, Eric Biggers wrote:
>
> 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;

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

commit 52d52d1c98a90cfe860b83498e4b6074aad95c15
Author: Christoph Hellwig <hch@lst.de>
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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt