crypto ahash requests on the stack

crypto ahash requests on the stack

[Mikulas Patocka]

Hi

I'd like to ask about this condition in crypto_ahash_digest:
	if (ahash_req_on_stack(req) && ahash_is_async(tfm))
		return -EAGAIN;

Can it be removed? Or, is there some reason why you can't have 
asynchronous requests on the stack (such as inability of doing DMA to 
virtually mapped stack)?

Or, should I just clear the flag CRYPTO_TFM_REQ_ON_STACK in my code?

I'm modifying dm-integrity to use asynchronous API so that Harald 
Freudenberger can use it on mainframes (the reason is that his 
implementation only provides asynchronous API) and I would prefer to place 
ahash requests on the stack (and wait for them before the function exits).

The commit 04bfa4c7d5119ca38f8133bfdae7957a60c8b221 says that we should 
clone the request with HASH_REQUEST_CLONE, but that is not usable in 
dm-integrity, because dm-integrity must work even when the system is out 
of memory.

Mikulas

Re: crypto ahash requests on the stack

[Herbert Xu]

On Mon, Aug 25, 2025 at 04:23:59PM +0200, Mikulas Patocka wrote:
> 
> I'd like to ask about this condition in crypto_ahash_digest:
> 	if (ahash_req_on_stack(req) && ahash_is_async(tfm))
> 		return -EAGAIN;
> 
> Can it be removed? Or, is there some reason why you can't have 
> asynchronous requests on the stack (such as inability of doing DMA to 
> virtually mapped stack)?

Right, in general you can't use stack requests for async hash
because they may DMA to the request memory.

> Or, should I just clear the flag CRYPTO_TFM_REQ_ON_STACK in my code?

That would not work in general because of DMA.

> The commit 04bfa4c7d5119ca38f8133bfdae7957a60c8b221 says that we should 
> clone the request with HASH_REQUEST_CLONE, but that is not usable in 
> dm-integrity, because dm-integrity must work even when the system is out 
> of memory.

The idea with HASH_REQUEST_CLONE is to fall back to software hashes
when the memory allocation fails.  This is transparent to dm-crypt
as the Crypto API will automatically switch to the fallback after
the failed CLONE call.  IOW you just write your code as if the CLONE
will always succeed.  It will simply replace the async hash with a
synchronous one in the failure case.

However, this doesn't even work for Harald's use case because it
can't have a software fallback as the hash key is stored in
hardware.

Of course Harald's case doesn't do DMA either, the only reason
it goes async is to wait for the hardware to become ready.

So what I can do is bypass the ahash_req_on_stack for Harald's
driver by changing the ahash_is_async test to something more
specific about DMA.  Let me write that up and I'll have something
for you to test in a couple of days.

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: crypto ahash requests on the stack

[Mikulas Patocka]

On Wed, 27 Aug 2025, Herbert Xu wrote:

> On Mon, Aug 25, 2025 at 04:23:59PM +0200, Mikulas Patocka wrote:
> > 
> > I'd like to ask about this condition in crypto_ahash_digest:
> > 	if (ahash_req_on_stack(req) && ahash_is_async(tfm))
> > 		return -EAGAIN;
> > 
> > Can it be removed? Or, is there some reason why you can't have 
> > asynchronous requests on the stack (such as inability of doing DMA to 
> > virtually mapped stack)?
> 
> Right, in general you can't use stack requests for async hash
> because they may DMA to the request memory.

Thanks for the confirmation.

BTW. what happens if you have an architecture that needs cacheline-aligned 
DMA accesses? For example, on parisc, some microarchitectures have 
128-byte cache line. As caches on parisc are not DMA-coherent, you must 
not touch the 128-byte region around the area where you are doing DMA.

Normally, this problem is solved by tweaking kmalloc, so that the minimum 
allocation size and alignment is 128 bytes. But if you do DMA into struct 
ahash_request, and struct ahash_request may be embedded in other 
structures, and doesn't have 128-byte alignment, it could break.

> So what I can do is bypass the ahash_req_on_stack for Harald's
> driver by changing the ahash_is_async test to something more
> specific about DMA.  Let me write that up and I'll have something
> for you to test in a couple of days.
> 
> Cheers,

I reworked my patchset so that it places asynchronous requests after the 
dm_integrity_io structure (that is allocated in directly mapped memory), 
so there are no longer any needs to change the crypto code.

Maybe I should allocate the ahash requests with kmalloc, to solve the 
DMA-into-request problem.

Mikulas

Re: crypto ahash requests on the stack

[Harald Freudenberger]

On 2025-08-25 16:23, Mikulas Patocka wrote:
> Hi
> 
> I'd like to ask about this condition in crypto_ahash_digest:
> 	if (ahash_req_on_stack(req) && ahash_is_async(tfm))
> 		return -EAGAIN;
> 
> Can it be removed? Or, is there some reason why you can't have
> asynchronous requests on the stack (such as inability of doing DMA to
> virtually mapped stack)?
> 
> Or, should I just clear the flag CRYPTO_TFM_REQ_ON_STACK in my code?
> 
> I'm modifying dm-integrity to use asynchronous API so that Harald
> Freudenberger can use it on mainframes (the reason is that his
> implementation only provides asynchronous API) and I would prefer to 
> place
> ahash requests on the stack (and wait for them before the function 
> exits).
> 
> The commit 04bfa4c7d5119ca38f8133bfdae7957a60c8b221 says that we should
> clone the request with HASH_REQUEST_CLONE, but that is not usable in
> dm-integrity, because dm-integrity must work even when the system is 
> out
> of memory.
> 
> Mikulas

The problem with this 'on the stack' is also with the buffer addresses.
The asynch implementations get scatterlists. By playing around there,
I found out that the addresses in scatterlists are checked:

scatterlist.h:

static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
			      unsigned int buflen)
{
#ifdef CONFIG_DEBUG_SG
	BUG_ON(!virt_addr_valid(buf));
#endif
	sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
}

and virt_addr_valid(x) fails on stack addresses (!). I talked with the
s390 subsystem maintainer and he confirms this. So stack addresses can't
be used for scatterlists even when there is no DMA involved.

Harald Freudenberger

Re: crypto ahash requests on the stack

[Herbert Xu]

On Tue, Sep 09, 2025 at 04:01:45PM +0200, Harald Freudenberger wrote:
>
> The problem with this 'on the stack' is also with the buffer addresses.
> The asynch implementations get scatterlists. By playing around there,
> I found out that the addresses in scatterlists are checked:

Right.  If we ever move to iov_iter this could be supported but
for now dynamic allocation is fine.

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