[PATCH] crypto/fsl: fsl_hash: Fix dcache issue in caam_hash_finish

[PATCH] crypto/fsl: fsl_hash: Fix dcache issue in caam_hash_finish

[Gaurav Jain]

HW accelerated hash operations are giving incorrect hash output.
so add flush and invalidate for input/output hash buffers.

Fixes: 94e3c8c4fd (crypto/fsl - Add progressive hashing support using hardware acceleration.)
Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
---
 drivers/crypto/fsl/fsl_hash.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/crypto/fsl/fsl_hash.c b/drivers/crypto/fsl/fsl_hash.c
index a52c4ac957..9e6829b7ad 100644
--- a/drivers/crypto/fsl/fsl_hash.c
+++ b/drivers/crypto/fsl/fsl_hash.c
@@ -149,12 +149,20 @@ static int caam_hash_finish(void *hash_ctx, void *dest_buf,
 				  driver_hash[caam_algo].digestsize,
 				  1);
 
+	flush_dcache_range((ulong)ctx->sg_tbl, (ulong)(ctx->sg_tbl) + len);
+	flush_dcache_range((ulong)ctx->sha_desc,
+			   (ulong)(ctx->sha_desc) + (sizeof(uint32_t) * MAX_CAAM_DESCSIZE));
+	flush_dcache_range((ulong)ctx->hash,
+			   (ulong)(ctx->hash) + driver_hash[caam_algo].digestsize);
+
 	ret = run_descriptor_jr(ctx->sha_desc);
 
 	if (ret) {
 		debug("Error %x\n", ret);
 		return ret;
 	} else {
+		invalidate_dcache_range((ulong)ctx->hash,
+					(ulong)(ctx->hash) + driver_hash[caam_algo].digestsize);
 		memcpy(dest_buf, ctx->hash, sizeof(ctx->hash));
 	}
 	free(ctx);
-- 
2.25.1

Re: [PATCH] crypto/fsl: fsl_hash: Fix dcache issue in caam_hash_finish

[Rasmus Villemoes]

On 11/05/2022 10.53, Gaurav Jain wrote:
> HW accelerated hash operations are giving incorrect hash output.
> so add flush and invalidate for input/output hash buffers.
> 
> Fixes: 94e3c8c4fd (crypto/fsl - Add progressive hashing support using hardware acceleration.)

AFAICT, it takes somewhat more to fix that commit; the progressive
hashing is entirely broken.

It doesn't actually do anything progressive, it just stashes the
address/length pairs it is given, but doesn't feed the contents of those
buffers to the hardware, folding it into the hash state. So the caller
must not touch the buffers it passes until the finalization. I.e. I
think this won't work:

  char buf[SOMETHING];

  update_buffer(buf);
  hash_update(buf, len);
  update_buffer_again(buf);
  hash_update(buf, len);

And this pattern can be found in e.g. drivers/dfu/dfu.c which seems to
repeatedly pass the same address (dfu->i_buf_start) to ->hash_update.

Am I reading the code wrong?

Rasmus

[PATCH] crypto/fsl: fsl_hash: Fix dcache issue in caam_hash_finish

[sbabic]

> HW accelerated hash operations are giving incorrect hash output.
> so add flush and invalidate for input/output hash buffers.
> Fixes: 94e3c8c4fd (crypto/fsl - Add progressive hashing support using hardware acceleration.)
> Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================

Re: [PATCH] crypto/fsl: fsl_hash: Fix dcache issue in caam_hash_finish

[Horia Geantă]

On 5/11/2022 4:55 PM, Rasmus Villemoes wrote:
> On 11/05/2022 10.53, Gaurav Jain wrote:
>> HW accelerated hash operations are giving incorrect hash output.
>> so add flush and invalidate for input/output hash buffers.
>>
>> Fixes: 94e3c8c4fd (crypto/fsl - Add progressive hashing support using hardware acceleration.)
> 
> AFAICT, it takes somewhat more to fix that commit; the progressive
> hashing is entirely broken.
> 
> It doesn't actually do anything progressive, it just stashes the
> address/length pairs it is given, but doesn't feed the contents of those
> buffers to the hardware, folding it into the hash state. So the caller
Correct.

> must not touch the buffers it passes until the finalization. I.e. I
> think this won't work:
> 
>   char buf[SOMETHING];
> 
>   update_buffer(buf);
>   hash_update(buf, len);
>   update_buffer_again(buf);
>   hash_update(buf, len);
> 
Indeed, this won't work.

> And this pattern can be found in e.g. drivers/dfu/dfu.c which seems to
> repeatedly pass the same address (dfu->i_buf_start) to ->hash_update.
> 
At first glance, dfu asks for crc32 algorithm, so it won't use this backend.
But the concept is correct: user is not forced to keep a buffer unmodified
(or even allocated) after .hash_update returns.

Thanks,
Horia