crypto: ahash - Bugs introduced due to CRYPTO_AHASH_ALG_BLOCK_ONLY

[BUG 1/2] crypto: ahash - import/export fails for algs using CRYPTO_AHASH_ALG_BLOCK_ONLY

Posted by T Pratham 2 months, 3 weeks ago

Hi,

Commit 9d7a0ab1c7536 ("crypto: ahash - Handle partial blocks in API")
introduced partial block handling for ahashes in the crypto API layer itself.
This enables ahash algorithms to return a positive integer from the update
function to indicate the number of bytes in the input which are not processed
and should be buffered for next update/finup/final call to process.

When CRYPTO_AHASH_ALG_BLOCK_ONLY is enabled to let crypto layer handle the
buffering for ahashes, it appears that the import/export for ahashes is broken.
Below are dmesg logs from my (work-in-progress) TI dthev2 hash driver:

[   12.713654] alg: ahash: sha256-dthev2 test failed (wrong result) on test vector 1, cfg="import/export"
[   12.735778] alg: ahash: sha512-dthev2 test failed (wrong result) on test vector 1, cfg="import/export"
[   12.740207] alg: self-tests for sha256 using sha256-dthev2 failed (rc=-22)
[   12.752055] alg: self-tests for sha256 using sha256-dthev2 failed (rc=-22)
[   12.760127] alg: self-tests for sha512 using sha512-dthev2 failed (rc=-22)
[   12.847079] alg: self-tests for sha512 using sha512-dthev2 failed (rc=-22)
[   13.632318] alg: ahash: md5-dthev2 test failed (wrong result) on test vector 2, cfg="import/export"
[   13.643908] alg: self-tests for md5 using md5-dthev2 failed (rc=-22)
[   13.655051] alg: self-tests for md5 using md5-dthev2 failed (rc=-22)

While debugging, I noticed the rather odd asymmetrical handling of the buffer
which stores the partial block in crypto_ahash_export vs crypto_ahash_import in
crypto/ahash.c: 

Export:
> if (crypto_ahash_block_only(tfm)) {
> 		unsigned int plen = crypto_ahash_blocksize(tfm) + 1;
> 		unsigned int reqsize = crypto_ahash_reqsize(tfm);
> 		unsigned int ss = crypto_ahash_statesize(tfm);
> 		u8 *buf = ahash_request_ctx(req);
> 
> 		memcpy(out + ss - plen, buf + reqsize - plen, plen);
> 	}

Import:
> if (crypto_ahash_block_only(tfm)) {
> 		unsigned int reqsize = crypto_ahash_reqsize(tfm);
> 		u8 *buf = ahash_request_ctx(req);
> 
> 		buf[reqsize - 1] = 0;
> 	}

Import seemingly not copying the buffer back to request ctx and zeroing the
length seems incorrect. Making export and import symmetrical seems to work and
my driver passes through import/export tests.

diff --git a/crypto/ahash.c b/crypto/ahash.c
index dfb4f5476428f..9510bdeda51de 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -674,10 +674,12 @@ int crypto_ahash_import(struct ahash_request *req, const void *in)
        if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
                return -ENOKEY;
        if (crypto_ahash_block_only(tfm)) {
+               unsigned int plen = crypto_ahash_blocksize(tfm) + 1;
                unsigned int reqsize = crypto_ahash_reqsize(tfm);
+               unsigned int ss = crypto_ahash_statesize(tfm);
                u8 *buf = ahash_request_ctx(req);
 
-               buf[reqsize - 1] = 0;
+               memcpy(buf + reqsize - plen, in + ss - plen, plen);
        }
        return crypto_ahash_alg(tfm)->import(req, in);
 }

Is there any particular reason why import is like how it is currently? As per
my understanding import should reverse whatever export is doing and vice-versa.

It is also noteworthy that similarly import/export could be broken for shash
algorithms as well. There also, import function zeros the length byte and does
not copy the buffer when CRYPTO_AHASH_ALG_BLOCK_ONLY is used. However I haven't
tested any shash algorithm to verify this.

Let me know if the above is the complete fix for ahash, so that I can send it
as a patch.

-- 
Regards
T Pratham <t-pratham@ti.com>

