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

[Eric Biggers <ebiggers@kernel.org>]

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