[BUG 0/2] crypto: ahash - Bugs introduced due to CRYPTO_AHASH_ALG_BLOCK_ONLY

[BUG 0/2] crypto: ahash - Bugs introduced due to CRYPTO_AHASH_ALG_BLOCK_ONLY

[T Pratham]

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.

I've discovered 2 bugs introduced due to the above commit:

[Bug 1/2]: import/export fails for algs using CRYPTO_AHASH_ALG_BLOCK_ONLY
[BUG 2/2]: testmgr false failures with CRYPTO_AHASH_ALG_BLOCK_ONLY

These are detailed in the following messages.
-- 
Regards
T Pratham <t-pratham@ti.com>

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

[T Pratham]

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>

[BUG 2/2] crypto: ahash - testmgr false failures with CRYPTO_AHASH_ALG_BLOCK_ONLY

[T Pratham]

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.

It appears that the testmgr is not updated to handle this positive return value
from update(). As a result, self-tests fail (falsely) for such ahash
algorithms. Below are dmesg logs from my (work-in-progress) TI dthev2 hash
driver:

[   12.933924] alg: ahash: sha256-dthev2 update() failed with err 62 on test vector 4, cfg="init+update+final aligned buffer"
[   12.951212] alg: self-tests for sha256 using sha256-dthev2 failed (rc=62)
[   13.008108] alg: self-tests for sha256 using sha256-dthev2 failed (rc=62)
[   13.361625] alg: ahash: sha512-dthev2 update() failed with err 126 on test vector 5, cfg="init+update+final aligned buffer"
[   13.376311] alg: self-tests for sha512 using sha512-dthev2 failed (rc=126)
[   13.388088] alg: self-tests for sha512 using sha512-dthev2 failed (rc=126)
[   13.389503] alg: ahash: md5-dthev2 update() failed with err 15 on test vector 6, cfg="init+update+final aligned buffer"
[   13.421488] alg: self-tests for md5 using md5-dthev2 failed (rc=15)
[   13.478062] alg: self-tests for md5 using md5-dthev2 failed (rc=15)

Note: the driver works completely fine on a TI internal v6.12 LTS tree where
this buffering is being handled by the driver itself.

Now, while debugging the issue, this messy fixup in testmgr code works fine and
my ahash driver passes.

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 6a490aaa71b9a..523507e79f760 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -34,6 +34,7 @@
 #include <crypto/kpp.h>
 #include <crypto/acompress.h>
 #include <crypto/sig.h>
+#include <crypto/internal/hash.h>
 #include <crypto/internal/cipher.h>
 #include <crypto/internal/simd.h>
 
@@ -1571,9 +1572,16 @@ static int test_ahash_vec_cfg(const struct hash_testvec *vec,
                                                pending_len);
                        err = do_ahash_op(crypto_ahash_update, req, &wait,
                                          divs[i]->nosimd);
-                       err = check_nonfinal_ahash_op("update", err,
-                                                     result, digestsize,
-                                                     driver, vec_name, cfg);
+                       if ((crypto_ahash_alg(tfm)->halg.base.cra_flags &
+                           CRYPTO_AHASH_ALG_BLOCK_ONLY) &&
+                           err >= 0)
+                               err = check_nonfinal_ahash_op("update", 0,
+                                                             result, digestsize,
+                                                             driver, vec_name, cfg);
+                       else
+                               err = check_nonfinal_ahash_op("update", err,
+                                                             result, digestsize,
+                                                             driver, vec_name, cfg);
                        if (err)
                                return err;
                        pending_sgl = NULL;
@@ -1614,8 +1622,14 @@ static int test_ahash_vec_cfg(const struct hash_testvec *vec,
        if (cfg->finalization_type == FINALIZATION_TYPE_FINAL) {
                /* finish with update() and final() */
                err = do_ahash_op(crypto_ahash_update, req, &wait, cfg->nosimd);
-               err = check_nonfinal_ahash_op("update", err, result, digestsize,
-                                             driver, vec_name, cfg);
+               if ((crypto_ahash_alg(tfm)->halg.base.cra_flags &
+                   CRYPTO_AHASH_ALG_BLOCK_ONLY) &&
+                   err >= 0)
+                       err = check_nonfinal_ahash_op("update", 0, result, digestsize,
+                                                     driver, vec_name, cfg);
+               else
+                       err = check_nonfinal_ahash_op("update", err, result, digestsize,
+                                                     driver, vec_name, cfg);
                if (err)
                        return err;
                err = do_ahash_op(crypto_ahash_final, req, &wait, cfg->nosimd);

While I have not tested any shash code with CRYPTO_AHASH_ALG_BLOCK_ONLY, it is
highly possible that testmgr will report a false negative there as well.

I can send a fix for ahash. Let me know if it is better to just add a separate
check in the testmgr (in which case, I'll cleanup the above diff and send as a
separate patch), or should we modify the crypto_ahash_update() function in
crypto/ahash.c to return 0 as it handles all the buffer management inside
itself. The callers of crypto_ahash_update() anyway don't expect a non-zero
positive value on success as far as I can see. 

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

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

[Herbert Xu]

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

[PATCH] crypto: ahash - Zero positive err value in ahash_update_finish

[Herbert Xu]

On Thu, Nov 13, 2025 at 07:30:13PM +0530, T Pratham wrote:
> 
> 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.

Thanks for the report!

This is a bug in the ahash API code, it should return zero instead
of the positive value.

---8<---
The partial block length returned by a block-only driver should
not be passed up to the caller since ahash itself deals with the
partial block data.

Set err to zero in ahash_update_finish if it was positive.

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..e3d0736e9afe 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -423,7 +423,11 @@ static int ahash_update_finish(struct ahash_request *req, int err)
 
 	req->nbytes += nonzero - blen;
 
-	blen = err < 0 ? 0 : err + nonzero;
+	blen = 0;
+	if (err >= 0) {
+		blen = err + nonzero;
+		err = 0;
+	}
 	if (ahash_request_isvirt(req))
 		memcpy(buf, req->svirt + req->nbytes - blen, blen);
 	else
-- 
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

[T Pratham]

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>

Re: [PATCH] crypto: ahash - Zero positive err value in ahash_update_finish

[T Pratham]

On 21/11/25 11:24, Herbert Xu wrote:
> On Thu, Nov 13, 2025 at 07:30:13PM +0530, T Pratham wrote:
> The partial block length returned by a block-only driver should
> not be passed up to the caller since ahash itself deals with the
> partial block data.
> 
> Set err to zero in ahash_update_finish if it was positive.
> 
> 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>