[PATCH] crypto: ahash - Fix crypto_ahash_import with partial block data

Posted by Herbert Xu 2 months, 2 weeks ago

On Thu, Nov 13, 2025 at 07:30:12PM +0530, T Pratham wrote:
>
> diff --git a/crypto/ahash.c b/crypto/ahash.c
> index dfb4f5476428f..9510bdeda51de 100644
> --- a/crypto/ahash.c
> +++ b/crypto/ahash.c
> @@ -674,10 +674,12 @@ int crypto_ahash_import(struct ahash_request *req, const void *in)
>         if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
>                 return -ENOKEY;
>         if (crypto_ahash_block_only(tfm)) {
> +               unsigned int plen = crypto_ahash_blocksize(tfm) + 1;
>                 unsigned int reqsize = crypto_ahash_reqsize(tfm);
> +               unsigned int ss = crypto_ahash_statesize(tfm);
>                 u8 *buf = ahash_request_ctx(req);
>  
> -               buf[reqsize - 1] = 0;
> +               memcpy(buf + reqsize - plen, in + ss - plen, plen);
>         }
>         return crypto_ahash_alg(tfm)->import(req, in);
>  }
> 
> Is there any particular reason why import is like how it is currently? As per
> my understanding import should reverse whatever export is doing and vice-versa.

Thanks, you're right that this is broken.

The zeroing of the partial block buffer should be in import_core
instead of import.

---8<---
Restore the partial block buffer in crypto_ahash_import by copying
it.  Check whether the partial block buffer exceeds the maximum
size and return -EOVERFLOW if it does.

Zero the partial block buffer in crypto_ahash_import_core.

Reported-by: T Pratham <t-pratham@ti.com>
Fixes: 9d7a0ab1c753 ("crypto: ahash - Handle partial blocks in API")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/ahash.c b/crypto/ahash.c
index dfb4f5476428..819b484a1a00 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -661,6 +661,12 @@ int crypto_ahash_import_core(struct ahash_request *req, const void *in)
 						in);
 	if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
 		return -ENOKEY;
+	if (crypto_ahash_block_only(tfm)) {
+		unsigned int reqsize = crypto_ahash_reqsize(tfm);
+		u8 *buf = ahash_request_ctx(req);
+
+		buf[reqsize - 1] = 0;
+	}
 	return crypto_ahash_alg(tfm)->import_core(req, in);
 }
 EXPORT_SYMBOL_GPL(crypto_ahash_import_core);
@@ -674,10 +680,14 @@ int crypto_ahash_import(struct ahash_request *req, const void *in)
 	if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
 		return -ENOKEY;
 	if (crypto_ahash_block_only(tfm)) {
+		unsigned int plen = crypto_ahash_blocksize(tfm) + 1;
 		unsigned int reqsize = crypto_ahash_reqsize(tfm);
+		unsigned int ss = crypto_ahash_statesize(tfm);
 		u8 *buf = ahash_request_ctx(req);
 
-		buf[reqsize - 1] = 0;
+		memcpy(buf + reqsize - plen, in + ss - plen, plen);
+		if (buf[reqsize - 1] >= plen)
+			return -EOVERFLOW;
 	}
 	return crypto_ahash_alg(tfm)->import(req, in);
 }
-- 
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: [PATCH] crypto: ahash - Fix crypto_ahash_import with partial block data

Posted by T Pratham 2 months, 2 weeks ago

On 21/11/25 11:06, Herbert Xu wrote:
> On Thu, Nov 13, 2025 at 07:30:12PM +0530, T Pratham wrote:
>>
> Restore the partial block buffer in crypto_ahash_import by copying
> it.  Check whether the partial block buffer exceeds the maximum
> size and return -EOVERFLOW if it does.
> 
> Zero the partial block buffer in crypto_ahash_import_core.
> 
> Reported-by: T Pratham <t-pratham@ti.com>
> Fixes: 9d7a0ab1c753 ("crypto: ahash - Handle partial blocks in API")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 

This fixes the issue.

Tested-by: T Pratham <t-pratham@ti.com>
-- 
Regards
T Pratham <t-pratham@ti.com>