[PATCH] stm32: stm32-hash: Add kmalloc_array allocation check

[PATCH] stm32: stm32-hash: Add kmalloc_array allocation check

[Li kunyu]

If rctx->hw_context allocation of the context pointer failed. Returning
-ENOMEM and assigning NULL to the out pointer should improve the
robustness of the function.

Signed-off-by: Li kunyu <kunyu@nfschina.com>
---
 drivers/crypto/stm32/stm32-hash.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index d33006d43f76..18e79f62be96 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -970,6 +970,10 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 	rctx->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
 					 sizeof(u32),
 					 GFP_KERNEL);
+	if (rctx->hw_context == NULL) {
+		out = NULL;
+		return -ENOMEM;
+	}
 
 	preg = rctx->hw_context;
 
-- 
2.18.2

Re: [PATCH] stm32: stm32-hash: Add kmalloc_array allocation check

[Herbert Xu]

On Sat, Feb 25, 2023 at 05:50:19AM +0800, Li kunyu wrote:
> If rctx->hw_context allocation of the context pointer failed. Returning
> -ENOMEM and assigning NULL to the out pointer should improve the
> robustness of the function.
> 
> Signed-off-by: Li kunyu <kunyu@nfschina.com>
> ---
>  drivers/crypto/stm32/stm32-hash.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
> index d33006d43f76..18e79f62be96 100644
> --- a/drivers/crypto/stm32/stm32-hash.c
> +++ b/drivers/crypto/stm32/stm32-hash.c
> @@ -970,6 +970,10 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
>  	rctx->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
>  					 sizeof(u32),
>  					 GFP_KERNEL);
> +	if (rctx->hw_context == NULL) {
> +		out = NULL;
> +		return -ENOMEM;
> +	}
>  
>  	preg = rctx->hw_context;

Shouldn't we free the hw_context at the end of the function?

Why does it even need to be stored in rctx in the first place?

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

Re: [PATCH] stm32: stm32-hash: Add kmalloc_array allocation check

[Li kunyu]

hello senior:
  'rctx->hw_context' is a member of the rctx pointer array object and will pass 'memcpy(out, rctx, sizeof(*rctx)) at the end of the function;  ', copied to the out pointer memory, it cannot be released, I think this patch does an allocation check on the 'rctx->hw_context' pointer should help a bit.

Re: [PATCH] stm32: stm32-hash: Add kmalloc_array allocation check

[Herbert Xu]

On Sat, Feb 25, 2023 at 07:14:29AM +0800, Li kunyu wrote:
> 
> hello senior:
>   'rctx->hw_context' is a member of the rctx pointer array object and will pass 'memcpy(out, rctx, sizeof(*rctx)) at the end of the function;  ', copied to the out pointer memory, it cannot be released, I think this patch does an allocation check on the 'rctx->hw_context' pointer should help a bit.

Ugh that's completely broken.  You can't export pointers.

I guess this driver needs to be disabled.

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

Re: [PATCH] stm32: stm32-hash: Add kmalloc_array allocation check

[Linus Walleij]

On Thu, Feb 23, 2023 at 10:33 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Sat, Feb 25, 2023 at 07:14:29AM +0800, Li kunyu wrote:
> >
> > hello senior:
> >   'rctx->hw_context' is a member of the rctx pointer array object and will pass 'memcpy(out, rctx, sizeof(*rctx)) at the end of the function;  ', copied to the out pointer memory, it cannot be released, I think this patch does an allocation check on the 'rctx->hw_context' pointer should help a bit.
>
> Ugh that's completely broken.  You can't export pointers.
>
> I guess this driver needs to be disabled.

Added Lionel to the To: line so he can look at this, I guess we should
also add an entry to MAINTAINERS so that the reports come to the right
address.

Can we fix the actual problem? It seems to have been there since the
initial submission in 2017.

I guess the right fix is to export the *actual* hardware state into "out"
and read it back from there instead of copying out the rctx struct.
Also .statesize needs to be fixed to correspond to that.
I can just use a roof:ed constant size for this.

If Lionel does not have time to look at it I can try to come up with a
quick fix.

Yours,
Linus Walleij

[PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Thu, Feb 23, 2023 at 10:52:17AM +0100, Linus Walleij wrote:
>
> Can we fix the actual problem? It seems to have been there since the
> initial submission in 2017.
> 
> I guess the right fix is to export the *actual* hardware state into "out"
> and read it back from there instead of copying out the rctx struct.
> Also .statesize needs to be fixed to correspond to that.
> I can just use a roof:ed constant size for this.

Indeed.  It looks like the driver already has everything we need
but it's just in the wrong place.

Here's my completely untested patch to move the hardware state
reading/writing to where I think it should be.  As it stands,
not only is export/import broken, but multiple hashing requests
occuring concurrently will overwrite each other's state.

---8<---
The Crypto API hashing paradigm requires the hardware state to
be exported between *each* request because multiple unrelated
hashes may be processed concurrently.

The stm32 hardware is capable of producing the hardware hashing
state but it was only doing it in the export function.  This is
not only broken for export as you can't export a kernel pointer
and reimport it, but it also means that concurrent hashing was
fundamentally broken.

Fix this by moving the saving and restoring of hardware hash
state between each and every hashing request.

Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
Reported-by: Li kunyu <kunyu@nfschina.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 7bf805563ac2..4bc69677a861 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -148,11 +148,12 @@ struct stm32_hash_request_ctx {
 	int			nents;
 
 	u8			data_type;
+	bool			started;
 
 	u8 buffer[HASH_BUFLEN] __aligned(sizeof(u32));
 
-	/* Export Context */
-	u32			*hw_context;
+	/* hash state */
+	u32			hw_context[3 + HASH_CSR_REGISTER_NUMBER];
 };
 
 struct stm32_hash_algs_info {
@@ -441,8 +442,20 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
 			err = 0;
 		}
+	} else {
+		u32 *preg = rctx->hw_context;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			*preg++ = stm32_hash_read(hdev, HASH_IMR);
+		*preg++ = stm32_hash_read(hdev, HASH_STR);
+		*preg++ = stm32_hash_read(hdev, HASH_CR);
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
 	}
 
+	rctx->started = !final;
+
 	return err;
 }
 
@@ -754,6 +767,7 @@ static int stm32_hash_init(struct ahash_request *req)
 	rctx->total = 0;
 	rctx->offset = 0;
 	rctx->data_type = HASH_DATA_8_BITS;
+	rctx->started = false;
 
 	memset(rctx->buffer, 0, HASH_BUFLEN);
 
@@ -959,6 +973,22 @@ static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 
 	rctx = ahash_request_ctx(req);
 
+	if (rctx->started) {
+		u32 *preg = rctx->hw_context;
+		u32 reg;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			stm32_hash_write(hdev, HASH_IMR, *preg++);
+		stm32_hash_write(hdev, HASH_STR, *preg++);
+		stm32_hash_write(hdev, HASH_CR, *preg);
+		reg = *preg++ | HASH_CR_INIT;
+		stm32_hash_write(hdev, HASH_CR, reg);
+
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			stm32_hash_write(hdev, HASH_CSR(i), *preg++);
+	}
+
 	if (rctx->op == HASH_OP_UPDATE)
 		err = stm32_hash_update_req(hdev);
 	else if (rctx->op == HASH_OP_FINAL)
@@ -1044,33 +1074,6 @@ static int stm32_hash_digest(struct ahash_request *req)
 static int stm32_hash_export(struct ahash_request *req, void *out)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	u32 *preg;
-	unsigned int i;
-	int ret;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	ret = stm32_hash_wait_busy(hdev);
-	if (ret)
-		return ret;
-
-	rctx->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					 sizeof(u32),
-					 GFP_KERNEL);
-
-	preg = rctx->hw_context;
-
-	if (!hdev->pdata->ux500)
-		*preg++ = stm32_hash_read(hdev, HASH_IMR);
-	*preg++ = stm32_hash_read(hdev, HASH_STR);
-	*preg++ = stm32_hash_read(hdev, HASH_CR);
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
 
 	memcpy(out, rctx, sizeof(*rctx));
 
@@ -1080,33 +1083,9 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 static int stm32_hash_import(struct ahash_request *req, const void *in)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	const u32 *preg = in;
-	u32 reg;
-	unsigned int i;
 
 	memcpy(rctx, in, sizeof(*rctx));
 
-	preg = rctx->hw_context;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	if (!hdev->pdata->ux500)
-		stm32_hash_write(hdev, HASH_IMR, *preg++);
-	stm32_hash_write(hdev, HASH_STR, *preg++);
-	stm32_hash_write(hdev, HASH_CR, *preg);
-	reg = *preg++ | HASH_CR_INIT;
-	stm32_hash_write(hdev, HASH_CR, reg);
-
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		stm32_hash_write(hdev, HASH_CSR(i), *preg++);
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
-
-	kfree(rctx->hw_context);
-
 	return 0;
 }
 
-- 
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

[v2 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

v2 fixes potential state clobbering from the disconnect between
hdev->flags and rctx->flags.

---8<---
The Crypto API hashing paradigm requires the hardware state to
be exported between *each* request because multiple unrelated
hashes may be processed concurrently.

The stm32 hardware is capable of producing the hardware hashing
state but it was only doing it in the export function.  This is
not only broken for export as you can't export a kernel pointer
and reimport it, but it also means that concurrent hashing was
fundamentally broken.

Fix this by moving the saving and restoring of hardware hash
state between each and every hashing request.

Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
Reported-by: Li kunyu <kunyu@nfschina.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index acf8bfc8de4b..523312f47166 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -152,8 +152,8 @@ struct stm32_hash_request_ctx {
 
 	u8 buffer[HASH_BUFLEN] __aligned(sizeof(u32));
 
-	/* Export Context */
-	u32			*hw_context;
+	/* hash state */
+	u32			hw_context[3 + HASH_CSR_REGISTER_NUMBER];
 };
 
 struct stm32_hash_algs_info {
@@ -184,7 +184,6 @@ struct stm32_hash_dev {
 	struct ahash_request	*req;
 	struct crypto_engine	*engine;
 
-	int			err;
 	unsigned long		flags;
 
 	struct dma_chan		*dma_lch;
@@ -442,6 +441,18 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
 			err = 0;
 		}
+	} else {
+		u32 *preg = rctx->hw_context;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			*preg++ = stm32_hash_read(hdev, HASH_IMR);
+		*preg++ = stm32_hash_read(hdev, HASH_STR);
+		*preg++ = stm32_hash_read(hdev, HASH_CR);
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
+
+		rctx->flags |= HASH_FLAGS_INIT;
 	}
 
 	return err;
@@ -884,11 +895,6 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
 		stm32_hash_copy_hash(req);
 		err = stm32_hash_finish(req);
-		hdev->flags &= ~(HASH_FLAGS_FINAL | HASH_FLAGS_CPU |
-				 HASH_FLAGS_INIT | HASH_FLAGS_DMA_READY |
-				 HASH_FLAGS_OUTPUT_READY | HASH_FLAGS_HMAC |
-				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
-				 HASH_FLAGS_HMAC_KEY);
 	} else {
 		rctx->flags |= HASH_FLAGS_ERRORS;
 	}
@@ -899,68 +905,58 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	crypto_finalize_hash_request(hdev->engine, req, err);
 }
 
-static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
+static void stm32_hash_hw_init(struct stm32_hash_dev *hdev,
 			      struct stm32_hash_request_ctx *rctx)
 {
 	pm_runtime_get_sync(hdev->dev);
-
-	if (!(HASH_FLAGS_INIT & hdev->flags)) {
-		stm32_hash_write(hdev, HASH_CR, HASH_CR_INIT);
-		stm32_hash_write(hdev, HASH_STR, 0);
-		stm32_hash_write(hdev, HASH_DIN, 0);
-		stm32_hash_write(hdev, HASH_IMR, 0);
-		hdev->err = 0;
-	}
-
-	return 0;
 }
 
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq);
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq);
-
 static int stm32_hash_handle_queue(struct stm32_hash_dev *hdev,
 				   struct ahash_request *req)
 {
 	return crypto_transfer_hash_request_to_engine(hdev->engine, req);
 }
 
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq)
+static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 {
 	struct ahash_request *req = container_of(areq, struct ahash_request,
 						 base);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
 	struct stm32_hash_request_ctx *rctx;
+	int err = 0;
 
 	if (!hdev)
 		return -ENODEV;
 
-	hdev->req = req;
-
-	rctx = ahash_request_ctx(req);
-
 	dev_dbg(hdev->dev, "processing new req, op: %lu, nbytes %d\n",
 		rctx->op, req->nbytes);
 
-	return stm32_hash_hw_init(hdev, rctx);
-}
-
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
-{
-	struct ahash_request *req = container_of(areq, struct ahash_request,
-						 base);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_request_ctx *rctx;
-	int err = 0;
-
-	if (!hdev)
-		return -ENODEV;
+	stm32_hash_hw_init(hdev, rctx);
 
 	hdev->req = req;
+	hdev->flags = 0;
 
 	rctx = ahash_request_ctx(req);
 
+	if (rctx->flags & HASH_FLAGS_INIT) {
+		u32 *preg = rctx->hw_context;
+		u32 reg;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			stm32_hash_write(hdev, HASH_IMR, *preg++);
+		stm32_hash_write(hdev, HASH_STR, *preg++);
+		stm32_hash_write(hdev, HASH_CR, *preg);
+		reg = *preg++ | HASH_CR_INIT;
+		stm32_hash_write(hdev, HASH_CR, reg);
+
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			stm32_hash_write(hdev, HASH_CSR(i), *preg++);
+
+		hdev->flags |= HASH_FLAGS_INIT;
+	}
+
 	if (rctx->op == HASH_OP_UPDATE)
 		err = stm32_hash_update_req(hdev);
 	else if (rctx->op == HASH_OP_FINAL)
@@ -1048,33 +1044,6 @@ static int stm32_hash_digest(struct ahash_request *req)
 static int stm32_hash_export(struct ahash_request *req, void *out)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	u32 *preg;
-	unsigned int i;
-	int ret;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	ret = stm32_hash_wait_busy(hdev);
-	if (ret)
-		return ret;
-
-	rctx->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					 sizeof(u32),
-					 GFP_KERNEL);
-
-	preg = rctx->hw_context;
-
-	if (!hdev->pdata->ux500)
-		*preg++ = stm32_hash_read(hdev, HASH_IMR);
-	*preg++ = stm32_hash_read(hdev, HASH_STR);
-	*preg++ = stm32_hash_read(hdev, HASH_CR);
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
 
 	memcpy(out, rctx, sizeof(*rctx));
 
@@ -1084,33 +1053,9 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 static int stm32_hash_import(struct ahash_request *req, const void *in)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	const u32 *preg = in;
-	u32 reg;
-	unsigned int i;
 
 	memcpy(rctx, in, sizeof(*rctx));
 
-	preg = rctx->hw_context;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	if (!hdev->pdata->ux500)
-		stm32_hash_write(hdev, HASH_IMR, *preg++);
-	stm32_hash_write(hdev, HASH_STR, *preg++);
-	stm32_hash_write(hdev, HASH_CR, *preg);
-	reg = *preg++ | HASH_CR_INIT;
-	stm32_hash_write(hdev, HASH_CR, reg);
-
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		stm32_hash_write(hdev, HASH_CSR(i), *preg++);
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
-
-	kfree(rctx->hw_context);
-
 	return 0;
 }
 
@@ -1166,8 +1111,6 @@ static int stm32_hash_cra_init_algs(struct crypto_tfm *tfm,
 		ctx->flags |= HASH_FLAGS_HMAC;
 
 	ctx->enginectx.op.do_one_request = stm32_hash_one_request;
-	ctx->enginectx.op.prepare_request = stm32_hash_prepare_req;
-	ctx->enginectx.op.unprepare_request = NULL;
 
 	return stm32_hash_init_fallback(tfm);
 }
-- 
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: [v2 PATCH] crypto: stm32 - Save and restore between each request

[Linus Walleij]

Hi Herbert,

I tested this on the Ux500 and sadly this happens
already in probe():

[    2.802539] stm32-hash a03c2000.hash: dma-maxburst not specified, using 0
[    2.809342] stm32-hash a03c2000.hash: No IRQ, use polling mode
[    2.815226] stm32-hash a03c2000.hash: DMA mode not available
[    2.821140] stm32-hash a03c2000.hash: will run requests pump with
realtime priority
[    2.828815] stm32-hash a03c2000.hash: Algo 0 : 0 failed
[    2.834144] stm32-hash: probe of a03c2000.hash failed with error -22

I don't quite understand why, we never get to the tests...

On Fri, Feb 24, 2023 at 6:52 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> v2 fixes potential state clobbering from the disconnect between
> hdev->flags and rctx->flags.
>
> ---8<---
> The Crypto API hashing paradigm requires the hardware state to
> be exported between *each* request because multiple unrelated
> hashes may be processed concurrently.
>
> The stm32 hardware is capable of producing the hardware hashing
> state but it was only doing it in the export function.  This is
> not only broken for export as you can't export a kernel pointer
> and reimport it, but it also means that concurrent hashing was
> fundamentally broken.
>
> Fix this by moving the saving and restoring of hardware hash
> state between each and every hashing request.
>
> Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
> Reported-by: Li kunyu <kunyu@nfschina.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

I think I understand the direction of the patch but it seems
the pm_runtime_* calls get unbalanced since now this is taken
in
stm32_hash_one_request
 -> stm32_hash_hw_init()
    -> pm_runtime_get_sync()

But no corresponding
pm_runtime_mark_last_busy(hdev->dev);
pm_runtime_put_autosuspend(hdev->dev);

Exist anymore? You know the semantics better than me,
I'm not sure where to put this, I guess where you save
the HW state in stm32_hash_update_cpu()?

I don't know about the DMA case then though.

Yours,
Linus Walleij

Re: [v2 PATCH] crypto: stm32 - Save and restore between each request

[Li kunyu]

hello senior:
    I understand the new patch and probably know where to add these two functions.
 
> +static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)

+	if (rctx->flags & HASH_FLAGS_INIT) {
+		u32 *preg = rctx->hw_context;
+		u32 reg;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			stm32_hash_write(hdev, HASH_IMR, *preg++);
+		stm32_hash_write(hdev, HASH_STR, *preg++);
+		stm32_hash_write(hdev, HASH_CR, *preg);
+		reg = *preg++ | HASH_CR_INIT;
+		stm32_hash_write(hdev, HASH_CR, reg);
+
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			stm32_hash_write(hdev, HASH_CSR(i), *preg++);
+
+		hdev->flags |= HASH_FLAGS_INIT;
-------------------------------------------------------------
Add functions:
		pm_runtime_mark_last_busy(hdev->dev);
        	pm_runtime_put_autosuspend(hdev->dev);
-------------------------------------------------------------
+	}
+

There is another place:

> @@ -442,6 +441,18 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
 			err = 0;
 		}
+	} else {
+		u32 *preg = rctx->hw_context;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			*preg++ = stm32_hash_read(hdev, HASH_IMR);
+		*preg++ = stm32_hash_read(hdev, HASH_STR);
+		*preg++ = stm32_hash_read(hdev, HASH_CR);
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
+
+		rctx->flags |= HASH_FLAGS_INIT;
-------------------------------------------------------------
Add functions:
		pm_runtime_mark_last_busy(hdev->dev);
        	pm_runtime_put_autosuspend(hdev->dev);
-------------------------------------------------------------
 	}

Re: [v2 PATCH] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Sat, Feb 25, 2023 at 1:01 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> I tested this on the Ux500 and sadly this happens
> already in probe():
(...)
> [    2.828815] stm32-hash a03c2000.hash: Algo 0 : 0 failed
> [    2.834144] stm32-hash: probe of a03c2000.hash failed with error -22

It turns out that this is because this:

-       /* Export Context */
-       u32                     *hw_context;
+       /* hash state */
+       u32                     hw_context[3 + HASH_CSR_REGISTER_NUMBER];

Makes struct stm32_hash_request_ctx 580 bytes
and that fails sanity check in ahash.c because
HASH_MAX_STATESIZE is 512.

I don't know the story behind why HASH_MAX_STATESIZE
is 512, the stm32 hash state contains a buffer of 256 bytes
so I guess either that buffer is a bit big or
HASH_MAX_STATESIZE is a bit small?

I'm happy to try to change either to make this fit.

Yours,
Linus Walleij

[v3 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

v3 moves the exported state into its own structure so that it
doesn't contain unnecessary data.  It also subsumes the "empty
message" patch, as only the stm32_hash_copy_hash hunk was really
needed and it's easiest if we put that here.

---8<---
The Crypto API hashing paradigm requires the hardware state to
be exported between *each* request because multiple unrelated
hashes may be processed concurrently.

The stm32 hardware is capable of producing the hardware hashing
state but it was only doing it in the export function.  This is
not only broken for export as you can't export a kernel pointer
and reimport it, but it also means that concurrent hashing was
fundamentally broken.

Fix this by moving the saving and restoring of hardware hash
state between each and every hashing request.

Also change the emptymsg check in stm32_hash_copy_hash to rely
on whether we have any existing hash state, rather than whether
this particular update request is empty.

Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
Reported-by: Li kunyu <kunyu@nfschina.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 7bf805563ac2..72806daab600 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -127,6 +127,16 @@ struct stm32_hash_ctx {
 	int			keylen;
 };
 
+struct stm32_hash_state {
+	u16			bufcnt;
+	u16			buflen;
+
+	u8 buffer[HASH_BUFLEN] __aligned(4);
+
+	/* hash state */
+	u32			hw_context[3 + HASH_CSR_REGISTER_NUMBER];
+};
+
 struct stm32_hash_request_ctx {
 	struct stm32_hash_dev	*hdev;
 	unsigned long		flags;
@@ -134,8 +144,6 @@ struct stm32_hash_request_ctx {
 
 	u8 digest[SHA256_DIGEST_SIZE] __aligned(sizeof(u32));
 	size_t			digcnt;
-	size_t			bufcnt;
-	size_t			buflen;
 
 	/* DMA */
 	struct scatterlist	*sg;
@@ -149,10 +157,7 @@ struct stm32_hash_request_ctx {
 
 	u8			data_type;
 
-	u8 buffer[HASH_BUFLEN] __aligned(sizeof(u32));
-
-	/* Export Context */
-	u32			*hw_context;
+	struct stm32_hash_state state;
 };
 
 struct stm32_hash_algs_info {
@@ -183,7 +188,6 @@ struct stm32_hash_dev {
 	struct ahash_request	*req;
 	struct crypto_engine	*engine;
 
-	int			err;
 	unsigned long		flags;
 
 	struct dma_chan		*dma_lch;
@@ -326,11 +330,12 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 
 static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 {
+	struct stm32_hash_state *state = &rctx->state;
 	size_t count;
 
-	while ((rctx->bufcnt < rctx->buflen) && rctx->total) {
+	while ((state->bufcnt < state->buflen) && rctx->total) {
 		count = min(rctx->sg->length - rctx->offset, rctx->total);
-		count = min(count, rctx->buflen - rctx->bufcnt);
+		count = min_t(size_t, count, state->buflen - state->bufcnt);
 
 		if (count <= 0) {
 			if ((rctx->sg->length == 0) && !sg_is_last(rctx->sg)) {
@@ -341,10 +346,10 @@ static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 			}
 		}
 
-		scatterwalk_map_and_copy(rctx->buffer + rctx->bufcnt, rctx->sg,
-					 rctx->offset, count, 0);
+		scatterwalk_map_and_copy(state->buffer + state->bufcnt,
+					 rctx->sg, rctx->offset, count, 0);
 
-		rctx->bufcnt += count;
+		state->bufcnt += count;
 		rctx->offset += count;
 		rctx->total -= count;
 
@@ -413,26 +418,27 @@ static int stm32_hash_xmit_cpu(struct stm32_hash_dev *hdev,
 static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	struct stm32_hash_state *state = &rctx->state;
 	int bufcnt, err = 0, final;
 
 	dev_dbg(hdev->dev, "%s flags %lx\n", __func__, rctx->flags);
 
 	final = (rctx->flags & HASH_FLAGS_FINUP);
 
-	while ((rctx->total >= rctx->buflen) ||
-	       (rctx->bufcnt + rctx->total >= rctx->buflen)) {
+	while ((rctx->total >= state->buflen) ||
+	       (state->bufcnt + rctx->total >= state->buflen)) {
 		stm32_hash_append_sg(rctx);
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 0);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 0);
 	}
 
 	stm32_hash_append_sg(rctx);
 
 	if (final) {
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 1);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
 
 		/* If we have an IRQ, wait for that, else poll for completion */
 		if (hdev->polled) {
@@ -441,8 +447,20 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
 			err = 0;
 		}
+	} else {
+		u32 *preg = state->hw_context;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			*preg++ = stm32_hash_read(hdev, HASH_IMR);
+		*preg++ = stm32_hash_read(hdev, HASH_STR);
+		*preg++ = stm32_hash_read(hdev, HASH_CR);
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
 	}
 
+	rctx->flags |= HASH_FLAGS_INIT;
+
 	return err;
 }
 
@@ -584,10 +602,10 @@ static int stm32_hash_dma_init(struct stm32_hash_dev *hdev)
 static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	u32 *buffer = (void *)rctx->state.buffer;
 	struct scatterlist sg[1], *tsg;
 	int err = 0, len = 0, reg, ncp = 0;
 	unsigned int i;
-	u32 *buffer = (void *)rctx->buffer;
 
 	rctx->sg = hdev->req->src;
 	rctx->total = hdev->req->nbytes;
@@ -615,7 +633,7 @@ static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 
 				ncp = sg_pcopy_to_buffer(
 					rctx->sg, rctx->nents,
-					rctx->buffer, sg->length - len,
+					rctx->state.buffer, sg->length - len,
 					rctx->total - sg->length + len);
 
 				sg->length = len;
@@ -671,6 +689,8 @@ static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 		err = stm32_hash_hmac_dma_send(hdev);
 	}
 
+	rctx->flags |= HASH_FLAGS_INIT;
+
 	return err;
 }
 
@@ -749,14 +769,12 @@ static int stm32_hash_init(struct ahash_request *req)
 		return -EINVAL;
 	}
 
-	rctx->bufcnt = 0;
-	rctx->buflen = HASH_BUFLEN;
+	rctx->state.bufcnt = 0;
+	rctx->state.buflen = HASH_BUFLEN;
 	rctx->total = 0;
 	rctx->offset = 0;
 	rctx->data_type = HASH_DATA_8_BITS;
 
-	memset(rctx->buffer, 0, HASH_BUFLEN);
-
 	if (ctx->flags & HASH_FLAGS_HMAC)
 		rctx->flags |= HASH_FLAGS_HMAC;
 
@@ -774,15 +792,16 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 {
 	struct ahash_request *req = hdev->req;
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
+	int buflen = state->bufcnt;
 	int err;
-	int buflen = rctx->bufcnt;
 
-	rctx->bufcnt = 0;
+	state->bufcnt = 0;
 
 	if (!(rctx->flags & HASH_FLAGS_CPU))
 		err = stm32_hash_dma_send(hdev);
 	else
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, buflen, 1);
 
 	/* If we have an IRQ, wait for that, else poll for completion */
 	if (hdev->polled) {
@@ -832,7 +851,7 @@ static void stm32_hash_copy_hash(struct ahash_request *req)
 	__be32 *hash = (void *)rctx->digest;
 	unsigned int i, hashsize;
 
-	if (hdev->pdata->broken_emptymsg && !req->nbytes)
+	if (hdev->pdata->broken_emptymsg && !(rctx->flags & HASH_FLAGS_INIT))
 		return stm32_hash_emptymsg_fallback(req);
 
 	switch (rctx->flags & HASH_FLAGS_ALGO_MASK) {
@@ -882,11 +901,6 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
 		stm32_hash_copy_hash(req);
 		err = stm32_hash_finish(req);
-		hdev->flags &= ~(HASH_FLAGS_FINAL | HASH_FLAGS_CPU |
-				 HASH_FLAGS_INIT | HASH_FLAGS_DMA_READY |
-				 HASH_FLAGS_OUTPUT_READY | HASH_FLAGS_HMAC |
-				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
-				 HASH_FLAGS_HMAC_KEY);
 	} else {
 		rctx->flags |= HASH_FLAGS_ERRORS;
 	}
@@ -897,68 +911,58 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	crypto_finalize_hash_request(hdev->engine, req, err);
 }
 
-static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
+static void stm32_hash_hw_init(struct stm32_hash_dev *hdev,
 			      struct stm32_hash_request_ctx *rctx)
 {
 	pm_runtime_get_sync(hdev->dev);
-
-	if (!(HASH_FLAGS_INIT & hdev->flags)) {
-		stm32_hash_write(hdev, HASH_CR, HASH_CR_INIT);
-		stm32_hash_write(hdev, HASH_STR, 0);
-		stm32_hash_write(hdev, HASH_DIN, 0);
-		stm32_hash_write(hdev, HASH_IMR, 0);
-		hdev->err = 0;
-	}
-
-	return 0;
 }
 
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq);
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq);
-
 static int stm32_hash_handle_queue(struct stm32_hash_dev *hdev,
 				   struct ahash_request *req)
 {
 	return crypto_transfer_hash_request_to_engine(hdev->engine, req);
 }
 
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq)
+static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 {
 	struct ahash_request *req = container_of(areq, struct ahash_request,
 						 base);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
 	struct stm32_hash_request_ctx *rctx;
+	int err = 0;
 
 	if (!hdev)
 		return -ENODEV;
 
-	hdev->req = req;
-
-	rctx = ahash_request_ctx(req);
-
 	dev_dbg(hdev->dev, "processing new req, op: %lu, nbytes %d\n",
 		rctx->op, req->nbytes);
 
-	return stm32_hash_hw_init(hdev, rctx);
-}
-
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
-{
-	struct ahash_request *req = container_of(areq, struct ahash_request,
-						 base);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_request_ctx *rctx;
-	int err = 0;
-
-	if (!hdev)
-		return -ENODEV;
+	stm32_hash_hw_init(hdev, rctx);
 
 	hdev->req = req;
+	hdev->flags = 0;
 
 	rctx = ahash_request_ctx(req);
 
+	if (rctx->flags & HASH_FLAGS_INIT) {
+		u32 *preg = rctx->state.hw_context;
+		u32 reg;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			stm32_hash_write(hdev, HASH_IMR, *preg++);
+		stm32_hash_write(hdev, HASH_STR, *preg++);
+		stm32_hash_write(hdev, HASH_CR, *preg);
+		reg = *preg++ | HASH_CR_INIT;
+		stm32_hash_write(hdev, HASH_CR, reg);
+
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			stm32_hash_write(hdev, HASH_CSR(i), *preg++);
+
+		hdev->flags |= HASH_FLAGS_INIT;
+	}
+
 	if (rctx->op == HASH_OP_UPDATE)
 		err = stm32_hash_update_req(hdev);
 	else if (rctx->op == HASH_OP_FINAL)
@@ -985,6 +989,7 @@ static int stm32_hash_enqueue(struct ahash_request *req, unsigned int op)
 static int stm32_hash_update(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 
 	if (!req->nbytes || !(rctx->flags & HASH_FLAGS_CPU))
 		return 0;
@@ -993,7 +998,7 @@ static int stm32_hash_update(struct ahash_request *req)
 	rctx->sg = req->src;
 	rctx->offset = 0;
 
-	if ((rctx->bufcnt + rctx->total < rctx->buflen)) {
+	if ((state->bufcnt + rctx->total < state->buflen)) {
 		stm32_hash_append_sg(rctx);
 		return 0;
 	}
@@ -1044,35 +1049,13 @@ static int stm32_hash_digest(struct ahash_request *req)
 static int stm32_hash_export(struct ahash_request *req, void *out)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	u32 *preg;
-	unsigned int i;
-	int ret;
+	bool empty = !(rctx->flags & HASH_FLAGS_INIT);
+	u8 *p = out;
 
-	pm_runtime_get_sync(hdev->dev);
-
-	ret = stm32_hash_wait_busy(hdev);
-	if (ret)
-		return ret;
-
-	rctx->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					 sizeof(u32),
-					 GFP_KERNEL);
+	*(u8 *)p = empty;
 
-	preg = rctx->hw_context;
-
-	if (!hdev->pdata->ux500)
-		*preg++ = stm32_hash_read(hdev, HASH_IMR);
-	*preg++ = stm32_hash_read(hdev, HASH_STR);
-	*preg++ = stm32_hash_read(hdev, HASH_CR);
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
-
-	memcpy(out, rctx, sizeof(*rctx));
+	if (!empty)
+		memcpy(p + 1, &rctx->state, sizeof(rctx->state));
 
 	return 0;
 }
@@ -1080,32 +1063,14 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 static int stm32_hash_import(struct ahash_request *req, const void *in)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	const u32 *preg = in;
-	u32 reg;
-	unsigned int i;
-
-	memcpy(rctx, in, sizeof(*rctx));
+	const u8 *p = in;
 
-	preg = rctx->hw_context;
-
-	pm_runtime_get_sync(hdev->dev);
+	stm32_hash_init(req);
 
-	if (!hdev->pdata->ux500)
-		stm32_hash_write(hdev, HASH_IMR, *preg++);
-	stm32_hash_write(hdev, HASH_STR, *preg++);
-	stm32_hash_write(hdev, HASH_CR, *preg);
-	reg = *preg++ | HASH_CR_INIT;
-	stm32_hash_write(hdev, HASH_CR, reg);
-
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		stm32_hash_write(hdev, HASH_CSR(i), *preg++);
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
-
-	kfree(rctx->hw_context);
+	if (!*(u8 *)p) {
+		rctx->flags |= HASH_FLAGS_INIT;
+		memcpy(&rctx->state, p + 1, sizeof(rctx->state));
+	}
 
 	return 0;
 }
@@ -1162,8 +1127,6 @@ static int stm32_hash_cra_init_algs(struct crypto_tfm *tfm,
 		ctx->flags |= HASH_FLAGS_HMAC;
 
 	ctx->enginectx.op.do_one_request = stm32_hash_one_request;
-	ctx->enginectx.op.prepare_request = stm32_hash_prepare_req;
-	ctx->enginectx.op.unprepare_request = NULL;
 
 	return stm32_hash_init_fallback(tfm);
 }
@@ -1255,7 +1218,7 @@ static struct ahash_alg algs_md5[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "md5",
 				.cra_driver_name = "stm32-md5",
@@ -1282,7 +1245,7 @@ static struct ahash_alg algs_md5[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "hmac(md5)",
 				.cra_driver_name = "stm32-hmac-md5",
@@ -1311,7 +1274,7 @@ static struct ahash_alg algs_sha1[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "sha1",
 				.cra_driver_name = "stm32-sha1",
@@ -1338,7 +1301,7 @@ static struct ahash_alg algs_sha1[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "hmac(sha1)",
 				.cra_driver_name = "stm32-hmac-sha1",
@@ -1367,7 +1330,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "sha224",
 				.cra_driver_name = "stm32-sha224",
@@ -1394,7 +1357,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "hmac(sha224)",
 				.cra_driver_name = "stm32-hmac-sha224",
@@ -1423,7 +1386,7 @@ static struct ahash_alg algs_sha256[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "sha256",
 				.cra_driver_name = "stm32-sha256",
@@ -1450,7 +1413,7 @@ static struct ahash_alg algs_sha256[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "hmac(sha256)",
 				.cra_driver_name = "stm32-hmac-sha256",
-- 
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: [v3 PATCH] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Mon, Feb 27, 2023 at 11:39 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> v3 moves the exported state into its own structure so that it
> doesn't contain unnecessary data.  It also subsumes the "empty
> message" patch, as only the stm32_hash_copy_hash hunk was really
> needed and it's easiest if we put that here.

Thanks Herbert, now the driver probes fine, but unfortunately the tests
fail:

[    2.963725] alg: extra crypto tests enabled.  This is intended for
developer use only.
[   17.106532] Key type encrypted registered
[   23.804729] stm32-hash a03c2000.hash: allocated hmac(sha256) fallback
[   23.812529] alg: ahash: stm32-hmac-sha256 test failed (wrong
result) on test vector 0, cfg="init+update+final aligned buffer"
[   23.823902] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-22)
[   23.823924] ------------[ cut here ]------------
[   23.836312] WARNING: CPU: 0 PID: 72 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[   23.844449] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-22)
[   23.844460] Modules linked in:
[   23.855261] CPU: 0 PID: 72 Comm: cryptomgr_test Not tainted
6.2.0-12019-g4bc85a2f8f63 #65
[   23.863440] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[   23.870408]  unwind_backtrace from show_stack+0x10/0x14
[   23.875653]  show_stack from dump_stack_lvl+0x40/0x4c
[   23.880727]  dump_stack_lvl from __warn+0x94/0xc0
[   23.885450]  __warn from warn_slowpath_fmt+0x118/0x164
[   23.890603]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[   23.896540]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[   23.902042]  cryptomgr_test from kthread+0xc0/0xc4
[   23.906847]  kthread from ret_from_fork+0x14/0x2c
[   23.911557] Exception stack(0xf0f39fb0 to 0xf0f39ff8)
[   23.916609] 9fa0:                                     00000000
00000000 00000000 00000000
[   23.924783] 9fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   23.932956] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[   23.939604] ---[ end trace 0000000000000000 ]---


All tests fail on vector 0 or 1.

I am trying to debug!

Yours,
Linus Walleij

Re: [v3 PATCH] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Mon, Feb 27, 2023 at 10:17 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> I am trying to debug!

I am confused by the use of the state flag HASH_FLAGS_INIT in this
driver.

In stm32_hash_write_ctrl() that set up all the algorithm properties
this flag seems to mean "initialized the control register", as it sets
the flag after initializing the register and then does not write it
again if the flag is set.

I wonder if this is in line with the crypto driver expectations of that
flag.

I am wondering because at the end stm32_hash_dma_send()
you set this flag, and that function calls
stm32_hash_write_ctrl() first thing it does which will set the flag
unless it was set already.

Yours,
Linus Walleij

Re: [v3 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Mon, Feb 27, 2023 at 10:28:36PM +0100, Linus Walleij wrote:
> I am confused by the use of the state flag HASH_FLAGS_INIT in this
> driver.

Sorry, I was simply reusing that bit because it isn't currently
used by rctx.

Its meaning with respect to hdev is unchanged.  It keeps track of
whether the hardware state has been initalised, either through
stm32_hash_write_ctrl or by import.

Its use with rctx is new (this bit was previously unused).  The
intended meaning is that the ahash_request has received a non-
zero update.

Could you enable DEBUG and see what the dev_dbg statements print
out?

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

Re: [v3 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Mon, Feb 27, 2023 at 10:17:49PM +0100, Linus Walleij wrote:
> 
> Thanks Herbert, now the driver probes fine, but unfortunately the tests
> fail:
> 
> [    2.963725] alg: extra crypto tests enabled.  This is intended for
> developer use only.
> [   17.106532] Key type encrypted registered
> [   23.804729] stm32-hash a03c2000.hash: allocated hmac(sha256) fallback

What about the non-hmac algorithms? Do they pass or fail? If they
fail what's the kernel dmesg?

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

Re: [v3 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Tue, Feb 28, 2023 at 05:12:01PM +0800, Herbert Xu wrote:
>
> What about the non-hmac algorithms? Do they pass or fail? If they
> fail what's the kernel dmesg?

I can see a problem with hmac.  The hmac key state wasn't stored
in the rctx so it gets repeatedly written to the hardware on each
operation, resulting in a bogus digest.

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

[v4 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

v4 fixes hmac to not reload the key over and over again causing
the hash state to be corrupted.

---8<---
The Crypto API hashing paradigm requires the hardware state to
be exported between *each* request because multiple unrelated
hashes may be processed concurrently.

The stm32 hardware is capable of producing the hardware hashing
state but it was only doing it in the export function.  This is
not only broken for export as you can't export a kernel pointer
and reimport it, but it also means that concurrent hashing was
fundamentally broken.

Fix this by moving the saving and restoring of hardware hash
state between each and every hashing request.

Also change the emptymsg check in stm32_hash_copy_hash to rely
on whether we have any existing hash state, rather than whether
this particular update request is empty.

Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
Reported-by: Li kunyu <kunyu@nfschina.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 7bf805563ac2..a4c4cb1735d4 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -7,7 +7,6 @@
  */
 
 #include <linux/clk.h>
-#include <linux/crypto.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
@@ -127,6 +126,16 @@ struct stm32_hash_ctx {
 	int			keylen;
 };
 
+struct stm32_hash_state {
+	u16			bufcnt;
+	u16			buflen;
+
+	u8 buffer[HASH_BUFLEN] __aligned(4);
+
+	/* hash state */
+	u32			hw_context[3 + HASH_CSR_REGISTER_NUMBER];
+};
+
 struct stm32_hash_request_ctx {
 	struct stm32_hash_dev	*hdev;
 	unsigned long		flags;
@@ -134,8 +143,6 @@ struct stm32_hash_request_ctx {
 
 	u8 digest[SHA256_DIGEST_SIZE] __aligned(sizeof(u32));
 	size_t			digcnt;
-	size_t			bufcnt;
-	size_t			buflen;
 
 	/* DMA */
 	struct scatterlist	*sg;
@@ -149,10 +156,7 @@ struct stm32_hash_request_ctx {
 
 	u8			data_type;
 
-	u8 buffer[HASH_BUFLEN] __aligned(sizeof(u32));
-
-	/* Export Context */
-	u32			*hw_context;
+	struct stm32_hash_state state;
 };
 
 struct stm32_hash_algs_info {
@@ -183,7 +187,6 @@ struct stm32_hash_dev {
 	struct ahash_request	*req;
 	struct crypto_engine	*engine;
 
-	int			err;
 	unsigned long		flags;
 
 	struct dma_chan		*dma_lch;
@@ -326,11 +329,12 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 
 static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 {
+	struct stm32_hash_state *state = &rctx->state;
 	size_t count;
 
-	while ((rctx->bufcnt < rctx->buflen) && rctx->total) {
+	while ((state->bufcnt < state->buflen) && rctx->total) {
 		count = min(rctx->sg->length - rctx->offset, rctx->total);
-		count = min(count, rctx->buflen - rctx->bufcnt);
+		count = min_t(size_t, count, state->buflen - state->bufcnt);
 
 		if (count <= 0) {
 			if ((rctx->sg->length == 0) && !sg_is_last(rctx->sg)) {
@@ -341,10 +345,10 @@ static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 			}
 		}
 
-		scatterwalk_map_and_copy(rctx->buffer + rctx->bufcnt, rctx->sg,
-					 rctx->offset, count, 0);
+		scatterwalk_map_and_copy(state->buffer + state->bufcnt,
+					 rctx->sg, rctx->offset, count, 0);
 
-		rctx->bufcnt += count;
+		state->bufcnt += count;
 		rctx->offset += count;
 		rctx->total -= count;
 
@@ -413,26 +417,27 @@ static int stm32_hash_xmit_cpu(struct stm32_hash_dev *hdev,
 static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	struct stm32_hash_state *state = &rctx->state;
 	int bufcnt, err = 0, final;
 
 	dev_dbg(hdev->dev, "%s flags %lx\n", __func__, rctx->flags);
 
 	final = (rctx->flags & HASH_FLAGS_FINUP);
 
-	while ((rctx->total >= rctx->buflen) ||
-	       (rctx->bufcnt + rctx->total >= rctx->buflen)) {
+	while ((rctx->total >= state->buflen) ||
+	       (state->bufcnt + rctx->total >= state->buflen)) {
 		stm32_hash_append_sg(rctx);
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 0);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 0);
 	}
 
 	stm32_hash_append_sg(rctx);
 
 	if (final) {
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 1);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
 
 		/* If we have an IRQ, wait for that, else poll for completion */
 		if (hdev->polled) {
@@ -441,8 +446,20 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
 			err = 0;
 		}
+	} else {
+		u32 *preg = state->hw_context;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			*preg++ = stm32_hash_read(hdev, HASH_IMR);
+		*preg++ = stm32_hash_read(hdev, HASH_STR);
+		*preg++ = stm32_hash_read(hdev, HASH_CR);
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
 	}
 
+	rctx->flags |= HASH_FLAGS_INIT;
+
 	return err;
 }
 
@@ -584,10 +601,10 @@ static int stm32_hash_dma_init(struct stm32_hash_dev *hdev)
 static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	u32 *buffer = (void *)rctx->state.buffer;
 	struct scatterlist sg[1], *tsg;
 	int err = 0, len = 0, reg, ncp = 0;
 	unsigned int i;
-	u32 *buffer = (void *)rctx->buffer;
 
 	rctx->sg = hdev->req->src;
 	rctx->total = hdev->req->nbytes;
@@ -615,7 +632,7 @@ static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 
 				ncp = sg_pcopy_to_buffer(
 					rctx->sg, rctx->nents,
-					rctx->buffer, sg->length - len,
+					rctx->state.buffer, sg->length - len,
 					rctx->total - sg->length + len);
 
 				sg->length = len;
@@ -671,6 +688,8 @@ static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 		err = stm32_hash_hmac_dma_send(hdev);
 	}
 
+	rctx->flags |= HASH_FLAGS_INIT;
+
 	return err;
 }
 
@@ -749,14 +768,12 @@ static int stm32_hash_init(struct ahash_request *req)
 		return -EINVAL;
 	}
 
-	rctx->bufcnt = 0;
-	rctx->buflen = HASH_BUFLEN;
+	rctx->state.bufcnt = 0;
+	rctx->state.buflen = HASH_BUFLEN;
 	rctx->total = 0;
 	rctx->offset = 0;
 	rctx->data_type = HASH_DATA_8_BITS;
 
-	memset(rctx->buffer, 0, HASH_BUFLEN);
-
 	if (ctx->flags & HASH_FLAGS_HMAC)
 		rctx->flags |= HASH_FLAGS_HMAC;
 
@@ -774,15 +791,16 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 {
 	struct ahash_request *req = hdev->req;
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
+	int buflen = state->bufcnt;
 	int err;
-	int buflen = rctx->bufcnt;
 
-	rctx->bufcnt = 0;
+	state->bufcnt = 0;
 
 	if (!(rctx->flags & HASH_FLAGS_CPU))
 		err = stm32_hash_dma_send(hdev);
 	else
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, buflen, 1);
 
 	/* If we have an IRQ, wait for that, else poll for completion */
 	if (hdev->polled) {
@@ -832,7 +850,7 @@ static void stm32_hash_copy_hash(struct ahash_request *req)
 	__be32 *hash = (void *)rctx->digest;
 	unsigned int i, hashsize;
 
-	if (hdev->pdata->broken_emptymsg && !req->nbytes)
+	if (hdev->pdata->broken_emptymsg && !(rctx->flags & HASH_FLAGS_INIT))
 		return stm32_hash_emptymsg_fallback(req);
 
 	switch (rctx->flags & HASH_FLAGS_ALGO_MASK) {
@@ -882,11 +900,6 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
 		stm32_hash_copy_hash(req);
 		err = stm32_hash_finish(req);
-		hdev->flags &= ~(HASH_FLAGS_FINAL | HASH_FLAGS_CPU |
-				 HASH_FLAGS_INIT | HASH_FLAGS_DMA_READY |
-				 HASH_FLAGS_OUTPUT_READY | HASH_FLAGS_HMAC |
-				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
-				 HASH_FLAGS_HMAC_KEY);
 	} else {
 		rctx->flags |= HASH_FLAGS_ERRORS;
 	}
@@ -897,67 +910,61 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	crypto_finalize_hash_request(hdev->engine, req, err);
 }
 
-static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
+static void stm32_hash_hw_init(struct stm32_hash_dev *hdev,
 			      struct stm32_hash_request_ctx *rctx)
 {
 	pm_runtime_get_sync(hdev->dev);
-
-	if (!(HASH_FLAGS_INIT & hdev->flags)) {
-		stm32_hash_write(hdev, HASH_CR, HASH_CR_INIT);
-		stm32_hash_write(hdev, HASH_STR, 0);
-		stm32_hash_write(hdev, HASH_DIN, 0);
-		stm32_hash_write(hdev, HASH_IMR, 0);
-		hdev->err = 0;
-	}
-
-	return 0;
 }
 
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq);
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq);
-
 static int stm32_hash_handle_queue(struct stm32_hash_dev *hdev,
 				   struct ahash_request *req)
 {
 	return crypto_transfer_hash_request_to_engine(hdev->engine, req);
 }
 
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq)
+static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 {
 	struct ahash_request *req = container_of(areq, struct ahash_request,
 						 base);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
 	struct stm32_hash_request_ctx *rctx;
+	int err = 0;
 
 	if (!hdev)
 		return -ENODEV;
 
+	dev_dbg(hdev->dev, "processing new req, op: %lu, nbytes %d\n",
+		rctx->op, req->nbytes);
+
+	stm32_hash_hw_init(hdev, rctx);
+
 	hdev->req = req;
+	hdev->flags = 0;
 
 	rctx = ahash_request_ctx(req);
 
-	dev_dbg(hdev->dev, "processing new req, op: %lu, nbytes %d\n",
-		rctx->op, req->nbytes);
+	if (rctx->flags & HASH_FLAGS_INIT) {
+		u32 *preg = rctx->state.hw_context;
+		u32 reg;
+		int i;
 
-	return stm32_hash_hw_init(hdev, rctx);
-}
-
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
-{
-	struct ahash_request *req = container_of(areq, struct ahash_request,
-						 base);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_request_ctx *rctx;
-	int err = 0;
+		if (!hdev->pdata->ux500)
+			stm32_hash_write(hdev, HASH_IMR, *preg++);
+		stm32_hash_write(hdev, HASH_STR, *preg++);
+		stm32_hash_write(hdev, HASH_CR, *preg);
+		reg = *preg++ | HASH_CR_INIT;
+		stm32_hash_write(hdev, HASH_CR, reg);
 
-	if (!hdev)
-		return -ENODEV;
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			stm32_hash_write(hdev, HASH_CSR(i), *preg++);
 
-	hdev->req = req;
+		hdev->flags |= HASH_FLAGS_INIT;
 
-	rctx = ahash_request_ctx(req);
+		if (rctx->flags & HASH_FLAGS_HMAC)
+			hdev->flags |= HASH_FLAGS_HMAC |
+				       HASH_FLAGS_HMAC_KEY;
+	}
 
 	if (rctx->op == HASH_OP_UPDATE)
 		err = stm32_hash_update_req(hdev);
@@ -985,6 +992,7 @@ static int stm32_hash_enqueue(struct ahash_request *req, unsigned int op)
 static int stm32_hash_update(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 
 	if (!req->nbytes || !(rctx->flags & HASH_FLAGS_CPU))
 		return 0;
@@ -993,7 +1001,7 @@ static int stm32_hash_update(struct ahash_request *req)
 	rctx->sg = req->src;
 	rctx->offset = 0;
 
-	if ((rctx->bufcnt + rctx->total < rctx->buflen)) {
+	if ((state->bufcnt + rctx->total < state->buflen)) {
 		stm32_hash_append_sg(rctx);
 		return 0;
 	}
@@ -1044,35 +1052,13 @@ static int stm32_hash_digest(struct ahash_request *req)
 static int stm32_hash_export(struct ahash_request *req, void *out)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	u32 *preg;
-	unsigned int i;
-	int ret;
+	bool empty = !(rctx->flags & HASH_FLAGS_INIT);
+	u8 *p = out;
 
-	pm_runtime_get_sync(hdev->dev);
-
-	ret = stm32_hash_wait_busy(hdev);
-	if (ret)
-		return ret;
-
-	rctx->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					 sizeof(u32),
-					 GFP_KERNEL);
+	*(u8 *)p = empty;
 
-	preg = rctx->hw_context;
-
-	if (!hdev->pdata->ux500)
-		*preg++ = stm32_hash_read(hdev, HASH_IMR);
-	*preg++ = stm32_hash_read(hdev, HASH_STR);
-	*preg++ = stm32_hash_read(hdev, HASH_CR);
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
-
-	memcpy(out, rctx, sizeof(*rctx));
+	if (!empty)
+		memcpy(p + 1, &rctx->state, sizeof(rctx->state));
 
 	return 0;
 }
@@ -1080,32 +1066,14 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 static int stm32_hash_import(struct ahash_request *req, const void *in)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	const u32 *preg = in;
-	u32 reg;
-	unsigned int i;
-
-	memcpy(rctx, in, sizeof(*rctx));
+	const u8 *p = in;
 
-	preg = rctx->hw_context;
-
-	pm_runtime_get_sync(hdev->dev);
+	stm32_hash_init(req);
 
-	if (!hdev->pdata->ux500)
-		stm32_hash_write(hdev, HASH_IMR, *preg++);
-	stm32_hash_write(hdev, HASH_STR, *preg++);
-	stm32_hash_write(hdev, HASH_CR, *preg);
-	reg = *preg++ | HASH_CR_INIT;
-	stm32_hash_write(hdev, HASH_CR, reg);
-
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		stm32_hash_write(hdev, HASH_CSR(i), *preg++);
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
-
-	kfree(rctx->hw_context);
+	if (!*(u8 *)p) {
+		rctx->flags |= HASH_FLAGS_INIT;
+		memcpy(&rctx->state, p + 1, sizeof(rctx->state));
+	}
 
 	return 0;
 }
@@ -1162,8 +1130,6 @@ static int stm32_hash_cra_init_algs(struct crypto_tfm *tfm,
 		ctx->flags |= HASH_FLAGS_HMAC;
 
 	ctx->enginectx.op.do_one_request = stm32_hash_one_request;
-	ctx->enginectx.op.prepare_request = stm32_hash_prepare_req;
-	ctx->enginectx.op.unprepare_request = NULL;
 
 	return stm32_hash_init_fallback(tfm);
 }
@@ -1255,7 +1221,7 @@ static struct ahash_alg algs_md5[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "md5",
 				.cra_driver_name = "stm32-md5",
@@ -1282,7 +1248,7 @@ static struct ahash_alg algs_md5[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "hmac(md5)",
 				.cra_driver_name = "stm32-hmac-md5",
@@ -1311,7 +1277,7 @@ static struct ahash_alg algs_sha1[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "sha1",
 				.cra_driver_name = "stm32-sha1",
@@ -1338,7 +1304,7 @@ static struct ahash_alg algs_sha1[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "hmac(sha1)",
 				.cra_driver_name = "stm32-hmac-sha1",
@@ -1367,7 +1333,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "sha224",
 				.cra_driver_name = "stm32-sha224",
@@ -1394,7 +1360,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "hmac(sha224)",
 				.cra_driver_name = "stm32-hmac-sha224",
@@ -1423,7 +1389,7 @@ static struct ahash_alg algs_sha256[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "sha256",
 				.cra_driver_name = "stm32-sha256",
@@ -1450,7 +1416,7 @@ static struct ahash_alg algs_sha256[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state) + 1,
 			.base = {
 				.cra_name = "hmac(sha256)",
 				.cra_driver_name = "stm32-hmac-sha256",
-- 
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: [v4 PATCH] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Tue, Feb 28, 2023 at 10:48 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> v4 fixes hmac to not reload the key over and over again causing
> the hash state to be corrupted.

OK I tested this, sadly the same results.

Notice though: the HMAC versions fail on test vector 0 and
the non-MAC:ed fail on vector 1, so I guess that means test
vector 0 works with those?

Here is the complete log:

[    2.997312] alg: extra crypto tests enabled.  This is intended for
developer use only.
[   15.203609] Key type encrypted registered
[   22.553791] stm32-hash a03c2000.hash: allocated hmac(sha256) fallback
[   22.561976] alg: ahash: stm32-hmac-sha256 test failed (wrong
result) on test vector 0, cfg="init+update+final aligned buffer"
[   22.573387] Expected:
[   22.575674] 00000000: a2 1b 1f 5d 4c f4 f7 3a 4d d9 39 75 0f 7a 06 6a
[   22.582160] 00000010: 7f 98 cc 13 1c b1 6a 66 92 75 90 21 cf ab 81 81
[   22.588613] Obtained:
[   22.590917] 00000000: 46 24 76 a8 97 dd fd bd 40 d1 42 0e 08 a5 bc fe
[   22.597368] 00000010: eb 25 c3 e2 ad e6 a0 a9 08 3b 32 7b 9e f9 fc a1
[   22.603865] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-22)
[   22.603887] ------------[ cut here ]------------
[   22.616297] WARNING: CPU: 1 PID: 75 at crypto/testmgr.c:5864
alg_test.part.0+0x4d0/0x4dc
[   22.624437] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-22)
[   22.624448] Modules linked in:
[   22.635258] CPU: 1 PID: 75 Comm: cryptomgr_test Not tainted
6.2.0-12020-g1c3e1a0051be #67
[   22.643437] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[   22.650405]  unwind_backtrace from show_stack+0x10/0x14
[   22.655650]  show_stack from dump_stack_lvl+0x40/0x4c
[   22.660724]  dump_stack_lvl from __warn+0x94/0xc0
[   22.665447]  __warn from warn_slowpath_fmt+0x118/0x164
[   22.670601]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[   22.676537]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[   22.682037]  cryptomgr_test from kthread+0xc0/0xc4
[   22.686843]  kthread from ret_from_fork+0x14/0x2c
[   22.691553] Exception stack(0xf0f45fb0 to 0xf0f45ff8)
[   22.696604] 5fa0:                                     00000000
00000000 00000000 00000000
[   22.704779] 5fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   22.712953] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[   22.719596] ---[ end trace 0000000000000000 ]---
[   22.724494] stm32-hash a03c2000.hash: allocated sha256 fallback
[   22.769732] alg: ahash: stm32-sha256 test failed (wrong result) on
test vector 1, cfg="init+update+final aligned buffer"
[   22.780648] Expected:
[   22.782952] 00000000: ba 78 16 bf 8f 01 cf ea 41 41 40 de 5d ae 22 23
[   22.789392] 00000010: b0 03 61 a3 96 17 7a 9c b4 10 ff 61 f2 00 15 ad
[   22.795874] Obtained:
[   22.798147] 00000000: e3 b0 c4 42 98 fc 1c 14 9a fb f4 c8 99 6f b9 24
[   22.804607] 00000010: 27 ae 41 e4 64 9b 93 4c a4 95 99 1b 78 52 b8 55
[   22.811074] alg: self-tests for sha256 using stm32-sha256 failed (rc=-22)
[   22.811083] ------------[ cut here ]------------
[   22.822480] WARNING: CPU: 1 PID: 85 at crypto/testmgr.c:5864
alg_test.part.0+0x4d0/0x4dc
[   22.830607] alg: self-tests for sha256 using stm32-sha256 failed (rc=-22)
[   22.830615] Modules linked in:
[   22.840457] CPU: 1 PID: 85 Comm: cryptomgr_test Tainted: G        W
         6.2.0-12020-g1c3e1a0051be #67
[   22.850109] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[   22.857069]  unwind_backtrace from show_stack+0x10/0x14
[   22.862307]  show_stack from dump_stack_lvl+0x40/0x4c
[   22.867373]  dump_stack_lvl from __warn+0x94/0xc0
[   22.872090]  __warn from warn_slowpath_fmt+0x118/0x164
[   22.877237]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[   22.883167]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[   22.888662]  cryptomgr_test from kthread+0xc0/0xc4
[   22.893462]  kthread from ret_from_fork+0x14/0x2c
[   22.898169] Exception stack(0xf0f6dfb0 to 0xf0f6dff8)
[   22.903216] dfa0:                                     00000000
00000000 00000000 00000000
[   22.911388] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   22.919559] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[   22.926182] ---[ end trace 0000000000000000 ]---
[   36.677933] stm32-hash a03c2000.hash: allocated hmac(sha1) fallback
[   36.686991] alg: ahash: stm32-hmac-sha1 test failed (wrong result)
on test vector 0, cfg="init+update+final aligned buffer"
[   36.698242] Expected:
[   36.700547] 00000000: b6 17 31 86 55 05 72 64 e2 8b c0 b6 fb 37 8c 8e
[   36.707002] 00000010: f1 46 be 00
[   36.710345] Obtained:
[   36.712624] 00000000: 12 3f d7 8b da 01 00 78 6a e8 6b 76 f5 0f 01 bd
[   36.719072] 00000010: 18 e4 77 f3
[   36.722450] alg: self-tests for hmac(sha1) using stm32-hmac-sha1
failed (rc=-22)
[   36.722472] ------------[ cut here ]------------
[   36.734495] WARNING: CPU: 1 PID: 88 at crypto/testmgr.c:5864
alg_test.part.0+0x4d0/0x4dc
[   36.742628] alg: self-tests for hmac(sha1) using stm32-hmac-sha1
failed (rc=-22)
[   36.742637] Modules linked in:
[   36.753097] CPU: 1 PID: 88 Comm: cryptomgr_test Tainted: G        W
         6.2.0-12020-g1c3e1a0051be #67
[   36.762754] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[   36.769719]  unwind_backtrace from show_stack+0x10/0x14
[   36.774963]  show_stack from dump_stack_lvl+0x40/0x4c
[   36.780036]  dump_stack_lvl from __warn+0x94/0xc0
[   36.784759]  __warn from warn_slowpath_fmt+0x118/0x164
[   36.789912]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[   36.795847]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[   36.801347]  cryptomgr_test from kthread+0xc0/0xc4
[   36.806153]  kthread from ret_from_fork+0x14/0x2c
[   36.810862] Exception stack(0xf0f79fb0 to 0xf0f79ff8)
[   36.815912] 9fa0:                                     00000000
00000000 00000000 00000000
[   36.824087] 9fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   36.832261] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[   36.838902] ---[ end trace 0000000000000000 ]---
[   36.843762] stm32-hash a03c2000.hash: allocated sha1 fallback
[   36.889782] alg: ahash: stm32-sha1 test failed (wrong result) on
test vector 1, cfg="init+update+final aligned buffer"
[   36.900507] Expected:
[   36.902786] 00000000: a9 99 3e 36 47 06 81 6a ba 3e 25 71 78 50 c2 6c
[   36.909225] 00000010: 9c d0 d8 9d
[   36.912564] Obtained:
[   36.914834] 00000000: da 39 a3 ee 5e 6b 4b 0d 32 55 bf ef 95 60 18 90
[   36.921296] 00000010: af d8 07 09
[   36.924627] alg: self-tests for sha1 using stm32-sha1 failed (rc=-22)
[   36.924635] ------------[ cut here ]------------
[   36.935687] WARNING: CPU: 1 PID: 100 at crypto/testmgr.c:5864
alg_test.part.0+0x4d0/0x4dc
[   36.943902] alg: self-tests for sha1 using stm32-sha1 failed (rc=-22)
[   36.943909] Modules linked in:
[   36.953406] CPU: 1 PID: 100 Comm: cryptomgr_test Tainted: G
W          6.2.0-12020-g1c3e1a0051be #67
[   36.963144] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[   36.970103]  unwind_backtrace from show_stack+0x10/0x14
[   36.975340]  show_stack from dump_stack_lvl+0x40/0x4c
[   36.980404]  dump_stack_lvl from __warn+0x94/0xc0
[   36.985120]  __warn from warn_slowpath_fmt+0x118/0x164
[   36.990266]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[   36.996195]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[   37.001689]  cryptomgr_test from kthread+0xc0/0xc4
[   37.006488]  kthread from ret_from_fork+0x14/0x2c
[   37.011193] Exception stack(0xf0f8dfb0 to 0xf0f8dff8)
[   37.016240] dfa0:                                     00000000
00000000 00000000 00000000
[   37.024411] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   37.032581] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[   37.039222] ---[ end trace 0000000000000000 ]---

Here I have applied a patch like this to see the failing vectors:

commit 1c3e1a0051be234ef109e97075783c28e3b07452 (HEAD ->
ux500-fixup-stm32-cryp-herbert-v4)
Author: Linus Walleij <linus.walleij@linaro.org>
Date:   Mon Dec 26 09:53:10 2022 +0100

    test hacks

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index c91e93ece20b..db511293933b 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1203,6 +1203,10 @@ static int check_hash_result(const char *type,
        if (memcmp(result, vec->digest, digestsize) != 0) {
                pr_err("alg: %s: %s test failed (wrong result) on test
vector %s, cfg=\"%s\"\n",
                       type, driver, vec_name, cfg->name);
+               pr_err("Expected:\n");
+               hexdump(vec->digest, digestsize);
+               pr_err("Obtained:\n");
+               hexdump(result, digestsize);
                return -EINVAL;

I'm a bit lost on what to try next :/

Yours,
Linus Walleij

Re: [v4 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Tue, Feb 28, 2023 at 09:50:55PM +0100, Linus Walleij wrote:
> 
> OK I tested this, sadly the same results.
> 
> Notice though: the HMAC versions fail on test vector 0 and
> the non-MAC:ed fail on vector 1, so I guess that means test
> vector 0 works with those?

Hah, test vector 0 for sha256 is an empty message.  While test
vector 1 is the same as test vector 0 for hmac(sha256).

So I guess at least the fallback is still working :)

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: [v4 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Tue, Feb 28, 2023 at 09:50:55PM +0100, Linus Walleij wrote:
> 
> Notice though: the HMAC versions fail on test vector 0 and
> the non-MAC:ed fail on vector 1, so I guess that means test
> vector 0 works with those?

The failing vector is the first one where we save the state from
the hardware and then try to restore it.

Is your device ux500 or stm32? Perhaps state saving/restoring is
simply broken on ux500 (as the original ux500 driver didn't support
export/import and always used a fallback)?

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

Re: [v4 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Wed, Mar 01, 2023 at 09:36:08AM +0800, Herbert Xu wrote:
>
> Is your device ux500 or stm32? Perhaps state saving/restoring is
> simply broken on ux500 (as the original ux500 driver didn't support
> export/import and always used a fallback)?

Interesting, I dug up the old ux500 driver and even though
it doesn't have export/import hooked up, it does actually appear
to save/restore hardware state.  In fact it seems to do it multiple
times per request, even when it's unnecessary.

I'll try to see if the saving/restoring is subtly different
between ux500 and stm32.

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: [v4 PATCH] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Wed, Mar 1, 2023 at 2:36 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The failing vector is the first one where we save the state from
> the hardware and then try to restore it.

Yeah that's typical :/

> Is your device ux500 or stm32? Perhaps state saving/restoring is
> simply broken on ux500 (as the original ux500 driver didn't support
> export/import and always used a fallback)?

It's Ux500 but I had no problem with import/export before,
and yeah it has state save/restore in HW.

Yours,
Linus Walleij

Re: [v4 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Wed, Mar 01, 2023 at 01:22:13PM +0100, Linus Walleij wrote:
>
> It's Ux500 but I had no problem with import/export before,
> and yeah it has state save/restore in HW.

So with the stm32 driver your ux500 is able to pass the extra
fuzz tests, right? That should indeed test export and import.

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

Re: [v4 PATCH] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Wed, Mar 01, 2023 at 01:22:13PM +0100, Linus Walleij wrote:
>
> It's Ux500 but I had no problem with import/export before,
> and yeah it has state save/restore in HW.

I think I see the problem.  My patch wasn't waiting for the hash
computation to complete before saving the state so obviously it
will get the wrong hash state every single time.

I'll fix this up and some other inconsistencies (my reading of the
documentation is that there are 54 registers (0-53), not 53) and
resend the patch.

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

[v5 PATCH 0/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Thu, Mar 02, 2023 at 02:04:38PM +0800, Herbert Xu wrote:
> 
> I think I see the problem.  My patch wasn't waiting for the hash
> computation to complete before saving the state so obviously it
> will get the wrong hash state every single time.
> 
> I'll fix this up and some other inconsistencies (my reading of the
> documentation is that there are 54 registers (0-53), not 53) and
> resend the patch.

I've split the patch up into smaller chunks for easier testing.

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

[v5 PATCH 1/7] crypto: stm32 - Save 54 CSR registers

[Herbert Xu]

The CSR registers go from 0 to 53.  So the number of registers
should be 54.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 7bf805563ac2..bde2b40a6a32 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -68,7 +68,7 @@
 #define HASH_MASK_DATA_INPUT		BIT(1)
 
 /* Context swap register */
-#define HASH_CSR_REGISTER_NUMBER	53
+#define HASH_CSR_REGISTER_NUMBER	54
 
 /* Status Flags */
 #define HASH_SR_DATA_INPUT_READY	BIT(0)

Re: [v5 PATCH 1/7] crypto: stm32 - Save 54 CSR registers

[Linus Walleij]

On Sat, Mar 4, 2023 at 10:37 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The CSR registers go from 0 to 53.  So the number of registers
> should be 54.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Hm I don't know where this misunderstanding comes from.
I think it's this tendency by some engineers to use index 1 :/

The datasheet for U8500 says:
0xF8            HASH_CSFULL      HASH context full register
0xFC            HASH_CSDATAIN    HASH context swap data input register
0x100           HASH_CSR0        HASH context swap register 0
0x104 to 0x1CC  HASH_CSR1 to 51  HASH context swap register 1 to 51

0xf8, 0xfc, 0x100 = 3 registers
0x104 to 0x1cc = 51 registers

Indeed 54.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[v5 PATCH 2/7] crypto: stm32 - Move polling into do_one_request

[Herbert Xu]

There is no need to poll separate for update and final.  We could
merge them into do_one_request.

Also fix the error handling so that we don't poll (and overwrite
the error) when an error has already occurred.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |   29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index bde2b40a6a32..298cabd29e36 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -425,6 +425,8 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 		bufcnt = rctx->bufcnt;
 		rctx->bufcnt = 0;
 		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 0);
+		if (err)
+			return err;
 	}
 
 	stm32_hash_append_sg(rctx);
@@ -433,14 +435,6 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 		bufcnt = rctx->bufcnt;
 		rctx->bufcnt = 0;
 		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 1);
-
-		/* If we have an IRQ, wait for that, else poll for completion */
-		if (hdev->polled) {
-			if (stm32_hash_wait_busy(hdev))
-				return -ETIMEDOUT;
-			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
-			err = 0;
-		}
 	}
 
 	return err;
@@ -784,15 +778,6 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 	else
 		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
 
-	/* If we have an IRQ, wait for that, else poll for completion */
-	if (hdev->polled) {
-		if (stm32_hash_wait_busy(hdev))
-			return -ETIMEDOUT;
-		hdev->flags |= HASH_FLAGS_OUTPUT_READY;
-		/* Caller will call stm32_hash_finish_req() */
-		err = 0;
-	}
-
 	return err;
 }
 
@@ -964,6 +949,16 @@ static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 	else if (rctx->op == HASH_OP_FINAL)
 		err = stm32_hash_final_req(hdev);
 
+	/* If we have an IRQ, wait for that, else poll for completion */
+	if (err == -EINPROGRESS && hdev->polled) {
+		if (stm32_hash_wait_busy(hdev))
+			err = -ETIMEDOUT;
+		else {
+			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
+			err = 0;
+		}
+	}
+
 	if (err != -EINPROGRESS)
 	/* done task will not finish it, so do it here */
 		stm32_hash_finish_req(req, err);

Re: [v5 PATCH 2/7] crypto: stm32 - Move polling into do_one_request

[Linus Walleij]

On Sat, Mar 4, 2023 at 10:37 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> There is no need to poll separate for update and final.  We could
> merge them into do_one_request.
>
> Also fix the error handling so that we don't poll (and overwrite
> the error) when an error has already occurred.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Works like a charm and passes all tests.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[v5 PATCH 3/7] crypto: stm32 - Simplify finup

[Herbert Xu]

The current finup code is unnecessarily convoluted.  There is no
need to call update and final separately as update already does
all the necessary work on its own.

Simplify this by utilising the HASH_FLAGS_FINUP bit in rctx to
indicate only finup and use the HASH_FLAGS_FINAL bit instead to
signify processing common to both final and finup.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |   34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 298cabd29e36..473809b26566 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -417,7 +417,7 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 
 	dev_dbg(hdev->dev, "%s flags %lx\n", __func__, rctx->flags);
 
-	final = (rctx->flags & HASH_FLAGS_FINUP);
+	final = rctx->flags & HASH_FLAGS_FINAL;
 
 	while ((rctx->total >= rctx->buflen) ||
 	       (rctx->bufcnt + rctx->total >= rctx->buflen)) {
@@ -761,6 +761,11 @@ static int stm32_hash_init(struct ahash_request *req)
 
 static int stm32_hash_update_req(struct stm32_hash_dev *hdev)
 {
+	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+
+	if (!(rctx->flags & HASH_FLAGS_CPU))
+		return stm32_hash_dma_send(hdev);
+
 	return stm32_hash_update_cpu(hdev);
 }
 
@@ -768,17 +773,14 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 {
 	struct ahash_request *req = hdev->req;
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	int err;
 	int buflen = rctx->bufcnt;
 
-	rctx->bufcnt = 0;
+	if (rctx->flags & HASH_FLAGS_FINUP)
+		return stm32_hash_update_req(hdev);
 
-	if (!(rctx->flags & HASH_FLAGS_CPU))
-		err = stm32_hash_dma_send(hdev);
-	else
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
+	rctx->bufcnt = 0;
 
-	return err;
+	return stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
 }
 
 static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
@@ -1000,7 +1002,7 @@ static int stm32_hash_final(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 
-	rctx->flags |= HASH_FLAGS_FINUP;
+	rctx->flags |= HASH_FLAGS_FINAL;
 
 	return stm32_hash_enqueue(req, HASH_OP_FINAL);
 }
@@ -1010,25 +1012,13 @@ static int stm32_hash_finup(struct ahash_request *req)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	int err1, err2;
 
 	rctx->flags |= HASH_FLAGS_FINUP;
 
 	if (hdev->dma_lch && stm32_hash_dma_aligned_data(req))
 		rctx->flags &= ~HASH_FLAGS_CPU;
 
-	err1 = stm32_hash_update(req);
-
-	if (err1 == -EINPROGRESS || err1 == -EBUSY)
-		return err1;
-
-	/*
-	 * final() has to be always called to cleanup resources
-	 * even if update() failed, except EINPROGRESS
-	 */
-	err2 = stm32_hash_final(req);
-
-	return err1 ?: err2;
+	return stm32_hash_final(req);
 }
 
 static int stm32_hash_digest(struct ahash_request *req)

Re: [v5 PATCH 3/7] crypto: stm32 - Simplify finup

[Linus Walleij]

On Sat, Mar 4, 2023 at 10:37 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The current finup code is unnecessarily convoluted.  There is no
> need to call update and final separately as update already does
> all the necessary work on its own.
>
> Simplify this by utilising the HASH_FLAGS_FINUP bit in rctx to
> indicate only finup and use the HASH_FLAGS_FINAL bit instead to
> signify processing common to both final and finup.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

All tests sadly fail after this patch:

[    4.699857] stm32-hash a03c2000.hash: allocated hmac(sha256) fallback
[    4.708231] alg: ahash: stm32-hmac-sha256 test failed (wrong
result) on test vector 0, cfg="init+finup aligned buffer"
[    4.719048] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-22)
[    4.719070] ------------[ cut here ]------------
[    4.731602] WARNING: CPU: 0 PID: 88 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[    4.739832] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-22)
[    4.739853] Modules linked in:
[    4.750718] CPU: 0 PID: 88 Comm: cryptomgr_test Not tainted
6.2.0-13567-g410ada7489b7 #76
[    4.758915] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[    4.765897]  unwind_backtrace from show_stack+0x10/0x14
[    4.771169]  show_stack from dump_stack_lvl+0x40/0x4c
[    4.776270]  dump_stack_lvl from __warn+0x94/0xc0
[    4.781022]  __warn from warn_slowpath_fmt+0x118/0x164
[    4.786199]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[    4.792167]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[    4.797696]  cryptomgr_test from kthread+0xc0/0xc4
[    4.802527]  kthread from ret_from_fork+0x14/0x2c
[    4.807257] Exception stack(0xf10b9fb0 to 0xf10b9ff8)
[    4.812320] 9fa0:                                     00000000
00000000 00000000 00000000
[    4.820510] 9fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    4.828698] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    4.835475] ---[ end trace 0000000000000000 ]---
[    4.840713] stm32-hash a03c2000.hash: allocated sha256 fallback
[    4.854071] alg: ahash: stm32-sha256 test failed (wrong result) on
test vector 1, cfg="init+finup aligned buffer"
[    4.864487] alg: self-tests for sha256 using stm32-sha256 failed (rc=-22)
[    4.864500] ------------[ cut here ]------------
[    4.875924] WARNING: CPU: 0 PID: 98 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[    4.884108] alg: self-tests for sha256 using stm32-sha256 failed (rc=-22)
[    4.884119] Modules linked in:
[    4.894022] CPU: 0 PID: 98 Comm: cryptomgr_test Tainted: G        W
         6.2.0-13567-g410ada7489b7 #76
[    4.903679] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[    4.910642]  unwind_backtrace from show_stack+0x10/0x14
[    4.915884]  show_stack from dump_stack_lvl+0x40/0x4c
[    4.920954]  dump_stack_lvl from __warn+0x94/0xc0
[    4.925675]  __warn from warn_slowpath_fmt+0x118/0x164
[    4.930826]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[    4.936764]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[    4.942265]  cryptomgr_test from kthread+0xc0/0xc4
[    4.947070]  kthread from ret_from_fork+0x14/0x2c
[    4.951780] Exception stack(0xf10e1fb0 to 0xf10e1ff8)
[    4.956830] 1fa0:                                     00000000
00000000 00000000 00000000
[    4.965005] 1fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    4.973179] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    4.979850] ---[ end trace 0000000000000000 ]---
[    5.449257] stm32-hash a03c2000.hash: allocated hmac(sha1) fallback
[    5.458347] alg: ahash: stm32-hmac-sha1 test failed (wrong result)
on test vector 0, cfg="init+finup aligned buffer"
[    5.469070] alg: self-tests for hmac(sha1) using stm32-hmac-sha1
failed (rc=-22)
[    5.469112] ------------[ cut here ]------------
[    5.481280] WARNING: CPU: 1 PID: 101 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[    5.489663] alg: self-tests for hmac(sha1) using stm32-hmac-sha1
failed (rc=-22)
[    5.489699] Modules linked in:
[    5.500277] CPU: 1 PID: 101 Comm: cryptomgr_test Tainted: G
W          6.2.0-13567-g410ada7489b7 #76
[    5.510038] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[    5.517014]  unwind_backtrace from show_stack+0x10/0x14
[    5.522287]  show_stack from dump_stack_lvl+0x40/0x4c
[    5.527390]  dump_stack_lvl from __warn+0x94/0xc0
[    5.532140]  __warn from warn_slowpath_fmt+0x118/0x164
[    5.537318]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[    5.543284]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[    5.548815]  cryptomgr_test from kthread+0xc0/0xc4
[    5.553648]  kthread from ret_from_fork+0x14/0x2c
[    5.558378] Exception stack(0xf10edfb0 to 0xf10edff8)
[    5.563443] dfa0:                                     00000000
00000000 00000000 00000000
[    5.571633] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    5.579822] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    5.586511] ---[ end trace 0000000000000000 ]---
[    5.591474] stm32-hash a03c2000.hash: allocated sha1 fallback
[    5.610829] alg: ahash: stm32-sha1 test failed (wrong result) on
test vector 1, cfg="init+finup aligned buffer"
[    5.621005] alg: self-tests for sha1 using stm32-sha1 failed (rc=-22)
[    5.621014] ------------[ cut here ]------------
[    5.632136] WARNING: CPU: 0 PID: 113 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[    5.640368] alg: self-tests for sha1 using stm32-sha1 failed (rc=-22)
[    5.640377] Modules linked in:
[    5.649882] CPU: 0 PID: 113 Comm: cryptomgr_test Tainted: G
W          6.2.0-13567-g410ada7489b7 #76
[    5.659620] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[    5.666579]  unwind_backtrace from show_stack+0x10/0x14
[    5.671817]  show_stack from dump_stack_lvl+0x40/0x4c
[    5.676881]  dump_stack_lvl from __warn+0x94/0xc0
[    5.681594]  __warn from warn_slowpath_fmt+0x118/0x164
[    5.686739]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[    5.692669]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[    5.698163]  cryptomgr_test from kthread+0xc0/0xc4
[    5.702962]  kthread from ret_from_fork+0x14/0x2c
[    5.707668] Exception stack(0xf10edfb0 to 0xf10edff8)
[    5.712716] dfa0:                                     00000000
00000000 00000000 00000000
[    5.720887] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    5.729057] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    5.735717] ---[ end trace 0000000000000000 ]---

As usual the fallback works. I tried to look into it but I don't
understand this part:

@ -768,17 +773,14 @@ static int stm32_hash_final_req(struct
stm32_hash_dev *hdev)

> +       if (rctx->flags & HASH_FLAGS_FINUP)
> +               return stm32_hash_update_req(hdev);

stm32_hash_update_req() will do

if (!(rctx->flags & HASH_FLAGS_CPU))
    return stm32_hash_dma_send(hdev);
(...)
return stm32_hash_update_cpu(hdev);

So it replaces:

> -       if (!(rctx->flags & HASH_FLAGS_CPU))
> -               err = stm32_hash_dma_send(hdev);
> -       else
> -               err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);

But now we are calling stm32_hash_update_cpu() instead of
stm32_hash_xmit_cpu(), because that is what stm32_hash_update_cpu()
does.

Yours,
Linus Walleij

Re: [v5 PATCH 3/7] crypto: stm32 - Simplify finup

[Herbert Xu]

On Sun, Mar 05, 2023 at 11:08:24PM +0100, Linus Walleij wrote:
>
> All tests sadly fail after this patch:
> 
> [    4.699857] stm32-hash a03c2000.hash: allocated hmac(sha256) fallback
> [    4.708231] alg: ahash: stm32-hmac-sha256 test failed (wrong
> result) on test vector 0, cfg="init+finup aligned buffer"

We're making progress.  This worked on init+update+final, but then
failed on init+finup.  So it is indeed the finup code that was
broken by my patch as I forgot to set the rctx fields to include
the new data.

I'll repost the series.

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

[v5 PATCH 4/7] crypto: stm32 - Remove unused hdev->err field

[Herbert Xu]

The variable hdev->err is never read so it can be removed.

Also remove a spurious inclusion of linux/crypto.h.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 473809b26566..d7c99c09debe 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -7,7 +7,6 @@
  */
 
 #include <linux/clk.h>
-#include <linux/crypto.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
@@ -183,7 +182,6 @@ struct stm32_hash_dev {
 	struct ahash_request	*req;
 	struct crypto_engine	*engine;
 
-	int			err;
 	unsigned long		flags;
 
 	struct dma_chan		*dma_lch;
@@ -894,7 +892,6 @@ static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
 		stm32_hash_write(hdev, HASH_STR, 0);
 		stm32_hash_write(hdev, HASH_DIN, 0);
 		stm32_hash_write(hdev, HASH_IMR, 0);
-		hdev->err = 0;
 	}
 
 	return 0;

Re: [v5 PATCH 4/7] crypto: stm32 - Remove unused hdev->err field

[Linus Walleij]

On Sat, Mar 4, 2023 at 10:37 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The variable hdev->err is never read so it can be removed.
>
> Also remove a spurious inclusion of linux/crypto.h.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[v5 PATCH 5/7] crypto: stm32 - Move hash state into separate structure

[Herbert Xu]

Create a new struct stm32_hash_state so that it may be exported
in future instead of the entire request context.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |  126 ++++++++++++++++++++------------------
 1 file changed, 69 insertions(+), 57 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index d7c99c09debe..7cdde961b626 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -126,15 +126,24 @@ struct stm32_hash_ctx {
 	int			keylen;
 };
 
+struct stm32_hash_state {
+	u32			flags;
+
+	u16			bufcnt;
+	u16			buflen;
+
+	u8 buffer[HASH_BUFLEN] __aligned(4);
+
+	/* hash state */
+	u32			*hw_context;
+};
+
 struct stm32_hash_request_ctx {
 	struct stm32_hash_dev	*hdev;
-	unsigned long		flags;
 	unsigned long		op;
 
 	u8 digest[SHA256_DIGEST_SIZE] __aligned(sizeof(u32));
 	size_t			digcnt;
-	size_t			bufcnt;
-	size_t			buflen;
 
 	/* DMA */
 	struct scatterlist	*sg;
@@ -148,10 +157,7 @@ struct stm32_hash_request_ctx {
 
 	u8			data_type;
 
-	u8 buffer[HASH_BUFLEN] __aligned(sizeof(u32));
-
-	/* Export Context */
-	u32			*hw_context;
+	struct stm32_hash_state state;
 };
 
 struct stm32_hash_algs_info {
@@ -268,11 +274,12 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
 	struct crypto_ahash *tfm = crypto_ahash_reqtfm(hdev->req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(tfm);
+	struct stm32_hash_state *state = &rctx->state;
 
 	u32 reg = HASH_CR_INIT;
 
 	if (!(hdev->flags & HASH_FLAGS_INIT)) {
-		switch (rctx->flags & HASH_FLAGS_ALGO_MASK) {
+		switch (state->flags & HASH_FLAGS_ALGO_MASK) {
 		case HASH_FLAGS_MD5:
 			reg |= HASH_CR_ALGO_MD5;
 			break;
@@ -297,7 +304,7 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 
 		reg |= (rctx->data_type << HASH_CR_DATATYPE_POS);
 
-		if (rctx->flags & HASH_FLAGS_HMAC) {
+		if (state->flags & HASH_FLAGS_HMAC) {
 			hdev->flags |= HASH_FLAGS_HMAC;
 			reg |= HASH_CR_MODE;
 			if (ctx->keylen > HASH_LONG_KEY)
@@ -324,11 +331,12 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 
 static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 {
+	struct stm32_hash_state *state = &rctx->state;
 	size_t count;
 
-	while ((rctx->bufcnt < rctx->buflen) && rctx->total) {
+	while ((state->bufcnt < state->buflen) && rctx->total) {
 		count = min(rctx->sg->length - rctx->offset, rctx->total);
-		count = min(count, rctx->buflen - rctx->bufcnt);
+		count = min_t(size_t, count, state->buflen - state->bufcnt);
 
 		if (count <= 0) {
 			if ((rctx->sg->length == 0) && !sg_is_last(rctx->sg)) {
@@ -339,10 +347,10 @@ static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 			}
 		}
 
-		scatterwalk_map_and_copy(rctx->buffer + rctx->bufcnt, rctx->sg,
-					 rctx->offset, count, 0);
+		scatterwalk_map_and_copy(state->buffer + state->bufcnt,
+					 rctx->sg, rctx->offset, count, 0);
 
-		rctx->bufcnt += count;
+		state->bufcnt += count;
 		rctx->offset += count;
 		rctx->total -= count;
 
@@ -411,18 +419,19 @@ static int stm32_hash_xmit_cpu(struct stm32_hash_dev *hdev,
 static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	struct stm32_hash_state *state = &rctx->state;
 	int bufcnt, err = 0, final;
 
-	dev_dbg(hdev->dev, "%s flags %lx\n", __func__, rctx->flags);
+	dev_dbg(hdev->dev, "%s flags %x\n", __func__, state->flags);
 
-	final = rctx->flags & HASH_FLAGS_FINAL;
+	final = state->flags & HASH_FLAGS_FINAL;
 
-	while ((rctx->total >= rctx->buflen) ||
-	       (rctx->bufcnt + rctx->total >= rctx->buflen)) {
+	while ((rctx->total >= state->buflen) ||
+	       (state->bufcnt + rctx->total >= state->buflen)) {
 		stm32_hash_append_sg(rctx);
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 0);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 0);
 		if (err)
 			return err;
 	}
@@ -430,9 +439,9 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 	stm32_hash_append_sg(rctx);
 
 	if (final) {
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 1);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
 	}
 
 	return err;
@@ -576,10 +585,10 @@ static int stm32_hash_dma_init(struct stm32_hash_dev *hdev)
 static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	u32 *buffer = (void *)rctx->state.buffer;
 	struct scatterlist sg[1], *tsg;
 	int err = 0, len = 0, reg, ncp = 0;
 	unsigned int i;
-	u32 *buffer = (void *)rctx->buffer;
 
 	rctx->sg = hdev->req->src;
 	rctx->total = hdev->req->nbytes;
@@ -607,7 +616,7 @@ static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 
 				ncp = sg_pcopy_to_buffer(
 					rctx->sg, rctx->nents,
-					rctx->buffer, sg->length - len,
+					rctx->state.buffer, sg->length - len,
 					rctx->total - sg->length + len);
 
 				sg->length = len;
@@ -718,41 +727,40 @@ static int stm32_hash_init(struct ahash_request *req)
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(tfm);
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 
 	rctx->hdev = hdev;
 
-	rctx->flags = HASH_FLAGS_CPU;
+	state->flags = HASH_FLAGS_CPU;
 
 	rctx->digcnt = crypto_ahash_digestsize(tfm);
 	switch (rctx->digcnt) {
 	case MD5_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_MD5;
+		state->flags |= HASH_FLAGS_MD5;
 		break;
 	case SHA1_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_SHA1;
+		state->flags |= HASH_FLAGS_SHA1;
 		break;
 	case SHA224_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_SHA224;
+		state->flags |= HASH_FLAGS_SHA224;
 		break;
 	case SHA256_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_SHA256;
+		state->flags |= HASH_FLAGS_SHA256;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	rctx->bufcnt = 0;
-	rctx->buflen = HASH_BUFLEN;
+	rctx->state.bufcnt = 0;
+	rctx->state.buflen = HASH_BUFLEN;
 	rctx->total = 0;
 	rctx->offset = 0;
 	rctx->data_type = HASH_DATA_8_BITS;
 
-	memset(rctx->buffer, 0, HASH_BUFLEN);
-
 	if (ctx->flags & HASH_FLAGS_HMAC)
-		rctx->flags |= HASH_FLAGS_HMAC;
+		state->flags |= HASH_FLAGS_HMAC;
 
-	dev_dbg(hdev->dev, "%s Flags %lx\n", __func__, rctx->flags);
+	dev_dbg(hdev->dev, "%s Flags %x\n", __func__, state->flags);
 
 	return 0;
 }
@@ -760,8 +768,9 @@ static int stm32_hash_init(struct ahash_request *req)
 static int stm32_hash_update_req(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	struct stm32_hash_state *state = &rctx->state;
 
-	if (!(rctx->flags & HASH_FLAGS_CPU))
+	if (!(state->flags & HASH_FLAGS_CPU))
 		return stm32_hash_dma_send(hdev);
 
 	return stm32_hash_update_cpu(hdev);
@@ -771,14 +780,12 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 {
 	struct ahash_request *req = hdev->req;
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	int buflen = rctx->bufcnt;
+	struct stm32_hash_state *state = &rctx->state;
 
-	if (rctx->flags & HASH_FLAGS_FINUP)
+	if (state->flags & HASH_FLAGS_FINUP)
 		return stm32_hash_update_req(hdev);
 
-	rctx->bufcnt = 0;
-
-	return stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
+	return stm32_hash_xmit_cpu(hdev, state->buffer, state->buflen, 1);
 }
 
 static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
@@ -813,6 +820,7 @@ static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
 static void stm32_hash_copy_hash(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 	struct stm32_hash_dev *hdev = rctx->hdev;
 	__be32 *hash = (void *)rctx->digest;
 	unsigned int i, hashsize;
@@ -820,7 +828,7 @@ static void stm32_hash_copy_hash(struct ahash_request *req)
 	if (hdev->pdata->broken_emptymsg && !req->nbytes)
 		return stm32_hash_emptymsg_fallback(req);
 
-	switch (rctx->flags & HASH_FLAGS_ALGO_MASK) {
+	switch (state->flags & HASH_FLAGS_ALGO_MASK) {
 	case HASH_FLAGS_MD5:
 		hashsize = MD5_DIGEST_SIZE;
 		break;
@@ -862,6 +870,7 @@ static int stm32_hash_finish(struct ahash_request *req)
 static void stm32_hash_finish_req(struct ahash_request *req, int err)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 	struct stm32_hash_dev *hdev = rctx->hdev;
 
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
@@ -873,7 +882,7 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
 				 HASH_FLAGS_HMAC_KEY);
 	} else {
-		rctx->flags |= HASH_FLAGS_ERRORS;
+		state->flags |= HASH_FLAGS_ERRORS;
 	}
 
 	pm_runtime_mark_last_busy(hdev->dev);
@@ -979,15 +988,16 @@ static int stm32_hash_enqueue(struct ahash_request *req, unsigned int op)
 static int stm32_hash_update(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 
-	if (!req->nbytes || !(rctx->flags & HASH_FLAGS_CPU))
+	if (!req->nbytes || !(state->flags & HASH_FLAGS_CPU))
 		return 0;
 
 	rctx->total = req->nbytes;
 	rctx->sg = req->src;
 	rctx->offset = 0;
 
-	if ((rctx->bufcnt + rctx->total < rctx->buflen)) {
+	if ((state->bufcnt + rctx->total < state->buflen)) {
 		stm32_hash_append_sg(rctx);
 		return 0;
 	}
@@ -998,8 +1008,9 @@ static int stm32_hash_update(struct ahash_request *req)
 static int stm32_hash_final(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 
-	rctx->flags |= HASH_FLAGS_FINAL;
+	state->flags |= HASH_FLAGS_FINAL;
 
 	return stm32_hash_enqueue(req, HASH_OP_FINAL);
 }
@@ -1009,11 +1020,12 @@ static int stm32_hash_finup(struct ahash_request *req)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 
-	rctx->flags |= HASH_FLAGS_FINUP;
+	state->flags |= HASH_FLAGS_FINUP;
 
 	if (hdev->dma_lch && stm32_hash_dma_aligned_data(req))
-		rctx->flags &= ~HASH_FLAGS_CPU;
+		state->flags &= ~HASH_FLAGS_CPU;
 
 	return stm32_hash_final(req);
 }
@@ -1028,6 +1040,7 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 	u32 *preg;
 	unsigned int i;
 	int ret;
@@ -1038,11 +1051,9 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 	if (ret)
 		return ret;
 
-	rctx->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					 sizeof(u32),
-					 GFP_KERNEL);
-
-	preg = rctx->hw_context;
+	state->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
+					  sizeof(u32), GFP_KERNEL);
+	preg = state->hw_context;
 
 	if (!hdev->pdata->ux500)
 		*preg++ = stm32_hash_read(hdev, HASH_IMR);
@@ -1064,13 +1075,14 @@ static int stm32_hash_import(struct ahash_request *req, const void *in)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 	const u32 *preg = in;
 	u32 reg;
 	unsigned int i;
 
 	memcpy(rctx, in, sizeof(*rctx));
 
-	preg = rctx->hw_context;
+	preg = state->hw_context;
 
 	pm_runtime_get_sync(hdev->dev);
 
@@ -1087,7 +1099,7 @@ static int stm32_hash_import(struct ahash_request *req, const void *in)
 	pm_runtime_mark_last_busy(hdev->dev);
 	pm_runtime_put_autosuspend(hdev->dev);
 
-	kfree(rctx->hw_context);
+	kfree(state->hw_context);
 
 	return 0;
 }

[v5 PATCH 6/7] crypto: stm32 - Remove unused HASH_FLAGS_ERRORS

[Herbert Xu]

The bit HASH_FLAGS_ERRORS was never used.  Remove it.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 7cdde961b626..fd9189472235 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -95,7 +95,6 @@
 #define HASH_FLAGS_SHA1			BIT(19)
 #define HASH_FLAGS_SHA224		BIT(20)
 #define HASH_FLAGS_SHA256		BIT(21)
-#define HASH_FLAGS_ERRORS		BIT(22)
 #define HASH_FLAGS_HMAC			BIT(23)
 
 #define HASH_OP_UPDATE			1
@@ -870,7 +869,6 @@ static int stm32_hash_finish(struct ahash_request *req)
 static void stm32_hash_finish_req(struct ahash_request *req, int err)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_state *state = &rctx->state;
 	struct stm32_hash_dev *hdev = rctx->hdev;
 
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
@@ -881,8 +879,6 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 				 HASH_FLAGS_OUTPUT_READY | HASH_FLAGS_HMAC |
 				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
 				 HASH_FLAGS_HMAC_KEY);
-	} else {
-		state->flags |= HASH_FLAGS_ERRORS;
 	}
 
 	pm_runtime_mark_last_busy(hdev->dev);

[v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

The Crypto API hashing paradigm requires the hardware state to
be exported between *each* request because multiple unrelated
hashes may be processed concurrently.

The stm32 hardware is capable of producing the hardware hashing
state but it was only doing it in the export function.  This is
not only broken for export as you can't export a kernel pointer
and reimport it, but it also means that concurrent hashing was
fundamentally broken.

Fix this by moving the saving and restoring of hardware hash
state between each and every hashing request.

Also change the emptymsg check in stm32_hash_copy_hash to rely
on whether we have any existing hash state, rather than whether
this particular update request is empty.  

Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
Reported-by: Li kunyu <kunyu@nfschina.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |  172 +++++++++++++-------------------------
 1 file changed, 62 insertions(+), 110 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index fd9189472235..85da2aa93017 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -95,6 +95,7 @@
 #define HASH_FLAGS_SHA1			BIT(19)
 #define HASH_FLAGS_SHA224		BIT(20)
 #define HASH_FLAGS_SHA256		BIT(21)
+#define HASH_FLAGS_EMPTY		BIT(22)
 #define HASH_FLAGS_HMAC			BIT(23)
 
 #define HASH_OP_UPDATE			1
@@ -134,7 +135,7 @@ struct stm32_hash_state {
 	u8 buffer[HASH_BUFLEN] __aligned(4);
 
 	/* hash state */
-	u32			*hw_context;
+	u32			hw_context[3 + HASH_CSR_REGISTER_NUMBER];
 };
 
 struct stm32_hash_request_ctx {
@@ -314,8 +315,11 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 		 * On the Ux500 we need to set a special flag to indicate that
 		 * the message is zero length.
 		 */
-		if (hdev->pdata->ux500 && bufcnt == 0)
+		if (hdev->pdata->ux500 && bufcnt == 0 &&
+		    (state->flags & HASH_FLAGS_FINAL)) {
 			reg |= HASH_CR_UX500_EMPTYMSG;
+			state->flags |= HASH_FLAGS_EMPTY;
+		}
 
 		if (!hdev->polled)
 			stm32_hash_write(hdev, HASH_IMR, HASH_DCIE);
@@ -419,7 +423,9 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
 	struct stm32_hash_state *state = &rctx->state;
+	u32 *preg = state->hw_context;
 	int bufcnt, err = 0, final;
+	int i;
 
 	dev_dbg(hdev->dev, "%s flags %x\n", __func__, state->flags);
 
@@ -440,9 +446,24 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 	if (final) {
 		bufcnt = state->bufcnt;
 		state->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
+		return stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
 	}
 
+	if (!(hdev->flags & HASH_FLAGS_INIT))
+		return 0;
+
+	if (stm32_hash_wait_busy(hdev))
+		return -ETIMEDOUT;
+
+	if (!hdev->pdata->ux500)
+		*preg++ = stm32_hash_read(hdev, HASH_IMR);
+	*preg++ = stm32_hash_read(hdev, HASH_STR);
+	*preg++ = stm32_hash_read(hdev, HASH_CR);
+	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
+
+	state->flags |= HASH_FLAGS_INIT;
+
 	return err;
 }
 
@@ -824,7 +845,7 @@ static void stm32_hash_copy_hash(struct ahash_request *req)
 	__be32 *hash = (void *)rctx->digest;
 	unsigned int i, hashsize;
 
-	if (hdev->pdata->broken_emptymsg && !req->nbytes)
+	if (hdev->pdata->broken_emptymsg && (state->flags & HASH_FLAGS_EMPTY))
 		return stm32_hash_emptymsg_fallback(req);
 
 	switch (state->flags & HASH_FLAGS_ALGO_MASK) {
@@ -874,11 +895,6 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
 		stm32_hash_copy_hash(req);
 		err = stm32_hash_finish(req);
-		hdev->flags &= ~(HASH_FLAGS_FINAL | HASH_FLAGS_CPU |
-				 HASH_FLAGS_INIT | HASH_FLAGS_DMA_READY |
-				 HASH_FLAGS_OUTPUT_READY | HASH_FLAGS_HMAC |
-				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
-				 HASH_FLAGS_HMAC_KEY);
 	}
 
 	pm_runtime_mark_last_busy(hdev->dev);
@@ -887,66 +903,54 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	crypto_finalize_hash_request(hdev->engine, req, err);
 }
 
-static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
-			      struct stm32_hash_request_ctx *rctx)
-{
-	pm_runtime_get_sync(hdev->dev);
-
-	if (!(HASH_FLAGS_INIT & hdev->flags)) {
-		stm32_hash_write(hdev, HASH_CR, HASH_CR_INIT);
-		stm32_hash_write(hdev, HASH_STR, 0);
-		stm32_hash_write(hdev, HASH_DIN, 0);
-		stm32_hash_write(hdev, HASH_IMR, 0);
-	}
-
-	return 0;
-}
-
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq);
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq);
-
 static int stm32_hash_handle_queue(struct stm32_hash_dev *hdev,
 				   struct ahash_request *req)
 {
 	return crypto_transfer_hash_request_to_engine(hdev->engine, req);
 }
 
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq)
+static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 {
 	struct ahash_request *req = container_of(areq, struct ahash_request,
 						 base);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
+	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_request_ctx *rctx;
+	struct stm32_hash_state *state = &rctx->state;
+	int err = 0;
 
 	if (!hdev)
 		return -ENODEV;
 
-	hdev->req = req;
-
-	rctx = ahash_request_ctx(req);
-
 	dev_dbg(hdev->dev, "processing new req, op: %lu, nbytes %d\n",
 		rctx->op, req->nbytes);
 
-	return stm32_hash_hw_init(hdev, rctx);
-}
+	pm_runtime_get_sync(hdev->dev);
 
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
-{
-	struct ahash_request *req = container_of(areq, struct ahash_request,
-						 base);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_request_ctx *rctx;
-	int err = 0;
+	hdev->req = req;
+	hdev->flags = 0;
+
+	if (state->flags & HASH_FLAGS_INIT) {
+		u32 *preg = rctx->state.hw_context;
+		u32 reg;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			stm32_hash_write(hdev, HASH_IMR, *preg++);
+		stm32_hash_write(hdev, HASH_STR, *preg++);
+		stm32_hash_write(hdev, HASH_CR, *preg);
+		reg = *preg++ | HASH_CR_INIT;
+		stm32_hash_write(hdev, HASH_CR, reg);
 
-	if (!hdev)
-		return -ENODEV;
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			stm32_hash_write(hdev, HASH_CSR(i), *preg++);
 
-	hdev->req = req;
+		hdev->flags |= HASH_FLAGS_INIT;
 
-	rctx = ahash_request_ctx(req);
+		if (state->flags & HASH_FLAGS_HMAC)
+			hdev->flags |= HASH_FLAGS_HMAC |
+				       HASH_FLAGS_HMAC_KEY;
+	}
 
 	if (rctx->op == HASH_OP_UPDATE)
 		err = stm32_hash_update_req(hdev);
@@ -1034,34 +1038,8 @@ static int stm32_hash_digest(struct ahash_request *req)
 static int stm32_hash_export(struct ahash_request *req, void *out)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_state *state = &rctx->state;
-	u32 *preg;
-	unsigned int i;
-	int ret;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	ret = stm32_hash_wait_busy(hdev);
-	if (ret)
-		return ret;
-
-	state->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					  sizeof(u32), GFP_KERNEL);
-	preg = state->hw_context;
 
-	if (!hdev->pdata->ux500)
-		*preg++ = stm32_hash_read(hdev, HASH_IMR);
-	*preg++ = stm32_hash_read(hdev, HASH_STR);
-	*preg++ = stm32_hash_read(hdev, HASH_CR);
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
-
-	memcpy(out, rctx, sizeof(*rctx));
+	memcpy(out, &rctx->state, sizeof(rctx->state));
 
 	return 0;
 }
@@ -1069,33 +1047,9 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 static int stm32_hash_import(struct ahash_request *req, const void *in)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_state *state = &rctx->state;
-	const u32 *preg = in;
-	u32 reg;
-	unsigned int i;
-
-	memcpy(rctx, in, sizeof(*rctx));
-
-	preg = state->hw_context;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	if (!hdev->pdata->ux500)
-		stm32_hash_write(hdev, HASH_IMR, *preg++);
-	stm32_hash_write(hdev, HASH_STR, *preg++);
-	stm32_hash_write(hdev, HASH_CR, *preg);
-	reg = *preg++ | HASH_CR_INIT;
-	stm32_hash_write(hdev, HASH_CR, reg);
-
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		stm32_hash_write(hdev, HASH_CSR(i), *preg++);
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
 
-	kfree(state->hw_context);
+	stm32_hash_init(req);
+	memcpy(&rctx->state, in, sizeof(rctx->state));
 
 	return 0;
 }
@@ -1152,8 +1106,6 @@ static int stm32_hash_cra_init_algs(struct crypto_tfm *tfm,
 		ctx->flags |= HASH_FLAGS_HMAC;
 
 	ctx->enginectx.op.do_one_request = stm32_hash_one_request;
-	ctx->enginectx.op.prepare_request = stm32_hash_prepare_req;
-	ctx->enginectx.op.unprepare_request = NULL;
 
 	return stm32_hash_init_fallback(tfm);
 }
@@ -1245,7 +1197,7 @@ static struct ahash_alg algs_md5[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "md5",
 				.cra_driver_name = "stm32-md5",
@@ -1272,7 +1224,7 @@ static struct ahash_alg algs_md5[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(md5)",
 				.cra_driver_name = "stm32-hmac-md5",
@@ -1301,7 +1253,7 @@ static struct ahash_alg algs_sha1[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "sha1",
 				.cra_driver_name = "stm32-sha1",
@@ -1328,7 +1280,7 @@ static struct ahash_alg algs_sha1[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(sha1)",
 				.cra_driver_name = "stm32-hmac-sha1",
@@ -1357,7 +1309,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "sha224",
 				.cra_driver_name = "stm32-sha224",
@@ -1384,7 +1336,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(sha224)",
 				.cra_driver_name = "stm32-hmac-sha224",
@@ -1413,7 +1365,7 @@ static struct ahash_alg algs_sha256[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "sha256",
 				.cra_driver_name = "stm32-sha256",
@@ -1440,7 +1392,7 @@ static struct ahash_alg algs_sha256[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(sha256)",
 				.cra_driver_name = "stm32-hmac-sha256",

[v6 PATCH 0/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Sat, Mar 04, 2023 at 05:34:04PM +0800, Herbert Xu wrote:
> 
> I've split the patch up into smaller chunks for easier testing.

v6 fixes a bug in the finup patch that caused the new data to
be discarded instead of hashed.

This patch series fixes the import/export functions in the stm32
driver.  As usual, a failure in import/export indicates a general
bug in the hash driver that may break as soon as two concurrent
users show up and hash at the same time using any method other
than digest or init+finup.

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

[v5 PATCH 1/7] crypto: stm32 - Save 54 CSR registers

[Herbert Xu]

The CSR registers go from 0 to 53.  So the number of registers
should be 54.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 7bf805563ac2..bde2b40a6a32 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -68,7 +68,7 @@
 #define HASH_MASK_DATA_INPUT		BIT(1)
 
 /* Context swap register */
-#define HASH_CSR_REGISTER_NUMBER	53
+#define HASH_CSR_REGISTER_NUMBER	54
 
 /* Status Flags */
 #define HASH_SR_DATA_INPUT_READY	BIT(0)

[v5 PATCH 2/7] crypto: stm32 - Move polling into do_one_request

[Herbert Xu]

There is no need to poll separate for update and final.  We could
merge them into do_one_request.

Also fix the error handling so that we don't poll (and overwrite
the error) when an error has already occurred.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |   29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index bde2b40a6a32..298cabd29e36 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -425,6 +425,8 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 		bufcnt = rctx->bufcnt;
 		rctx->bufcnt = 0;
 		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 0);
+		if (err)
+			return err;
 	}
 
 	stm32_hash_append_sg(rctx);
@@ -433,14 +435,6 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 		bufcnt = rctx->bufcnt;
 		rctx->bufcnt = 0;
 		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 1);
-
-		/* If we have an IRQ, wait for that, else poll for completion */
-		if (hdev->polled) {
-			if (stm32_hash_wait_busy(hdev))
-				return -ETIMEDOUT;
-			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
-			err = 0;
-		}
 	}
 
 	return err;
@@ -784,15 +778,6 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 	else
 		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
 
-	/* If we have an IRQ, wait for that, else poll for completion */
-	if (hdev->polled) {
-		if (stm32_hash_wait_busy(hdev))
-			return -ETIMEDOUT;
-		hdev->flags |= HASH_FLAGS_OUTPUT_READY;
-		/* Caller will call stm32_hash_finish_req() */
-		err = 0;
-	}
-
 	return err;
 }
 
@@ -964,6 +949,16 @@ static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 	else if (rctx->op == HASH_OP_FINAL)
 		err = stm32_hash_final_req(hdev);
 
+	/* If we have an IRQ, wait for that, else poll for completion */
+	if (err == -EINPROGRESS && hdev->polled) {
+		if (stm32_hash_wait_busy(hdev))
+			err = -ETIMEDOUT;
+		else {
+			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
+			err = 0;
+		}
+	}
+
 	if (err != -EINPROGRESS)
 	/* done task will not finish it, so do it here */
 		stm32_hash_finish_req(req, err);

[v5 PATCH 3/7] crypto: stm32 - Simplify finup

[Herbert Xu]

The current finup code is unnecessarily convoluted.  There is no
need to call update and final separately as update already does
all the necessary work on its own.

Simplify this by utilising the HASH_FLAGS_FINUP bit in rctx to
indicate only finup and use the HASH_FLAGS_FINAL bit instead to
signify processing common to both final and finup.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |   41 +++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 298cabd29e36..e16f9aaec6bf 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -417,7 +417,7 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 
 	dev_dbg(hdev->dev, "%s flags %lx\n", __func__, rctx->flags);
 
-	final = (rctx->flags & HASH_FLAGS_FINUP);
+	final = rctx->flags & HASH_FLAGS_FINAL;
 
 	while ((rctx->total >= rctx->buflen) ||
 	       (rctx->bufcnt + rctx->total >= rctx->buflen)) {
@@ -761,6 +761,11 @@ static int stm32_hash_init(struct ahash_request *req)
 
 static int stm32_hash_update_req(struct stm32_hash_dev *hdev)
 {
+	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+
+	if (!(rctx->flags & HASH_FLAGS_CPU))
+		return stm32_hash_dma_send(hdev);
+
 	return stm32_hash_update_cpu(hdev);
 }
 
@@ -768,17 +773,14 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 {
 	struct ahash_request *req = hdev->req;
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	int err;
 	int buflen = rctx->bufcnt;
 
-	rctx->bufcnt = 0;
+	if (rctx->flags & HASH_FLAGS_FINUP)
+		return stm32_hash_update_req(hdev);
 
-	if (!(rctx->flags & HASH_FLAGS_CPU))
-		err = stm32_hash_dma_send(hdev);
-	else
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
+	rctx->bufcnt = 0;
 
-	return err;
+	return stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
 }
 
 static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
@@ -1000,7 +1002,7 @@ static int stm32_hash_final(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 
-	rctx->flags |= HASH_FLAGS_FINUP;
+	rctx->flags |= HASH_FLAGS_FINAL;
 
 	return stm32_hash_enqueue(req, HASH_OP_FINAL);
 }
@@ -1010,25 +1012,20 @@ static int stm32_hash_finup(struct ahash_request *req)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	int err1, err2;
+
+	if (!req->nbytes)
+		goto out;
 
 	rctx->flags |= HASH_FLAGS_FINUP;
+	rctx->total = req->nbytes;
+	rctx->sg = req->src;
+	rctx->offset = 0;
 
 	if (hdev->dma_lch && stm32_hash_dma_aligned_data(req))
 		rctx->flags &= ~HASH_FLAGS_CPU;
 
-	err1 = stm32_hash_update(req);
-
-	if (err1 == -EINPROGRESS || err1 == -EBUSY)
-		return err1;
-
-	/*
-	 * final() has to be always called to cleanup resources
-	 * even if update() failed, except EINPROGRESS
-	 */
-	err2 = stm32_hash_final(req);
-
-	return err1 ?: err2;
+out:
+	return stm32_hash_final(req);
 }
 
 static int stm32_hash_digest(struct ahash_request *req)

Re: [v5 PATCH 3/7] crypto: stm32 - Simplify finup

[Linus Walleij]

On Mon, Mar 6, 2023 at 5:42 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The current finup code is unnecessarily convoluted.  There is no
> need to call update and final separately as update already does
> all the necessary work on its own.
>
> Simplify this by utilising the HASH_FLAGS_FINUP bit in rctx to
> indicate only finup and use the HASH_FLAGS_FINAL bit instead to
> signify processing common to both final and finup.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This "v5.5 version" indeed works!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

I had to pick the patch manually because Konstantin's b4 tool
got completely confused by the version numbering and picked
the old v5, but once I got the right one, all worked fine!

Yours,
Linus Walleij

[v5 PATCH 4/7] crypto: stm32 - Remove unused hdev->err field

[Herbert Xu]

The variable hdev->err is never read so it can be removed.

Also remove a spurious inclusion of linux/crypto.h.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index e16f9aaec6bf..e35fee945371 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -7,7 +7,6 @@
  */
 
 #include <linux/clk.h>
-#include <linux/crypto.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
@@ -183,7 +182,6 @@ struct stm32_hash_dev {
 	struct ahash_request	*req;
 	struct crypto_engine	*engine;
 
-	int			err;
 	unsigned long		flags;
 
 	struct dma_chan		*dma_lch;
@@ -894,7 +892,6 @@ static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
 		stm32_hash_write(hdev, HASH_STR, 0);
 		stm32_hash_write(hdev, HASH_DIN, 0);
 		stm32_hash_write(hdev, HASH_IMR, 0);
-		hdev->err = 0;
 	}
 
 	return 0;

[v5 PATCH 5/7] crypto: stm32 - Move hash state into separate structure

[Herbert Xu]

Create a new struct stm32_hash_state so that it may be exported
in future instead of the entire request context.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |  126 ++++++++++++++++++++------------------
 1 file changed, 69 insertions(+), 57 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index e35fee945371..de8275a80271 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -126,15 +126,24 @@ struct stm32_hash_ctx {
 	int			keylen;
 };
 
+struct stm32_hash_state {
+	u32			flags;
+
+	u16			bufcnt;
+	u16			buflen;
+
+	u8 buffer[HASH_BUFLEN] __aligned(4);
+
+	/* hash state */
+	u32			*hw_context;
+};
+
 struct stm32_hash_request_ctx {
 	struct stm32_hash_dev	*hdev;
-	unsigned long		flags;
 	unsigned long		op;
 
 	u8 digest[SHA256_DIGEST_SIZE] __aligned(sizeof(u32));
 	size_t			digcnt;
-	size_t			bufcnt;
-	size_t			buflen;
 
 	/* DMA */
 	struct scatterlist	*sg;
@@ -148,10 +157,7 @@ struct stm32_hash_request_ctx {
 
 	u8			data_type;
 
-	u8 buffer[HASH_BUFLEN] __aligned(sizeof(u32));
-
-	/* Export Context */
-	u32			*hw_context;
+	struct stm32_hash_state state;
 };
 
 struct stm32_hash_algs_info {
@@ -268,11 +274,12 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
 	struct crypto_ahash *tfm = crypto_ahash_reqtfm(hdev->req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(tfm);
+	struct stm32_hash_state *state = &rctx->state;
 
 	u32 reg = HASH_CR_INIT;
 
 	if (!(hdev->flags & HASH_FLAGS_INIT)) {
-		switch (rctx->flags & HASH_FLAGS_ALGO_MASK) {
+		switch (state->flags & HASH_FLAGS_ALGO_MASK) {
 		case HASH_FLAGS_MD5:
 			reg |= HASH_CR_ALGO_MD5;
 			break;
@@ -297,7 +304,7 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 
 		reg |= (rctx->data_type << HASH_CR_DATATYPE_POS);
 
-		if (rctx->flags & HASH_FLAGS_HMAC) {
+		if (state->flags & HASH_FLAGS_HMAC) {
 			hdev->flags |= HASH_FLAGS_HMAC;
 			reg |= HASH_CR_MODE;
 			if (ctx->keylen > HASH_LONG_KEY)
@@ -324,11 +331,12 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 
 static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 {
+	struct stm32_hash_state *state = &rctx->state;
 	size_t count;
 
-	while ((rctx->bufcnt < rctx->buflen) && rctx->total) {
+	while ((state->bufcnt < state->buflen) && rctx->total) {
 		count = min(rctx->sg->length - rctx->offset, rctx->total);
-		count = min(count, rctx->buflen - rctx->bufcnt);
+		count = min_t(size_t, count, state->buflen - state->bufcnt);
 
 		if (count <= 0) {
 			if ((rctx->sg->length == 0) && !sg_is_last(rctx->sg)) {
@@ -339,10 +347,10 @@ static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 			}
 		}
 
-		scatterwalk_map_and_copy(rctx->buffer + rctx->bufcnt, rctx->sg,
-					 rctx->offset, count, 0);
+		scatterwalk_map_and_copy(state->buffer + state->bufcnt,
+					 rctx->sg, rctx->offset, count, 0);
 
-		rctx->bufcnt += count;
+		state->bufcnt += count;
 		rctx->offset += count;
 		rctx->total -= count;
 
@@ -411,18 +419,19 @@ static int stm32_hash_xmit_cpu(struct stm32_hash_dev *hdev,
 static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	struct stm32_hash_state *state = &rctx->state;
 	int bufcnt, err = 0, final;
 
-	dev_dbg(hdev->dev, "%s flags %lx\n", __func__, rctx->flags);
+	dev_dbg(hdev->dev, "%s flags %x\n", __func__, state->flags);
 
-	final = rctx->flags & HASH_FLAGS_FINAL;
+	final = state->flags & HASH_FLAGS_FINAL;
 
-	while ((rctx->total >= rctx->buflen) ||
-	       (rctx->bufcnt + rctx->total >= rctx->buflen)) {
+	while ((rctx->total >= state->buflen) ||
+	       (state->bufcnt + rctx->total >= state->buflen)) {
 		stm32_hash_append_sg(rctx);
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 0);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 0);
 		if (err)
 			return err;
 	}
@@ -430,9 +439,9 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 	stm32_hash_append_sg(rctx);
 
 	if (final) {
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 1);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
 	}
 
 	return err;
@@ -576,10 +585,10 @@ static int stm32_hash_dma_init(struct stm32_hash_dev *hdev)
 static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	u32 *buffer = (void *)rctx->state.buffer;
 	struct scatterlist sg[1], *tsg;
 	int err = 0, len = 0, reg, ncp = 0;
 	unsigned int i;
-	u32 *buffer = (void *)rctx->buffer;
 
 	rctx->sg = hdev->req->src;
 	rctx->total = hdev->req->nbytes;
@@ -607,7 +616,7 @@ static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 
 				ncp = sg_pcopy_to_buffer(
 					rctx->sg, rctx->nents,
-					rctx->buffer, sg->length - len,
+					rctx->state.buffer, sg->length - len,
 					rctx->total - sg->length + len);
 
 				sg->length = len;
@@ -718,41 +727,40 @@ static int stm32_hash_init(struct ahash_request *req)
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(tfm);
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 
 	rctx->hdev = hdev;
 
-	rctx->flags = HASH_FLAGS_CPU;
+	state->flags = HASH_FLAGS_CPU;
 
 	rctx->digcnt = crypto_ahash_digestsize(tfm);
 	switch (rctx->digcnt) {
 	case MD5_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_MD5;
+		state->flags |= HASH_FLAGS_MD5;
 		break;
 	case SHA1_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_SHA1;
+		state->flags |= HASH_FLAGS_SHA1;
 		break;
 	case SHA224_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_SHA224;
+		state->flags |= HASH_FLAGS_SHA224;
 		break;
 	case SHA256_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_SHA256;
+		state->flags |= HASH_FLAGS_SHA256;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	rctx->bufcnt = 0;
-	rctx->buflen = HASH_BUFLEN;
+	rctx->state.bufcnt = 0;
+	rctx->state.buflen = HASH_BUFLEN;
 	rctx->total = 0;
 	rctx->offset = 0;
 	rctx->data_type = HASH_DATA_8_BITS;
 
-	memset(rctx->buffer, 0, HASH_BUFLEN);
-
 	if (ctx->flags & HASH_FLAGS_HMAC)
-		rctx->flags |= HASH_FLAGS_HMAC;
+		state->flags |= HASH_FLAGS_HMAC;
 
-	dev_dbg(hdev->dev, "%s Flags %lx\n", __func__, rctx->flags);
+	dev_dbg(hdev->dev, "%s Flags %x\n", __func__, state->flags);
 
 	return 0;
 }
@@ -760,8 +768,9 @@ static int stm32_hash_init(struct ahash_request *req)
 static int stm32_hash_update_req(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	struct stm32_hash_state *state = &rctx->state;
 
-	if (!(rctx->flags & HASH_FLAGS_CPU))
+	if (!(state->flags & HASH_FLAGS_CPU))
 		return stm32_hash_dma_send(hdev);
 
 	return stm32_hash_update_cpu(hdev);
@@ -771,14 +780,12 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 {
 	struct ahash_request *req = hdev->req;
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	int buflen = rctx->bufcnt;
+	struct stm32_hash_state *state = &rctx->state;
 
-	if (rctx->flags & HASH_FLAGS_FINUP)
+	if (state->flags & HASH_FLAGS_FINUP)
 		return stm32_hash_update_req(hdev);
 
-	rctx->bufcnt = 0;
-
-	return stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
+	return stm32_hash_xmit_cpu(hdev, state->buffer, state->buflen, 1);
 }
 
 static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
@@ -813,6 +820,7 @@ static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
 static void stm32_hash_copy_hash(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 	struct stm32_hash_dev *hdev = rctx->hdev;
 	__be32 *hash = (void *)rctx->digest;
 	unsigned int i, hashsize;
@@ -820,7 +828,7 @@ static void stm32_hash_copy_hash(struct ahash_request *req)
 	if (hdev->pdata->broken_emptymsg && !req->nbytes)
 		return stm32_hash_emptymsg_fallback(req);
 
-	switch (rctx->flags & HASH_FLAGS_ALGO_MASK) {
+	switch (state->flags & HASH_FLAGS_ALGO_MASK) {
 	case HASH_FLAGS_MD5:
 		hashsize = MD5_DIGEST_SIZE;
 		break;
@@ -862,6 +870,7 @@ static int stm32_hash_finish(struct ahash_request *req)
 static void stm32_hash_finish_req(struct ahash_request *req, int err)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 	struct stm32_hash_dev *hdev = rctx->hdev;
 
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
@@ -873,7 +882,7 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
 				 HASH_FLAGS_HMAC_KEY);
 	} else {
-		rctx->flags |= HASH_FLAGS_ERRORS;
+		state->flags |= HASH_FLAGS_ERRORS;
 	}
 
 	pm_runtime_mark_last_busy(hdev->dev);
@@ -979,15 +988,16 @@ static int stm32_hash_enqueue(struct ahash_request *req, unsigned int op)
 static int stm32_hash_update(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 
-	if (!req->nbytes || !(rctx->flags & HASH_FLAGS_CPU))
+	if (!req->nbytes || !(state->flags & HASH_FLAGS_CPU))
 		return 0;
 
 	rctx->total = req->nbytes;
 	rctx->sg = req->src;
 	rctx->offset = 0;
 
-	if ((rctx->bufcnt + rctx->total < rctx->buflen)) {
+	if ((state->bufcnt + rctx->total < state->buflen)) {
 		stm32_hash_append_sg(rctx);
 		return 0;
 	}
@@ -998,8 +1008,9 @@ static int stm32_hash_update(struct ahash_request *req)
 static int stm32_hash_final(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 
-	rctx->flags |= HASH_FLAGS_FINAL;
+	state->flags |= HASH_FLAGS_FINAL;
 
 	return stm32_hash_enqueue(req, HASH_OP_FINAL);
 }
@@ -1009,17 +1020,18 @@ static int stm32_hash_finup(struct ahash_request *req)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 
 	if (!req->nbytes)
 		goto out;
 
-	rctx->flags |= HASH_FLAGS_FINUP;
+	state->flags |= HASH_FLAGS_FINUP;
 	rctx->total = req->nbytes;
 	rctx->sg = req->src;
 	rctx->offset = 0;
 
 	if (hdev->dma_lch && stm32_hash_dma_aligned_data(req))
-		rctx->flags &= ~HASH_FLAGS_CPU;
+		state->flags &= ~HASH_FLAGS_CPU;
 
 out:
 	return stm32_hash_final(req);
@@ -1035,6 +1047,7 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 	u32 *preg;
 	unsigned int i;
 	int ret;
@@ -1045,11 +1058,9 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 	if (ret)
 		return ret;
 
-	rctx->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					 sizeof(u32),
-					 GFP_KERNEL);
-
-	preg = rctx->hw_context;
+	state->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
+					  sizeof(u32), GFP_KERNEL);
+	preg = state->hw_context;
 
 	if (!hdev->pdata->ux500)
 		*preg++ = stm32_hash_read(hdev, HASH_IMR);
@@ -1071,13 +1082,14 @@ static int stm32_hash_import(struct ahash_request *req, const void *in)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 	const u32 *preg = in;
 	u32 reg;
 	unsigned int i;
 
 	memcpy(rctx, in, sizeof(*rctx));
 
-	preg = rctx->hw_context;
+	preg = state->hw_context;
 
 	pm_runtime_get_sync(hdev->dev);
 
@@ -1094,7 +1106,7 @@ static int stm32_hash_import(struct ahash_request *req, const void *in)
 	pm_runtime_mark_last_busy(hdev->dev);
 	pm_runtime_put_autosuspend(hdev->dev);
 
-	kfree(rctx->hw_context);
+	kfree(state->hw_context);
 
 	return 0;
 }

Re: [v5 PATCH 5/7] crypto: stm32 - Move hash state into separate structure

[Linus Walleij]

Hi Herbert,

thanks for keeping going at this!

On Mon, Mar 6, 2023 at 5:42 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Create a new struct stm32_hash_state so that it may be exported
> in future instead of the entire request context.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

First the patch bugged but I found the problem in a small semantic
glitch:

diff --git a/drivers/crypto/stm32/stm32-hash.c
b/drivers/crypto/stm32/stm32-hash.c
index de8275a80271..3743f55b5c04 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -785,7 +785,7 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
        if (state->flags & HASH_FLAGS_FINUP)
                return stm32_hash_update_req(hdev);

-       return stm32_hash_xmit_cpu(hdev, state->buffer, state->buflen, 1);
+       return stm32_hash_xmit_cpu(hdev, state->buffer, state->bufcnt, 1);
 }

 static void stm32_hash_emptymsg_fallback(struct ahash_request *req)

Afte this all (extended) tests pass fine.

Just fold in this and you can add:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[v5 PATCH 6/7] crypto: stm32 - Remove unused HASH_FLAGS_ERRORS

[Herbert Xu]

The bit HASH_FLAGS_ERRORS was never used.  Remove it.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index de8275a80271..95cd8876689b 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -95,7 +95,6 @@
 #define HASH_FLAGS_SHA1			BIT(19)
 #define HASH_FLAGS_SHA224		BIT(20)
 #define HASH_FLAGS_SHA256		BIT(21)
-#define HASH_FLAGS_ERRORS		BIT(22)
 #define HASH_FLAGS_HMAC			BIT(23)
 
 #define HASH_OP_UPDATE			1
@@ -870,7 +869,6 @@ static int stm32_hash_finish(struct ahash_request *req)
 static void stm32_hash_finish_req(struct ahash_request *req, int err)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_state *state = &rctx->state;
 	struct stm32_hash_dev *hdev = rctx->hdev;
 
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
@@ -881,8 +879,6 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 				 HASH_FLAGS_OUTPUT_READY | HASH_FLAGS_HMAC |
 				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
 				 HASH_FLAGS_HMAC_KEY);
-	} else {
-		state->flags |= HASH_FLAGS_ERRORS;
 	}
 
 	pm_runtime_mark_last_busy(hdev->dev);

Re: [v5 PATCH 6/7] crypto: stm32 - Remove unused HASH_FLAGS_ERRORS

[Linus Walleij]

On Mon, Mar 6, 2023 at 5:42 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The bit HASH_FLAGS_ERRORS was never used.  Remove it.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

The Crypto API hashing paradigm requires the hardware state to
be exported between *each* request because multiple unrelated
hashes may be processed concurrently.

The stm32 hardware is capable of producing the hardware hashing
state but it was only doing it in the export function.  This is
not only broken for export as you can't export a kernel pointer
and reimport it, but it also means that concurrent hashing was
fundamentally broken.

Fix this by moving the saving and restoring of hardware hash
state between each and every hashing request.

Also change the emptymsg check in stm32_hash_copy_hash to rely
on whether we have any existing hash state, rather than whether
this particular update request is empty.  

Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
Reported-by: Li kunyu <kunyu@nfschina.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |  172 +++++++++++++-------------------------
 1 file changed, 62 insertions(+), 110 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 95cd8876689b..e87a909dd97e 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -95,6 +95,7 @@
 #define HASH_FLAGS_SHA1			BIT(19)
 #define HASH_FLAGS_SHA224		BIT(20)
 #define HASH_FLAGS_SHA256		BIT(21)
+#define HASH_FLAGS_EMPTY		BIT(22)
 #define HASH_FLAGS_HMAC			BIT(23)
 
 #define HASH_OP_UPDATE			1
@@ -134,7 +135,7 @@ struct stm32_hash_state {
 	u8 buffer[HASH_BUFLEN] __aligned(4);
 
 	/* hash state */
-	u32			*hw_context;
+	u32			hw_context[3 + HASH_CSR_REGISTER_NUMBER];
 };
 
 struct stm32_hash_request_ctx {
@@ -314,8 +315,11 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 		 * On the Ux500 we need to set a special flag to indicate that
 		 * the message is zero length.
 		 */
-		if (hdev->pdata->ux500 && bufcnt == 0)
+		if (hdev->pdata->ux500 && bufcnt == 0 &&
+		    (state->flags & HASH_FLAGS_FINAL)) {
 			reg |= HASH_CR_UX500_EMPTYMSG;
+			state->flags |= HASH_FLAGS_EMPTY;
+		}
 
 		if (!hdev->polled)
 			stm32_hash_write(hdev, HASH_IMR, HASH_DCIE);
@@ -419,7 +423,9 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
 	struct stm32_hash_state *state = &rctx->state;
+	u32 *preg = state->hw_context;
 	int bufcnt, err = 0, final;
+	int i;
 
 	dev_dbg(hdev->dev, "%s flags %x\n", __func__, state->flags);
 
@@ -440,9 +446,24 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 	if (final) {
 		bufcnt = state->bufcnt;
 		state->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
+		return stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
 	}
 
+	if (!(hdev->flags & HASH_FLAGS_INIT))
+		return 0;
+
+	if (stm32_hash_wait_busy(hdev))
+		return -ETIMEDOUT;
+
+	if (!hdev->pdata->ux500)
+		*preg++ = stm32_hash_read(hdev, HASH_IMR);
+	*preg++ = stm32_hash_read(hdev, HASH_STR);
+	*preg++ = stm32_hash_read(hdev, HASH_CR);
+	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
+
+	state->flags |= HASH_FLAGS_INIT;
+
 	return err;
 }
 
@@ -824,7 +845,7 @@ static void stm32_hash_copy_hash(struct ahash_request *req)
 	__be32 *hash = (void *)rctx->digest;
 	unsigned int i, hashsize;
 
-	if (hdev->pdata->broken_emptymsg && !req->nbytes)
+	if (hdev->pdata->broken_emptymsg && (state->flags & HASH_FLAGS_EMPTY))
 		return stm32_hash_emptymsg_fallback(req);
 
 	switch (state->flags & HASH_FLAGS_ALGO_MASK) {
@@ -874,11 +895,6 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
 		stm32_hash_copy_hash(req);
 		err = stm32_hash_finish(req);
-		hdev->flags &= ~(HASH_FLAGS_FINAL | HASH_FLAGS_CPU |
-				 HASH_FLAGS_INIT | HASH_FLAGS_DMA_READY |
-				 HASH_FLAGS_OUTPUT_READY | HASH_FLAGS_HMAC |
-				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
-				 HASH_FLAGS_HMAC_KEY);
 	}
 
 	pm_runtime_mark_last_busy(hdev->dev);
@@ -887,66 +903,54 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	crypto_finalize_hash_request(hdev->engine, req, err);
 }
 
-static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
-			      struct stm32_hash_request_ctx *rctx)
-{
-	pm_runtime_get_sync(hdev->dev);
-
-	if (!(HASH_FLAGS_INIT & hdev->flags)) {
-		stm32_hash_write(hdev, HASH_CR, HASH_CR_INIT);
-		stm32_hash_write(hdev, HASH_STR, 0);
-		stm32_hash_write(hdev, HASH_DIN, 0);
-		stm32_hash_write(hdev, HASH_IMR, 0);
-	}
-
-	return 0;
-}
-
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq);
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq);
-
 static int stm32_hash_handle_queue(struct stm32_hash_dev *hdev,
 				   struct ahash_request *req)
 {
 	return crypto_transfer_hash_request_to_engine(hdev->engine, req);
 }
 
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq)
+static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 {
 	struct ahash_request *req = container_of(areq, struct ahash_request,
 						 base);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
+	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_request_ctx *rctx;
+	struct stm32_hash_state *state = &rctx->state;
+	int err = 0;
 
 	if (!hdev)
 		return -ENODEV;
 
-	hdev->req = req;
-
-	rctx = ahash_request_ctx(req);
-
 	dev_dbg(hdev->dev, "processing new req, op: %lu, nbytes %d\n",
 		rctx->op, req->nbytes);
 
-	return stm32_hash_hw_init(hdev, rctx);
-}
+	pm_runtime_get_sync(hdev->dev);
 
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
-{
-	struct ahash_request *req = container_of(areq, struct ahash_request,
-						 base);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_request_ctx *rctx;
-	int err = 0;
+	hdev->req = req;
+	hdev->flags = 0;
+
+	if (state->flags & HASH_FLAGS_INIT) {
+		u32 *preg = rctx->state.hw_context;
+		u32 reg;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			stm32_hash_write(hdev, HASH_IMR, *preg++);
+		stm32_hash_write(hdev, HASH_STR, *preg++);
+		stm32_hash_write(hdev, HASH_CR, *preg);
+		reg = *preg++ | HASH_CR_INIT;
+		stm32_hash_write(hdev, HASH_CR, reg);
 
-	if (!hdev)
-		return -ENODEV;
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			stm32_hash_write(hdev, HASH_CSR(i), *preg++);
 
-	hdev->req = req;
+		hdev->flags |= HASH_FLAGS_INIT;
 
-	rctx = ahash_request_ctx(req);
+		if (state->flags & HASH_FLAGS_HMAC)
+			hdev->flags |= HASH_FLAGS_HMAC |
+				       HASH_FLAGS_HMAC_KEY;
+	}
 
 	if (rctx->op == HASH_OP_UPDATE)
 		err = stm32_hash_update_req(hdev);
@@ -1041,34 +1045,8 @@ static int stm32_hash_digest(struct ahash_request *req)
 static int stm32_hash_export(struct ahash_request *req, void *out)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_state *state = &rctx->state;
-	u32 *preg;
-	unsigned int i;
-	int ret;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	ret = stm32_hash_wait_busy(hdev);
-	if (ret)
-		return ret;
-
-	state->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					  sizeof(u32), GFP_KERNEL);
-	preg = state->hw_context;
 
-	if (!hdev->pdata->ux500)
-		*preg++ = stm32_hash_read(hdev, HASH_IMR);
-	*preg++ = stm32_hash_read(hdev, HASH_STR);
-	*preg++ = stm32_hash_read(hdev, HASH_CR);
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
-
-	memcpy(out, rctx, sizeof(*rctx));
+	memcpy(out, &rctx->state, sizeof(rctx->state));
 
 	return 0;
 }
@@ -1076,33 +1054,9 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 static int stm32_hash_import(struct ahash_request *req, const void *in)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_state *state = &rctx->state;
-	const u32 *preg = in;
-	u32 reg;
-	unsigned int i;
-
-	memcpy(rctx, in, sizeof(*rctx));
-
-	preg = state->hw_context;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	if (!hdev->pdata->ux500)
-		stm32_hash_write(hdev, HASH_IMR, *preg++);
-	stm32_hash_write(hdev, HASH_STR, *preg++);
-	stm32_hash_write(hdev, HASH_CR, *preg);
-	reg = *preg++ | HASH_CR_INIT;
-	stm32_hash_write(hdev, HASH_CR, reg);
-
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		stm32_hash_write(hdev, HASH_CSR(i), *preg++);
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
 
-	kfree(state->hw_context);
+	stm32_hash_init(req);
+	memcpy(&rctx->state, in, sizeof(rctx->state));
 
 	return 0;
 }
@@ -1159,8 +1113,6 @@ static int stm32_hash_cra_init_algs(struct crypto_tfm *tfm,
 		ctx->flags |= HASH_FLAGS_HMAC;
 
 	ctx->enginectx.op.do_one_request = stm32_hash_one_request;
-	ctx->enginectx.op.prepare_request = stm32_hash_prepare_req;
-	ctx->enginectx.op.unprepare_request = NULL;
 
 	return stm32_hash_init_fallback(tfm);
 }
@@ -1252,7 +1204,7 @@ static struct ahash_alg algs_md5[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "md5",
 				.cra_driver_name = "stm32-md5",
@@ -1279,7 +1231,7 @@ static struct ahash_alg algs_md5[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(md5)",
 				.cra_driver_name = "stm32-hmac-md5",
@@ -1308,7 +1260,7 @@ static struct ahash_alg algs_sha1[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "sha1",
 				.cra_driver_name = "stm32-sha1",
@@ -1335,7 +1287,7 @@ static struct ahash_alg algs_sha1[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(sha1)",
 				.cra_driver_name = "stm32-hmac-sha1",
@@ -1364,7 +1316,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "sha224",
 				.cra_driver_name = "stm32-sha224",
@@ -1391,7 +1343,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(sha224)",
 				.cra_driver_name = "stm32-hmac-sha224",
@@ -1420,7 +1372,7 @@ static struct ahash_alg algs_sha256[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "sha256",
 				.cra_driver_name = "stm32-sha256",
@@ -1447,7 +1399,7 @@ static struct ahash_alg algs_sha256[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(sha256)",
 				.cra_driver_name = "stm32-hmac-sha256",

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Mon, Mar 6, 2023 at 5:42 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The Crypto API hashing paradigm requires the hardware state to
> be exported between *each* request because multiple unrelated
> hashes may be processed concurrently.
>
> The stm32 hardware is capable of producing the hardware hashing
> state but it was only doing it in the export function.  This is
> not only broken for export as you can't export a kernel pointer
> and reimport it, but it also means that concurrent hashing was
> fundamentally broken.
>
> Fix this by moving the saving and restoring of hardware hash
> state between each and every hashing request.
>
> Also change the emptymsg check in stm32_hash_copy_hash to rely
> on whether we have any existing hash state, rather than whether
> this particular update request is empty.
>
> Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
> Reported-by: Li kunyu <kunyu@nfschina.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This partly works (after my folded in fix in patch 5)!

Clean SHA1 and SHA256 works flawlessly.
HMAC still fails, but not until we start testing random vectors:

[    7.541954] alg: ahash: stm32-hmac-sha256 digest() failed on test
vector "random: psize=0 ksize=80"; expected_error=0,
actual_error=-110, cfg="random: may_sleep"
[    7.567212] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-110)
[    7.567222] ------------[ cut here ]------------
[    7.579669] WARNING: CPU: 0 PID: 89 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[    7.587809] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-110)
[    7.587817] Modules linked in:
[    7.598702] CPU: 0 PID: 89 Comm: cryptomgr_test Not tainted
6.2.0-13572-gcdc48b2701b2 #87
[    7.606877] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[    7.613842]  unwind_backtrace from show_stack+0x10/0x14
[    7.619080]  show_stack from dump_stack_lvl+0x40/0x4c
[    7.624145]  dump_stack_lvl from __warn+0x94/0xc0
[    7.628861]  __warn from warn_slowpath_fmt+0x118/0x164
[    7.634007]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[    7.639936]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[    7.645430]  cryptomgr_test from kthread+0xc0/0xc4
[    7.650229]  kthread from ret_from_fork+0x14/0x2c
[    7.654936] Exception stack(0xf10b5fb0 to 0xf10b5ff8)
[    7.659984] 5fa0:                                     00000000
00000000 00000000 00000000
[    7.668154] 5fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    7.676325] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    7.682986] ---[ end trace 0000000000000000 ]---
[    7.688219] stm32-hash a03c2000.hash: allocated sha256 fallback
[   10.675002] stm32-hash a03c2000.hash: allocated hmac(sha1) fallback
[   11.269604] alg: ahash: stm32-hmac-sha1 finup() failed with err
-110 on test vector "random: psize=0 ksize=15", cfg="random: use_finup
src_divs=[100.0%@+4081] i"
[   11.285037] alg: self-tests for hmac(sha1) using stm32-hmac-sha1
failed (rc=-110)
[   11.285048] ------------[ cut here ]------------
[   11.297141] WARNING: CPU: 1 PID: 102 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[   11.305352] alg: self-tests for hmac(sha1) using stm32-hmac-sha1
failed (rc=-110)
[   11.305361] Modules linked in:
[   11.315894] CPU: 1 PID: 102 Comm: cryptomgr_test Tainted: G
W          6.2.0-13572-gcdc48b2701b2 #87
[   11.325633] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[   11.332594]  unwind_backtrace from show_stack+0x10/0x14
[   11.337832]  show_stack from dump_stack_lvl+0x40/0x4c
[   11.342897]  dump_stack_lvl from __warn+0x94/0xc0
[   11.347611]  __warn from warn_slowpath_fmt+0x118/0x164
[   11.352758]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[   11.358687]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[   11.364181]  cryptomgr_test from kthread+0xc0/0xc4
[   11.368981]  kthread from ret_from_fork+0x14/0x2c
[   11.373687] Exception stack(0xf10f1fb0 to 0xf10f1ff8)
[   11.378734] 1fa0:                                     00000000
00000000 00000000 00000000
[   11.386906] 1fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   11.395076] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[   11.401724] ---[ end trace 0000000000000000 ]---
[   11.407162] stm32-hash a03c2000.hash: allocated sha1 fallback

I will try to investigate further as time permits. (Right now I have
to go off for lunch...)

Yours,
Linus Walleij

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Mon, Mar 06, 2023 at 11:08:13AM +0100, Linus Walleij wrote:
>
> This partly works (after my folded in fix in patch 5)!
> 
> Clean SHA1 and SHA256 works flawlessly.
> HMAC still fails, but not until we start testing random vectors:
> 
> [    7.541954] alg: ahash: stm32-hmac-sha256 digest() failed on test
> vector "random: psize=0 ksize=80"; expected_error=0,
> actual_error=-110, cfg="random: may_sleep"
> [    7.567212] alg: self-tests for hmac(sha256) using
> stm32-hmac-sha256 failed (rc=-110)

So it's timing out.  I wonder if the timeout in stm32_hash_wait_busy
is long enough.  Perhaps try adding a zero so that the timeout becomes
100,000us and see if it still breaks?

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

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Tue, Mar 7, 2023 at 11:10 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Mon, Mar 06, 2023 at 11:08:13AM +0100, Linus Walleij wrote:
> >
> > This partly works (after my folded in fix in patch 5)!
> >
> > Clean SHA1 and SHA256 works flawlessly.
> > HMAC still fails, but not until we start testing random vectors:
> >
> > [    7.541954] alg: ahash: stm32-hmac-sha256 digest() failed on test
> > vector "random: psize=0 ksize=80"; expected_error=0,
> > actual_error=-110, cfg="random: may_sleep"
> > [    7.567212] alg: self-tests for hmac(sha256) using
> > stm32-hmac-sha256 failed (rc=-110)
>
> So it's timing out.  I wonder if the timeout in stm32_hash_wait_busy
> is long enough.  Perhaps try adding a zero so that the timeout becomes
> 100,000us and see if it still breaks?

Sadly this doesn't work.

I tried increasing with one and even two orders of magnitude,
but the timeouts still happen, usually two of them, sometimes
one sometimes three, depending on randomness, as can be
expected.

I think you mentioned something about that we need to store
the key in the state as well though?

Yours,
Linus Walleij

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Tue, Mar 07, 2023 at 04:31:44PM +0100, Linus Walleij wrote:
>
> Sadly this doesn't work.

Nevermind, I know why it doesn't work.  It's specific to ux500
where you're polling the DCAL bit.  But the DCAL bit only works
for the final hash, it doesn't work for the intermediate state.

Let me check how the old ux500 handled this case.

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

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Wed, Mar 08, 2023 at 11:23:24AM +0800, Herbert Xu wrote:
>
> Nevermind, I know why it doesn't work.  It's specific to ux500
> where you're polling the DCAL bit.  But the DCAL bit only works
> for the final hash, it doesn't work for the intermediate state.
> 
> Let me check how the old ux500 handled this case.

Hmm, it seems to use the same bit.  I guess the meaning must be
different with ux500.

Could you check for me which wait_busy() call is actually failing?
Is it the one I added right before we save the state, or is it
something else?

If it's something perhaps we aren't restoring the state in the
right way, because the stm32 state restoring code is quite different
compared to the ux500 code.

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

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Wed, Mar 08, 2023 at 11:40:02AM +0800, Herbert Xu wrote:
>
> If it's something perhaps we aren't restoring the state in the
> right way, because the stm32 state restoring code is quite different
> compared to the ux500 code.

Could you also confirm that the old ux500 driver actually passes
all the extra tests on your hardware? It literally saves and
restores the state every 256 bytes :)

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

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Wed, Mar 8, 2023 at 4:40 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Wed, Mar 08, 2023 at 11:23:24AM +0800, Herbert Xu wrote:
> >
> > Nevermind, I know why it doesn't work.  It's specific to ux500
> > where you're polling the DCAL bit.  But the DCAL bit only works
> > for the final hash, it doesn't work for the intermediate state.
> >
> > Let me check how the old ux500 handled this case.
>
> Hmm, it seems to use the same bit.  I guess the meaning must be
> different with ux500.

Not sure if it's that or if we were just lucky, or the tests were not
as extensive back on the old driver :/

> Could you check for me which wait_busy() call is actually failing?
> Is it the one I added right before we save the state, or is it
> something else?

It times out - always - before writing the HMAC key in
stm32_hash_xmit_cpu().

So this:

                stm32_hash_set_nblw(hdev, length);
                reg = stm32_hash_read(hdev, HASH_STR);
                reg |= HASH_STR_DCAL;
                stm32_hash_write(hdev, HASH_STR, reg);
                if (hdev->flags & HASH_FLAGS_HMAC) {
-                       if (stm32_hash_wait_busy(hdev))
+                       if (stm32_hash_wait_busy(hdev)) {
+                               dev_err(hdev->dev,
+                                       "timeout before writing key in
stm32_hash_xmit_cpu()\n");
                                return -ETIMEDOUT;
+                       }
                        stm32_hash_write_key(hdev);
                }
                return -EINPROGRESS;

Gives:

[    4.812106] stm32-hash a03c2000.hash: allocated hmac(sha256) fallback
[    5.008829] stm32-hash a03c2000.hash: timeout before writing key in
stm32_hash_xmit_cpu()
[    5.017167] alg: ahash: stm32-hmac-sha256 final() failed with err
-110 on test vector "random: psize=0 ksize=70", cfg="random: may_sleep
use_final src_divs=[<fl"
[    5.041034] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-110)

I put error messages at all other timeouts too but they do not
trigger.

This is why non-HMAC works fine all of the time.

> If it's something perhaps we aren't restoring the state in the
> right way, because the stm32 state restoring code is quite different
> compared to the ux500 code.

It's just the HMAC that is failing so I'm puzzled, because until
this point it is essentially a clean SHA sum.

I also noticed that it only happens with long keys (more than
64 bytes) that need to set bit 16 (HASH_CR_LKEY) in the control
register.

> Could you also confirm that the old ux500 driver actually passes
> all the extra tests on your hardware? It literally saves and
> restores the state every 256 bytes :)

I had this old patch set where I tried to clean up the old driver:
https://lore.kernel.org/linux-crypto/20220816140049.102306-1-linus.walleij@linaro.org/
I put this old patch set on top of v6.0.

First I enabled the old crypto driver without extended tests: all works fine.

Then I enabled the extended tests. It does seem like it has
the same problem!

[   25.486878] CPU: 1 PID: 91 Comm: cryptomgr_test Not tainted
6.0.0-00016-g23791565aac5 #3
[   25.494967] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[   25.501932]  unwind_backtrace from show_stack+0x10/0x14
[   25.507175]  show_stack from dump_stack_lvl+0x40/0x4c
[   25.512236]  dump_stack_lvl from nmi_cpu_backtrace+0xd4/0x104
[   25.517988]  nmi_cpu_backtrace from nmi_trigger_cpumask_backtrace+0xdc/0x128
[   25.525034]  nmi_trigger_cpumask_backtrace from
trigger_single_cpu_backtrace+0x24/0x2c
[   25.532952]  trigger_single_cpu_backtrace from
rcu_dump_cpu_stacks+0x10c/0x150
[   25.540173]  rcu_dump_cpu_stacks from print_cpu_stall+0x14c/0x1d0
[   25.546268]  print_cpu_stall from check_cpu_stall+0x1cc/0x26c
[   25.552013]  check_cpu_stall from rcu_sched_clock_irq+0x74/0x16c
[   25.558017]  rcu_sched_clock_irq from update_process_times+0x68/0x94
[   25.564377]  update_process_times from tick_sched_timer+0x60/0xec
[   25.570470]  tick_sched_timer from __hrtimer_run_queues+0x15c/0x218
[   25.576737]  __hrtimer_run_queues from hrtimer_interrupt+0x124/0x2b0
[   25.583090]  hrtimer_interrupt from twd_handler+0x34/0x3c
[   25.588489]  twd_handler from handle_percpu_devid_irq+0x78/0x134
[   25.594498]  handle_percpu_devid_irq from generic_handle_domain_irq+0x24/0x34
[   25.601640]  generic_handle_domain_irq from gic_handle_irq+0x74/0x88
[   25.608000]  gic_handle_irq from generic_handle_arch_irq+0x34/0x44
[   25.614187]  generic_handle_arch_irq from call_with_stack+0x18/0x20
[   25.620462]  call_with_stack from __irq_svc+0x98/0xb0
[   25.625514] Exception stack(0xf10ad970 to 0xf10ad9b8)
[   25.630563] d960:                                     c1d59a00
40000013 c056bbfc 00005f32
[   25.638734] d980: 00000000 00000008 f10ad9e0 00000020 c1f69bc0
c1f69bc0 c2364140 f10adb34
[   25.646904] d9a0: 00000000 f10ad9c0 c056da50 c099d1ec 20000013 ffffffff
[   25.653512]  __irq_svc from _raw_spin_unlock_irqrestore+0x1c/0x20
[   25.659605]  _raw_spin_unlock_irqrestore from regmap_read+0x50/0x60
[   25.665882]  regmap_read from hash_hw_write_key+0xd8/0x100
[   25.671377]  hash_hw_write_key from init_hash_hw+0xd8/0xfc
[   25.676862]  init_hash_hw from hash_hw_final+0x88/0x36c
[   25.682089]  hash_hw_final from ahash_final+0x58/0x9c
[   25.687138]  ahash_final from do_ahash_op+0x20/0x98
[   25.692019]  do_ahash_op from test_ahash_vec_cfg+0x68c/0x984
[   25.697677]  test_ahash_vec_cfg from test_hash_vec+0x64/0x168
[   25.703421]  test_hash_vec from __alg_test_hash+0x158/0x2d8
[   25.708991]  __alg_test_hash from alg_test_hash+0xc0/0x170
[   25.714474]  alg_test_hash from alg_test.part.0+0x378/0x4b8
[   25.720044]  alg_test.part.0 from cryptomgr_test+0x24/0x44
[   25.725537]  cryptomgr_test from kthread+0xc0/0xc4
[   25.730334]  kthread from ret_from_fork+0x14/0x2c
[   25.735036] Exception stack(0xf10adfb0 to 0xf10adff8)
[   25.740081] dfa0:                                     00000000
00000000 00000000 00000000
[   25.748252] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   25.756422] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000

I can't see 100% if it is the same issue but it does hang on something
related to writing keys.

So for Ux500 at least I suppose it would be best to inhibit .import and
.export when using HMAC with long keys unless I can figure out exactly
what the issue is here. I wonder if that is possible?
Or do I have to remove it from the HMAC algos altogether?

Yours,
Linus Walleij

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Wed, Mar 08, 2023 at 10:05:14AM +0100, Linus Walleij wrote:
>
> So for Ux500 at least I suppose it would be best to inhibit .import and
> .export when using HMAC with long keys unless I can figure out exactly
> what the issue is here. I wonder if that is possible?
> Or do I have to remove it from the HMAC algos altogether?

If the hardware is buggered it's not a big deal if it's just HMAC.
Because any HMAC hash can easily be broken down into three underlying
hash operations.

But let me digest your new information first, and see if we can
figure out a way to get it to work.  If not then we could just disable
hmac (unless we can get confirmation from stm32 hardware we should
just disable it for everything in stm32) and I'll fix the generic
hmac to actually work with hardware drivers.

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

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Wed, Mar 08, 2023 at 10:05:14AM +0100, Linus Walleij wrote:
>
> [    4.812106] stm32-hash a03c2000.hash: allocated hmac(sha256) fallback
> [    5.008829] stm32-hash a03c2000.hash: timeout before writing key in
> stm32_hash_xmit_cpu()
> [    5.017167] alg: ahash: stm32-hmac-sha256 final() failed with err
> -110 on test vector "random: psize=0 ksize=70", cfg="random: may_sleep
> use_final src_divs=[<fl"

Wait a second, this is an empty message.  Can you reproduce the
hang if you exclude all psize=0 test vectors?

If it's just empty messages, which we know are broken with ux500
to begin with, then we can simply not do the hash at all (doing
it and then throwing it away seems pointless).

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

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Wed, Mar 08, 2023 at 06:10:14PM +0800, Herbert Xu wrote:
>
> If it's just empty messages, which we know are broken with ux500
> to begin with, then we can simply not do the hash at all (doing
> it and then throwing it away seems pointless).

Here is a patch to not process empty messages at all.

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
--
diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index ff6e4f1e47ed..3f436fa0e5c1 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -374,9 +374,15 @@ static int stm32_hash_xmit_cpu(struct stm32_hash_dev *hdev,
 	const u32 *buffer = (const u32 *)buf;
 	u32 reg;
 
-	if (final)
+	if (final) {
 		hdev->flags |= HASH_FLAGS_FINAL;
 
+		/* Do not process empty messages if hw is buggy. */
+		if (!(hdev->flags & HASH_FLAGS_INIT) && !length &&
+		    hdev->pdata->broken_emptymsg)
+			return 0;
+	}
+
 	len32 = DIV_ROUND_UP(length, sizeof(u32));
 
 	dev_dbg(hdev->dev, "%s: length: %zd, final: %x len32 %i\n",

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Wed, Mar 8, 2023 at 11:19 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Wed, Mar 08, 2023 at 06:10:14PM +0800, Herbert Xu wrote:
> >
> > If it's just empty messages, which we know are broken with ux500
> > to begin with, then we can simply not do the hash at all (doing
> > it and then throwing it away seems pointless).
>
> Here is a patch to not process empty messages at all.

Hey it works :D

I had to tweak it slightly because we need to set the state right:

@@ -374,9 +387,20 @@ static int stm32_hash_xmit_cpu(struct stm32_hash_dev *hdev,
        const u32 *buffer = (const u32 *)buf;
        u32 reg;

-       if (final)
+       if (final) {
                hdev->flags |= HASH_FLAGS_FINAL;

+               /* Do not process empty messages if hw is buggy. */
+               if (!(hdev->flags & HASH_FLAGS_INIT) && !length &&
+                   hdev->pdata->broken_emptymsg) {
+                       struct stm32_hash_request_ctx *rctx =
ahash_request_ctx(hdev->req);
+                       struct stm32_hash_state *state = &rctx->state;
+
+                       state->flags |= HASH_FLAGS_EMPTY;
+                       return 0;
+               }
+       }
+
        len32 = DIV_ROUND_UP(length, sizeof(u32));

        dev_dbg(hdev->dev, "%s: length: %zd, final: %x len32 %i\n",


After this it WORKS!

For the "v5.5" patch and this (and my other patch) folded in:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

So now the driver is fixed from a Ux500 point of view.

I actually have the STM32MP board, I can try to find some
time to test the final patch set on it as well if it has the same
has as the other STM32 SoCs.

Yours,
Linus Walleij

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Wed, Mar 08, 2023 at 10:19:48PM +0100, Linus Walleij wrote:
> 
> So now the driver is fixed from a Ux500 point of view.

I think there is actually a nasty bug in it that may be hard to
trigger.

The stm32 driver as it stands will write up to 256 bytes into
the FIFO which on the ux500 is limited to 64 bytes.  We need to
change the fixed 256-byte size to be dependent on the hardware
type.

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: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Thu, Mar 9, 2023 at 6:58 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Wed, Mar 08, 2023 at 10:19:48PM +0100, Linus Walleij wrote:
> >
> > So now the driver is fixed from a Ux500 point of view.
>
> I think there is actually a nasty bug in it that may be hard to
> trigger.
>
> The stm32 driver as it stands will write up to 256 bytes into
> the FIFO which on the ux500 is limited to 64 bytes.  We need to
> change the fixed 256-byte size to be dependent on the hardware
> type.

Right so that is done implicitly by using a buffer of 256 bytes.

But actually I think the bug will never trigger, because the datasheet
for the DB8500 (Ux500) says this:

"Then the message can be sent, by writing it word per word into the
HASH_DIN register.
When a block of 512 bits, i.e. 16 words have been written, a partial
digest computation will
start upon writing the first data of the next block. The AHB bus will
be busy for 82 cycles for
SHA-1 algorithm (66 cycles for SHA-256 algorithm)."

The way I interpret it is that if you write 64 bytes (16 32bit words)
the AHB bus will simply
stall until the data is processed, so the writel() hangs there and
then 66/82 bus cycles
later it will continue.

This isn't the prettiest from a system PoV, as it can stall interrupt
handling and
cause latency jitter, but it's not actually a bug. It's kind of
similar to that user
experience "bug" on x86 PCs where the sound starts breaking up if you have too
intense graphics going on, because the bus is too busy so the sound FIFO goes
empty.

But I can certainly make a patch to shrink the buffer from 256 to 64 bytes on
Ux500 if it's the right thing to do.

Yours,
Linus Walleij

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Thu, Mar 09, 2023 at 08:35:21AM +0100, Linus Walleij wrote:
>
> The way I interpret it is that if you write 64 bytes (16 32bit words)
> the AHB bus will simply
> stall until the data is processed, so the writel() hangs there and
> then 66/82 bus cycles
> later it will continue.

You're right.  I'll respin the patches with your updates in it.

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

RE: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[David Laight]

From: Linus Walleij
> Sent: 09 March 2023 07:35
...
> But actually I think the bug will never trigger, because the datasheet
> for the DB8500 (Ux500) says this:
> 
> "Then the message can be sent, by writing it word per word into the
> HASH_DIN register.
> When a block of 512 bits, i.e. 16 words have been written, a partial
> digest computation will
> start upon writing the first data of the next block. The AHB bus will
> be busy for 82 cycles for
> SHA-1 algorithm (66 cycles for SHA-256 algorithm)."

What speed clock is that?

4 or 5 extra clocks/word may (or may not) be significant.

In terms of latency it may be noise compared to some PCIe
reads done by hardware interrupt handlers.
Some slow PCIe targets (like the fpga one we use) pretty
much take 1us to handle a read cycle.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [v5 PATCH 7/7] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Thu, Mar 9, 2023 at 11:19 PM David Laight <David.Laight@aculab.com> wrote:

> > But actually I think the bug will never trigger, because the datasheet
> > for the DB8500 (Ux500) says this:
> >
> > "Then the message can be sent, by writing it word per word into the
> > HASH_DIN register.
> > When a block of 512 bits, i.e. 16 words have been written, a partial
> > digest computation will
> > start upon writing the first data of the next block. The AHB bus will
> > be busy for 82 cycles for
> > SHA-1 algorithm (66 cycles for SHA-256 algorithm)."
>
> What speed clock is that?

133 MHz.

> 4 or 5 extra clocks/word may (or may not) be significant.
>
> In terms of latency it may be noise compared to some PCIe
> reads done by hardware interrupt handlers.
> Some slow PCIe targets (like the fpga one we use) pretty
> much take 1us to handle a read cycle.

So in this case it's 1/133M s = 8ns cycle time, 82 in worst case,
so 82*8 = 656 ns < 1 us.

Yours,
Linus Walleij

RE: [v6 PATCH 0/7] crypto: stm32 - Save and restore between each request

[lionel.debieve]

Hi All,

Sorry for the very (very very) late response.
Thanks for highlighting the issue. I'm worried about the issue seen that
we've fixed at our downstream level.
We (ST) are currently working on upstreaming the new peripheral update for
STM32MP13 that fixed the old issue seen (such as CSR register numbers), and
so on....

The issue about the context management relies on a question I've get time to
ask you. There is no internal test purpose (using test manager) that really
show the need of a hash update that needs to be "self-content". We've seen
the issue using openssl use cases that is not using import/export.
I'm wondering to understand the real need of import/export in the framework
if the request must be safe itself?

From hardware point of view, it is a penalty to wait for completion to save
the context after each request. I understand the need of multiple hash
request in // but I was wondering that it can be managed by the
import/export, but it seems I was wrong. The penalty of the context saving
will impact all hash requests where, in a runtime context is probably not
the most important use case.
I'm looking deeper to check with the DMA use case and there is some new HW
restriction on the coming hash version that doesn't allow the read of CSR
register at some times.

BR,
Lionel


ST Restricted

-----Original Message-----
From: Herbert Xu <herbert@gondor.apana.org.au> 
Sent: Monday, March 6, 2023 5:42 AM
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Lionel Debieve <lionel.debieve@foss.st.com>; Li kunyu
<kunyu@nfschina.com>; davem@davemloft.net;
linux-arm-kernel@lists.infradead.org; linux-crypto@vger.kernel.org;
linux-kernel@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
mcoquelin.stm32@gmail.com
Subject: [v6 PATCH 0/7] crypto: stm32 - Save and restore between each
request

On Sat, Mar 04, 2023 at 05:34:04PM +0800, Herbert Xu wrote:
> 
> I've split the patch up into smaller chunks for easier testing.

v6 fixes a bug in the finup patch that caused the new data to be discarded
instead of hashed.

This patch series fixes the import/export functions in the stm32 driver.  As
usual, a failure in import/export indicates a general bug in the hash driver
that may break as soon as two concurrent users show up and hash at the same
time using any method other than digest or init+finup.

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: [v6 PATCH 0/7] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Tue, Mar 07, 2023 at 02:55:29PM +0100, lionel.debieve@foss.st.com wrote:
>
> The issue about the context management relies on a question I've get time to
> ask you. There is no internal test purpose (using test manager) that really
> show the need of a hash update that needs to be "self-content". We've seen

Indeed this functionality is sorely missed.  It shouldn't be hard
to implement, because simply hashing two different requests
interleaved with each other should show the problem:

init(A) => update(A) => init(B) => update(B) => final(A) => final(B)

> the issue using openssl use cases that is not using import/export.
> I'm wondering to understand the real need of import/export in the framework
> if the request must be safe itself?

The hash state is normally stored in the request context.  The
import/export functions let you save a copy of the state for
subsequent processing.  The request could then be freed after
the export and re-allocated prior to import, or in other contexts
the request could be reused for a completely different hash in
the time being (init/update/final).

> >From hardware point of view, it is a penalty to wait for completion to save
> the context after each request. I understand the need of multiple hash
> request in // but I was wondering that it can be managed by the
> import/export, but it seems I was wrong. The penalty of the context saving
> will impact all hash requests where, in a runtime context is probably not
> the most important use case.

Oh of course we try to avoid unnecessary savings/restoring as much
as we can.  That's why we encourage users to use finup/digest as
much as possible, in which case there may be be no need to save and
restore at all.

However, if the user has to do a partial update through the update
function, then we have to save the state.

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

[v7 PATCH 0/8] crypto: stm32 - Save and restore between each request

[Herbert Xu]

v7 fixes empty message hashing and moves that into its own patch.
As a result of this arrangement, I have removed the Reviewd-by's
and Tested-by's for those two final patches.

This patch series fixes the import/export functions in the stm32
driver.  As usual, a failure in import/export indicates a general
bug in the hash driver that may break as soon as two concurrent
users show up and hash at the same time using any method other
than digest or init+finup.

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

[v7 PATCH 1/8] crypto: stm32 - Save 54 CSR registers

[Herbert Xu]

The CSR registers go from 0 to 53.  So the number of registers
should be 54.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 7bf805563ac2..bde2b40a6a32 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -68,7 +68,7 @@
 #define HASH_MASK_DATA_INPUT		BIT(1)
 
 /* Context swap register */
-#define HASH_CSR_REGISTER_NUMBER	53
+#define HASH_CSR_REGISTER_NUMBER	54
 
 /* Status Flags */
 #define HASH_SR_DATA_INPUT_READY	BIT(0)

[v7 PATCH 2/8] crypto: stm32 - Move polling into do_one_request

[Herbert Xu]

There is no need to poll separate for update and final.  We could
merge them into do_one_request.

Also fix the error handling so that we don't poll (and overwrite
the error) when an error has already occurred.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |   29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index bde2b40a6a32..298cabd29e36 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -425,6 +425,8 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 		bufcnt = rctx->bufcnt;
 		rctx->bufcnt = 0;
 		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 0);
+		if (err)
+			return err;
 	}
 
 	stm32_hash_append_sg(rctx);
@@ -433,14 +435,6 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 		bufcnt = rctx->bufcnt;
 		rctx->bufcnt = 0;
 		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 1);
-
-		/* If we have an IRQ, wait for that, else poll for completion */
-		if (hdev->polled) {
-			if (stm32_hash_wait_busy(hdev))
-				return -ETIMEDOUT;
-			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
-			err = 0;
-		}
 	}
 
 	return err;
@@ -784,15 +778,6 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 	else
 		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
 
-	/* If we have an IRQ, wait for that, else poll for completion */
-	if (hdev->polled) {
-		if (stm32_hash_wait_busy(hdev))
-			return -ETIMEDOUT;
-		hdev->flags |= HASH_FLAGS_OUTPUT_READY;
-		/* Caller will call stm32_hash_finish_req() */
-		err = 0;
-	}
-
 	return err;
 }
 
@@ -964,6 +949,16 @@ static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 	else if (rctx->op == HASH_OP_FINAL)
 		err = stm32_hash_final_req(hdev);
 
+	/* If we have an IRQ, wait for that, else poll for completion */
+	if (err == -EINPROGRESS && hdev->polled) {
+		if (stm32_hash_wait_busy(hdev))
+			err = -ETIMEDOUT;
+		else {
+			hdev->flags |= HASH_FLAGS_OUTPUT_READY;
+			err = 0;
+		}
+	}
+
 	if (err != -EINPROGRESS)
 	/* done task will not finish it, so do it here */
 		stm32_hash_finish_req(req, err);

[v7 PATCH 3/8] crypto: stm32 - Simplify finup

[Herbert Xu]

The current finup code is unnecessarily convoluted.  There is no
need to call update and final separately as update already does
all the necessary work on its own.

Simplify this by utilising the HASH_FLAGS_FINUP bit in rctx to
indicate only finup and use the HASH_FLAGS_FINAL bit instead to
signify processing common to both final and finup.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |   41 +++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 298cabd29e36..e16f9aaec6bf 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -417,7 +417,7 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 
 	dev_dbg(hdev->dev, "%s flags %lx\n", __func__, rctx->flags);
 
-	final = (rctx->flags & HASH_FLAGS_FINUP);
+	final = rctx->flags & HASH_FLAGS_FINAL;
 
 	while ((rctx->total >= rctx->buflen) ||
 	       (rctx->bufcnt + rctx->total >= rctx->buflen)) {
@@ -761,6 +761,11 @@ static int stm32_hash_init(struct ahash_request *req)
 
 static int stm32_hash_update_req(struct stm32_hash_dev *hdev)
 {
+	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+
+	if (!(rctx->flags & HASH_FLAGS_CPU))
+		return stm32_hash_dma_send(hdev);
+
 	return stm32_hash_update_cpu(hdev);
 }
 
@@ -768,17 +773,14 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 {
 	struct ahash_request *req = hdev->req;
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	int err;
 	int buflen = rctx->bufcnt;
 
-	rctx->bufcnt = 0;
+	if (rctx->flags & HASH_FLAGS_FINUP)
+		return stm32_hash_update_req(hdev);
 
-	if (!(rctx->flags & HASH_FLAGS_CPU))
-		err = stm32_hash_dma_send(hdev);
-	else
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
+	rctx->bufcnt = 0;
 
-	return err;
+	return stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
 }
 
 static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
@@ -1000,7 +1002,7 @@ static int stm32_hash_final(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 
-	rctx->flags |= HASH_FLAGS_FINUP;
+	rctx->flags |= HASH_FLAGS_FINAL;
 
 	return stm32_hash_enqueue(req, HASH_OP_FINAL);
 }
@@ -1010,25 +1012,20 @@ static int stm32_hash_finup(struct ahash_request *req)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	int err1, err2;
+
+	if (!req->nbytes)
+		goto out;
 
 	rctx->flags |= HASH_FLAGS_FINUP;
+	rctx->total = req->nbytes;
+	rctx->sg = req->src;
+	rctx->offset = 0;
 
 	if (hdev->dma_lch && stm32_hash_dma_aligned_data(req))
 		rctx->flags &= ~HASH_FLAGS_CPU;
 
-	err1 = stm32_hash_update(req);
-
-	if (err1 == -EINPROGRESS || err1 == -EBUSY)
-		return err1;
-
-	/*
-	 * final() has to be always called to cleanup resources
-	 * even if update() failed, except EINPROGRESS
-	 */
-	err2 = stm32_hash_final(req);
-
-	return err1 ?: err2;
+out:
+	return stm32_hash_final(req);
 }
 
 static int stm32_hash_digest(struct ahash_request *req)

[v7 PATCH 4/8] crypto: stm32 - Remove unused hdev->err field

[Herbert Xu]

The variable hdev->err is never read so it can be removed.

Also remove a spurious inclusion of linux/crypto.h.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index e16f9aaec6bf..e35fee945371 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -7,7 +7,6 @@
  */
 
 #include <linux/clk.h>
-#include <linux/crypto.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
@@ -183,7 +182,6 @@ struct stm32_hash_dev {
 	struct ahash_request	*req;
 	struct crypto_engine	*engine;
 
-	int			err;
 	unsigned long		flags;
 
 	struct dma_chan		*dma_lch;
@@ -894,7 +892,6 @@ static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
 		stm32_hash_write(hdev, HASH_STR, 0);
 		stm32_hash_write(hdev, HASH_DIN, 0);
 		stm32_hash_write(hdev, HASH_IMR, 0);
-		hdev->err = 0;
 	}
 
 	return 0;

[v7 PATCH 5/8] crypto: stm32 - Move hash state into separate structure

[Herbert Xu]

Create a new struct stm32_hash_state so that it may be exported
in future instead of the entire request context.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |  127 +++++++++++++++++++++-----------------
 1 file changed, 71 insertions(+), 56 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index e35fee945371..c836163a9fd4 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -126,15 +126,24 @@ struct stm32_hash_ctx {
 	int			keylen;
 };
 
+struct stm32_hash_state {
+	u32			flags;
+
+	u16			bufcnt;
+	u16			buflen;
+
+	u8 buffer[HASH_BUFLEN] __aligned(4);
+
+	/* hash state */
+	u32			*hw_context;
+};
+
 struct stm32_hash_request_ctx {
 	struct stm32_hash_dev	*hdev;
-	unsigned long		flags;
 	unsigned long		op;
 
 	u8 digest[SHA256_DIGEST_SIZE] __aligned(sizeof(u32));
 	size_t			digcnt;
-	size_t			bufcnt;
-	size_t			buflen;
 
 	/* DMA */
 	struct scatterlist	*sg;
@@ -148,10 +157,7 @@ struct stm32_hash_request_ctx {
 
 	u8			data_type;
 
-	u8 buffer[HASH_BUFLEN] __aligned(sizeof(u32));
-
-	/* Export Context */
-	u32			*hw_context;
+	struct stm32_hash_state state;
 };
 
 struct stm32_hash_algs_info {
@@ -268,11 +274,12 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
 	struct crypto_ahash *tfm = crypto_ahash_reqtfm(hdev->req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(tfm);
+	struct stm32_hash_state *state = &rctx->state;
 
 	u32 reg = HASH_CR_INIT;
 
 	if (!(hdev->flags & HASH_FLAGS_INIT)) {
-		switch (rctx->flags & HASH_FLAGS_ALGO_MASK) {
+		switch (state->flags & HASH_FLAGS_ALGO_MASK) {
 		case HASH_FLAGS_MD5:
 			reg |= HASH_CR_ALGO_MD5;
 			break;
@@ -297,7 +304,7 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 
 		reg |= (rctx->data_type << HASH_CR_DATATYPE_POS);
 
-		if (rctx->flags & HASH_FLAGS_HMAC) {
+		if (state->flags & HASH_FLAGS_HMAC) {
 			hdev->flags |= HASH_FLAGS_HMAC;
 			reg |= HASH_CR_MODE;
 			if (ctx->keylen > HASH_LONG_KEY)
@@ -324,11 +331,12 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 
 static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 {
+	struct stm32_hash_state *state = &rctx->state;
 	size_t count;
 
-	while ((rctx->bufcnt < rctx->buflen) && rctx->total) {
+	while ((state->bufcnt < state->buflen) && rctx->total) {
 		count = min(rctx->sg->length - rctx->offset, rctx->total);
-		count = min(count, rctx->buflen - rctx->bufcnt);
+		count = min_t(size_t, count, state->buflen - state->bufcnt);
 
 		if (count <= 0) {
 			if ((rctx->sg->length == 0) && !sg_is_last(rctx->sg)) {
@@ -339,10 +347,10 @@ static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 			}
 		}
 
-		scatterwalk_map_and_copy(rctx->buffer + rctx->bufcnt, rctx->sg,
-					 rctx->offset, count, 0);
+		scatterwalk_map_and_copy(state->buffer + state->bufcnt,
+					 rctx->sg, rctx->offset, count, 0);
 
-		rctx->bufcnt += count;
+		state->bufcnt += count;
 		rctx->offset += count;
 		rctx->total -= count;
 
@@ -411,18 +419,19 @@ static int stm32_hash_xmit_cpu(struct stm32_hash_dev *hdev,
 static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	struct stm32_hash_state *state = &rctx->state;
 	int bufcnt, err = 0, final;
 
-	dev_dbg(hdev->dev, "%s flags %lx\n", __func__, rctx->flags);
+	dev_dbg(hdev->dev, "%s flags %x\n", __func__, state->flags);
 
-	final = rctx->flags & HASH_FLAGS_FINAL;
+	final = state->flags & HASH_FLAGS_FINAL;
 
-	while ((rctx->total >= rctx->buflen) ||
-	       (rctx->bufcnt + rctx->total >= rctx->buflen)) {
+	while ((rctx->total >= state->buflen) ||
+	       (state->bufcnt + rctx->total >= state->buflen)) {
 		stm32_hash_append_sg(rctx);
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 0);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 0);
 		if (err)
 			return err;
 	}
@@ -430,9 +439,9 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 	stm32_hash_append_sg(rctx);
 
 	if (final) {
-		bufcnt = rctx->bufcnt;
-		rctx->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, bufcnt, 1);
+		bufcnt = state->bufcnt;
+		state->bufcnt = 0;
+		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
 	}
 
 	return err;
@@ -576,10 +585,10 @@ static int stm32_hash_dma_init(struct stm32_hash_dev *hdev)
 static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	u32 *buffer = (void *)rctx->state.buffer;
 	struct scatterlist sg[1], *tsg;
 	int err = 0, len = 0, reg, ncp = 0;
 	unsigned int i;
-	u32 *buffer = (void *)rctx->buffer;
 
 	rctx->sg = hdev->req->src;
 	rctx->total = hdev->req->nbytes;
@@ -607,7 +616,7 @@ static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
 
 				ncp = sg_pcopy_to_buffer(
 					rctx->sg, rctx->nents,
-					rctx->buffer, sg->length - len,
+					rctx->state.buffer, sg->length - len,
 					rctx->total - sg->length + len);
 
 				sg->length = len;
@@ -718,41 +727,40 @@ static int stm32_hash_init(struct ahash_request *req)
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(tfm);
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 
 	rctx->hdev = hdev;
 
-	rctx->flags = HASH_FLAGS_CPU;
+	state->flags = HASH_FLAGS_CPU;
 
 	rctx->digcnt = crypto_ahash_digestsize(tfm);
 	switch (rctx->digcnt) {
 	case MD5_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_MD5;
+		state->flags |= HASH_FLAGS_MD5;
 		break;
 	case SHA1_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_SHA1;
+		state->flags |= HASH_FLAGS_SHA1;
 		break;
 	case SHA224_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_SHA224;
+		state->flags |= HASH_FLAGS_SHA224;
 		break;
 	case SHA256_DIGEST_SIZE:
-		rctx->flags |= HASH_FLAGS_SHA256;
+		state->flags |= HASH_FLAGS_SHA256;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	rctx->bufcnt = 0;
-	rctx->buflen = HASH_BUFLEN;
+	rctx->state.bufcnt = 0;
+	rctx->state.buflen = HASH_BUFLEN;
 	rctx->total = 0;
 	rctx->offset = 0;
 	rctx->data_type = HASH_DATA_8_BITS;
 
-	memset(rctx->buffer, 0, HASH_BUFLEN);
-
 	if (ctx->flags & HASH_FLAGS_HMAC)
-		rctx->flags |= HASH_FLAGS_HMAC;
+		state->flags |= HASH_FLAGS_HMAC;
 
-	dev_dbg(hdev->dev, "%s Flags %lx\n", __func__, rctx->flags);
+	dev_dbg(hdev->dev, "%s Flags %x\n", __func__, state->flags);
 
 	return 0;
 }
@@ -760,8 +768,9 @@ static int stm32_hash_init(struct ahash_request *req)
 static int stm32_hash_update_req(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	struct stm32_hash_state *state = &rctx->state;
 
-	if (!(rctx->flags & HASH_FLAGS_CPU))
+	if (!(state->flags & HASH_FLAGS_CPU))
 		return stm32_hash_dma_send(hdev);
 
 	return stm32_hash_update_cpu(hdev);
@@ -771,14 +780,15 @@ static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 {
 	struct ahash_request *req = hdev->req;
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	int buflen = rctx->bufcnt;
+	struct stm32_hash_state *state = &rctx->state;
+	int buflen = state->bufcnt;
 
-	if (rctx->flags & HASH_FLAGS_FINUP)
+	if (state->flags & HASH_FLAGS_FINUP)
 		return stm32_hash_update_req(hdev);
 
-	rctx->bufcnt = 0;
+	state->bufcnt = 0;
 
-	return stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
+	return stm32_hash_xmit_cpu(hdev, state->buffer, buflen, 1);
 }
 
 static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
@@ -813,6 +823,7 @@ static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
 static void stm32_hash_copy_hash(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 	struct stm32_hash_dev *hdev = rctx->hdev;
 	__be32 *hash = (void *)rctx->digest;
 	unsigned int i, hashsize;
@@ -820,7 +831,7 @@ static void stm32_hash_copy_hash(struct ahash_request *req)
 	if (hdev->pdata->broken_emptymsg && !req->nbytes)
 		return stm32_hash_emptymsg_fallback(req);
 
-	switch (rctx->flags & HASH_FLAGS_ALGO_MASK) {
+	switch (state->flags & HASH_FLAGS_ALGO_MASK) {
 	case HASH_FLAGS_MD5:
 		hashsize = MD5_DIGEST_SIZE;
 		break;
@@ -862,6 +873,7 @@ static int stm32_hash_finish(struct ahash_request *req)
 static void stm32_hash_finish_req(struct ahash_request *req, int err)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 	struct stm32_hash_dev *hdev = rctx->hdev;
 
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
@@ -873,7 +885,7 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
 				 HASH_FLAGS_HMAC_KEY);
 	} else {
-		rctx->flags |= HASH_FLAGS_ERRORS;
+		state->flags |= HASH_FLAGS_ERRORS;
 	}
 
 	pm_runtime_mark_last_busy(hdev->dev);
@@ -979,15 +991,16 @@ static int stm32_hash_enqueue(struct ahash_request *req, unsigned int op)
 static int stm32_hash_update(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 
-	if (!req->nbytes || !(rctx->flags & HASH_FLAGS_CPU))
+	if (!req->nbytes || !(state->flags & HASH_FLAGS_CPU))
 		return 0;
 
 	rctx->total = req->nbytes;
 	rctx->sg = req->src;
 	rctx->offset = 0;
 
-	if ((rctx->bufcnt + rctx->total < rctx->buflen)) {
+	if ((state->bufcnt + rctx->total < state->buflen)) {
 		stm32_hash_append_sg(rctx);
 		return 0;
 	}
@@ -998,8 +1011,9 @@ static int stm32_hash_update(struct ahash_request *req)
 static int stm32_hash_final(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
+	struct stm32_hash_state *state = &rctx->state;
 
-	rctx->flags |= HASH_FLAGS_FINAL;
+	state->flags |= HASH_FLAGS_FINAL;
 
 	return stm32_hash_enqueue(req, HASH_OP_FINAL);
 }
@@ -1009,17 +1023,18 @@ static int stm32_hash_finup(struct ahash_request *req)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 
 	if (!req->nbytes)
 		goto out;
 
-	rctx->flags |= HASH_FLAGS_FINUP;
+	state->flags |= HASH_FLAGS_FINUP;
 	rctx->total = req->nbytes;
 	rctx->sg = req->src;
 	rctx->offset = 0;
 
 	if (hdev->dma_lch && stm32_hash_dma_aligned_data(req))
-		rctx->flags &= ~HASH_FLAGS_CPU;
+		state->flags &= ~HASH_FLAGS_CPU;
 
 out:
 	return stm32_hash_final(req);
@@ -1035,6 +1050,7 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 	u32 *preg;
 	unsigned int i;
 	int ret;
@@ -1045,11 +1061,9 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 	if (ret)
 		return ret;
 
-	rctx->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					 sizeof(u32),
-					 GFP_KERNEL);
-
-	preg = rctx->hw_context;
+	state->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
+					  sizeof(u32), GFP_KERNEL);
+	preg = state->hw_context;
 
 	if (!hdev->pdata->ux500)
 		*preg++ = stm32_hash_read(hdev, HASH_IMR);
@@ -1071,13 +1085,14 @@ static int stm32_hash_import(struct ahash_request *req, const void *in)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
+	struct stm32_hash_state *state = &rctx->state;
 	const u32 *preg = in;
 	u32 reg;
 	unsigned int i;
 
 	memcpy(rctx, in, sizeof(*rctx));
 
-	preg = rctx->hw_context;
+	preg = state->hw_context;
 
 	pm_runtime_get_sync(hdev->dev);
 
@@ -1094,7 +1109,7 @@ static int stm32_hash_import(struct ahash_request *req, const void *in)
 	pm_runtime_mark_last_busy(hdev->dev);
 	pm_runtime_put_autosuspend(hdev->dev);
 
-	kfree(rctx->hw_context);
+	kfree(state->hw_context);
 
 	return 0;
 }

[v7 PATCH 6/8] crypto: stm32 - Remove unused HASH_FLAGS_ERRORS

[Herbert Xu]

The bit HASH_FLAGS_ERRORS was never used.  Remove it.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index c836163a9fd4..478822fc7a4e 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -95,7 +95,6 @@
 #define HASH_FLAGS_SHA1			BIT(19)
 #define HASH_FLAGS_SHA224		BIT(20)
 #define HASH_FLAGS_SHA256		BIT(21)
-#define HASH_FLAGS_ERRORS		BIT(22)
 #define HASH_FLAGS_HMAC			BIT(23)
 
 #define HASH_OP_UPDATE			1
@@ -873,7 +872,6 @@ static int stm32_hash_finish(struct ahash_request *req)
 static void stm32_hash_finish_req(struct ahash_request *req, int err)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_state *state = &rctx->state;
 	struct stm32_hash_dev *hdev = rctx->hdev;
 
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
@@ -884,8 +882,6 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 				 HASH_FLAGS_OUTPUT_READY | HASH_FLAGS_HMAC |
 				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
 				 HASH_FLAGS_HMAC_KEY);
-	} else {
-		state->flags |= HASH_FLAGS_ERRORS;
 	}
 
 	pm_runtime_mark_last_busy(hdev->dev);

[v7 PATCH 7/8] crypto: stm32 - Fix empty message processing

[Herbert Xu]

Change the emptymsg check in stm32_hash_copy_hash to rely on whether
we have any existing hash state, rather than whether this particular
update request is empty.  

Also avoid computing the hash for empty messages as this could hang.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 478822fc7a4e..f898ec62b459 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -95,6 +95,7 @@
 #define HASH_FLAGS_SHA1			BIT(19)
 #define HASH_FLAGS_SHA224		BIT(20)
 #define HASH_FLAGS_SHA256		BIT(21)
+#define HASH_FLAGS_EMPTY		BIT(22)
 #define HASH_FLAGS_HMAC			BIT(23)
 
 #define HASH_OP_UPDATE			1
@@ -310,13 +311,6 @@ static void stm32_hash_write_ctrl(struct stm32_hash_dev *hdev, int bufcnt)
 				reg |= HASH_CR_LKEY;
 		}
 
-		/*
-		 * On the Ux500 we need to set a special flag to indicate that
-		 * the message is zero length.
-		 */
-		if (hdev->pdata->ux500 && bufcnt == 0)
-			reg |= HASH_CR_UX500_EMPTYMSG;
-
 		if (!hdev->polled)
 			stm32_hash_write(hdev, HASH_IMR, HASH_DCIE);
 
@@ -366,13 +360,23 @@ static void stm32_hash_append_sg(struct stm32_hash_request_ctx *rctx)
 static int stm32_hash_xmit_cpu(struct stm32_hash_dev *hdev,
 			       const u8 *buf, size_t length, int final)
 {
+	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+	struct stm32_hash_state *state = &rctx->state;
 	unsigned int count, len32;
 	const u32 *buffer = (const u32 *)buf;
 	u32 reg;
 
-	if (final)
+	if (final) {
 		hdev->flags |= HASH_FLAGS_FINAL;
 
+		/* Do not process empty messages if hw is buggy. */
+		if (!(hdev->flags & HASH_FLAGS_INIT) && !length &&
+		    hdev->pdata->broken_emptymsg) {
+			state->flags |= HASH_FLAGS_EMPTY;
+			return 0;
+		}
+	}
+
 	len32 = DIV_ROUND_UP(length, sizeof(u32));
 
 	dev_dbg(hdev->dev, "%s: length: %zd, final: %x len32 %i\n",
@@ -827,7 +831,7 @@ static void stm32_hash_copy_hash(struct ahash_request *req)
 	__be32 *hash = (void *)rctx->digest;
 	unsigned int i, hashsize;
 
-	if (hdev->pdata->broken_emptymsg && !req->nbytes)
+	if (hdev->pdata->broken_emptymsg && (state->flags & HASH_FLAGS_EMPTY))
 		return stm32_hash_emptymsg_fallback(req);
 
 	switch (state->flags & HASH_FLAGS_ALGO_MASK) {

Re: [v7 PATCH 7/8] crypto: stm32 - Fix empty message processing

[Linus Walleij]

On Sat, Mar 11, 2023 at 10:09 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Change the emptymsg check in stm32_hash_copy_hash to rely on whether
> we have any existing hash state, rather than whether this particular
> update request is empty.
>
> Also avoid computing the hash for empty messages as this could hang.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Excellent patch. Also works flawlessly.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[v7 PATCH 8/8] crypto: stm32 - Save and restore between each request

[Herbert Xu]

The Crypto API hashing paradigm requires the hardware state to
be exported between *each* request because multiple unrelated
hashes may be processed concurrently.

The stm32 hardware is capable of producing the hardware hashing
state but it was only doing it in the export function.  This is
not only broken for export as you can't export a kernel pointer
and reimport it, but it also means that concurrent hashing was
fundamentally broken.

Fix this by moving the saving and restoring of hardware hash
state between each and every hashing request.

Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
Reported-by: Li kunyu <kunyu@nfschina.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |  164 ++++++++++++--------------------------
 1 file changed, 56 insertions(+), 108 deletions(-)

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index f898ec62b459..17183f631bb4 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -135,7 +135,7 @@ struct stm32_hash_state {
 	u8 buffer[HASH_BUFLEN] __aligned(4);
 
 	/* hash state */
-	u32			*hw_context;
+	u32			hw_context[3 + HASH_CSR_REGISTER_NUMBER];
 };
 
 struct stm32_hash_request_ctx {
@@ -423,7 +423,9 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
 	struct stm32_hash_state *state = &rctx->state;
+	u32 *preg = state->hw_context;
 	int bufcnt, err = 0, final;
+	int i;
 
 	dev_dbg(hdev->dev, "%s flags %x\n", __func__, state->flags);
 
@@ -444,9 +446,24 @@ static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 	if (final) {
 		bufcnt = state->bufcnt;
 		state->bufcnt = 0;
-		err = stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
+		return stm32_hash_xmit_cpu(hdev, state->buffer, bufcnt, 1);
 	}
 
+	if (!(hdev->flags & HASH_FLAGS_INIT))
+		return 0;
+
+	if (stm32_hash_wait_busy(hdev))
+		return -ETIMEDOUT;
+
+	if (!hdev->pdata->ux500)
+		*preg++ = stm32_hash_read(hdev, HASH_IMR);
+	*preg++ = stm32_hash_read(hdev, HASH_STR);
+	*preg++ = stm32_hash_read(hdev, HASH_CR);
+	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
+
+	state->flags |= HASH_FLAGS_INIT;
+
 	return err;
 }
 
@@ -881,11 +898,6 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	if (!err && (HASH_FLAGS_FINAL & hdev->flags)) {
 		stm32_hash_copy_hash(req);
 		err = stm32_hash_finish(req);
-		hdev->flags &= ~(HASH_FLAGS_FINAL | HASH_FLAGS_CPU |
-				 HASH_FLAGS_INIT | HASH_FLAGS_DMA_READY |
-				 HASH_FLAGS_OUTPUT_READY | HASH_FLAGS_HMAC |
-				 HASH_FLAGS_HMAC_INIT | HASH_FLAGS_HMAC_FINAL |
-				 HASH_FLAGS_HMAC_KEY);
 	}
 
 	pm_runtime_mark_last_busy(hdev->dev);
@@ -894,66 +906,54 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
 	crypto_finalize_hash_request(hdev->engine, req, err);
 }
 
-static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
-			      struct stm32_hash_request_ctx *rctx)
-{
-	pm_runtime_get_sync(hdev->dev);
-
-	if (!(HASH_FLAGS_INIT & hdev->flags)) {
-		stm32_hash_write(hdev, HASH_CR, HASH_CR_INIT);
-		stm32_hash_write(hdev, HASH_STR, 0);
-		stm32_hash_write(hdev, HASH_DIN, 0);
-		stm32_hash_write(hdev, HASH_IMR, 0);
-	}
-
-	return 0;
-}
-
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq);
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq);
-
 static int stm32_hash_handle_queue(struct stm32_hash_dev *hdev,
 				   struct ahash_request *req)
 {
 	return crypto_transfer_hash_request_to_engine(hdev->engine, req);
 }
 
-static int stm32_hash_prepare_req(struct crypto_engine *engine, void *areq)
+static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
 {
 	struct ahash_request *req = container_of(areq, struct ahash_request,
 						 base);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
+	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_request_ctx *rctx;
+	struct stm32_hash_state *state = &rctx->state;
+	int err = 0;
 
 	if (!hdev)
 		return -ENODEV;
 
-	hdev->req = req;
-
-	rctx = ahash_request_ctx(req);
-
 	dev_dbg(hdev->dev, "processing new req, op: %lu, nbytes %d\n",
 		rctx->op, req->nbytes);
 
-	return stm32_hash_hw_init(hdev, rctx);
-}
+	pm_runtime_get_sync(hdev->dev);
 
-static int stm32_hash_one_request(struct crypto_engine *engine, void *areq)
-{
-	struct ahash_request *req = container_of(areq, struct ahash_request,
-						 base);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_request_ctx *rctx;
-	int err = 0;
+	hdev->req = req;
+	hdev->flags = 0;
+
+	if (state->flags & HASH_FLAGS_INIT) {
+		u32 *preg = rctx->state.hw_context;
+		u32 reg;
+		int i;
+
+		if (!hdev->pdata->ux500)
+			stm32_hash_write(hdev, HASH_IMR, *preg++);
+		stm32_hash_write(hdev, HASH_STR, *preg++);
+		stm32_hash_write(hdev, HASH_CR, *preg);
+		reg = *preg++ | HASH_CR_INIT;
+		stm32_hash_write(hdev, HASH_CR, reg);
 
-	if (!hdev)
-		return -ENODEV;
+		for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
+			stm32_hash_write(hdev, HASH_CSR(i), *preg++);
 
-	hdev->req = req;
+		hdev->flags |= HASH_FLAGS_INIT;
 
-	rctx = ahash_request_ctx(req);
+		if (state->flags & HASH_FLAGS_HMAC)
+			hdev->flags |= HASH_FLAGS_HMAC |
+				       HASH_FLAGS_HMAC_KEY;
+	}
 
 	if (rctx->op == HASH_OP_UPDATE)
 		err = stm32_hash_update_req(hdev);
@@ -1048,34 +1048,8 @@ static int stm32_hash_digest(struct ahash_request *req)
 static int stm32_hash_export(struct ahash_request *req, void *out)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_state *state = &rctx->state;
-	u32 *preg;
-	unsigned int i;
-	int ret;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	ret = stm32_hash_wait_busy(hdev);
-	if (ret)
-		return ret;
-
-	state->hw_context = kmalloc_array(3 + HASH_CSR_REGISTER_NUMBER,
-					  sizeof(u32), GFP_KERNEL);
-	preg = state->hw_context;
-
-	if (!hdev->pdata->ux500)
-		*preg++ = stm32_hash_read(hdev, HASH_IMR);
-	*preg++ = stm32_hash_read(hdev, HASH_STR);
-	*preg++ = stm32_hash_read(hdev, HASH_CR);
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		*preg++ = stm32_hash_read(hdev, HASH_CSR(i));
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
 
-	memcpy(out, rctx, sizeof(*rctx));
+	memcpy(out, &rctx->state, sizeof(rctx->state));
 
 	return 0;
 }
@@ -1083,33 +1057,9 @@ static int stm32_hash_export(struct ahash_request *req, void *out)
 static int stm32_hash_import(struct ahash_request *req, const void *in)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
-	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	struct stm32_hash_state *state = &rctx->state;
-	const u32 *preg = in;
-	u32 reg;
-	unsigned int i;
-
-	memcpy(rctx, in, sizeof(*rctx));
-
-	preg = state->hw_context;
-
-	pm_runtime_get_sync(hdev->dev);
-
-	if (!hdev->pdata->ux500)
-		stm32_hash_write(hdev, HASH_IMR, *preg++);
-	stm32_hash_write(hdev, HASH_STR, *preg++);
-	stm32_hash_write(hdev, HASH_CR, *preg);
-	reg = *preg++ | HASH_CR_INIT;
-	stm32_hash_write(hdev, HASH_CR, reg);
-
-	for (i = 0; i < HASH_CSR_REGISTER_NUMBER; i++)
-		stm32_hash_write(hdev, HASH_CSR(i), *preg++);
-
-	pm_runtime_mark_last_busy(hdev->dev);
-	pm_runtime_put_autosuspend(hdev->dev);
 
-	kfree(state->hw_context);
+	stm32_hash_init(req);
+	memcpy(&rctx->state, in, sizeof(rctx->state));
 
 	return 0;
 }
@@ -1166,8 +1116,6 @@ static int stm32_hash_cra_init_algs(struct crypto_tfm *tfm,
 		ctx->flags |= HASH_FLAGS_HMAC;
 
 	ctx->enginectx.op.do_one_request = stm32_hash_one_request;
-	ctx->enginectx.op.prepare_request = stm32_hash_prepare_req;
-	ctx->enginectx.op.unprepare_request = NULL;
 
 	return stm32_hash_init_fallback(tfm);
 }
@@ -1259,7 +1207,7 @@ static struct ahash_alg algs_md5[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "md5",
 				.cra_driver_name = "stm32-md5",
@@ -1286,7 +1234,7 @@ static struct ahash_alg algs_md5[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = MD5_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(md5)",
 				.cra_driver_name = "stm32-hmac-md5",
@@ -1315,7 +1263,7 @@ static struct ahash_alg algs_sha1[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "sha1",
 				.cra_driver_name = "stm32-sha1",
@@ -1342,7 +1290,7 @@ static struct ahash_alg algs_sha1[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA1_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(sha1)",
 				.cra_driver_name = "stm32-hmac-sha1",
@@ -1371,7 +1319,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "sha224",
 				.cra_driver_name = "stm32-sha224",
@@ -1398,7 +1346,7 @@ static struct ahash_alg algs_sha224[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA224_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(sha224)",
 				.cra_driver_name = "stm32-hmac-sha224",
@@ -1427,7 +1375,7 @@ static struct ahash_alg algs_sha256[] = {
 		.import = stm32_hash_import,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "sha256",
 				.cra_driver_name = "stm32-sha256",
@@ -1454,7 +1402,7 @@ static struct ahash_alg algs_sha256[] = {
 		.setkey = stm32_hash_setkey,
 		.halg = {
 			.digestsize = SHA256_DIGEST_SIZE,
-			.statesize = sizeof(struct stm32_hash_request_ctx),
+			.statesize = sizeof(struct stm32_hash_state),
 			.base = {
 				.cra_name = "hmac(sha256)",
 				.cra_driver_name = "stm32-hmac-sha256",

Re: [v7 PATCH 8/8] crypto: stm32 - Save and restore between each request

[Linus Walleij]

On Sat, Mar 11, 2023 at 10:09 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The Crypto API hashing paradigm requires the hardware state to
> be exported between *each* request because multiple unrelated
> hashes may be processed concurrently.
>
> The stm32 hardware is capable of producing the hardware hashing
> state but it was only doing it in the export function.  This is
> not only broken for export as you can't export a kernel pointer
> and reimport it, but it also means that concurrent hashing was
> fundamentally broken.
>
> Fix this by moving the saving and restoring of hardware hash
> state between each and every hashing request.
>
> Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
> Reported-by: Li kunyu <kunyu@nfschina.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [Linux-stm32] [v7 PATCH 8/8] crypto: stm32 - Save and restore between each request

[Herbert Xu]

On Mon, Mar 27, 2023 at 10:33:10AM +0200, Thomas BOURGOIN wrote:
> 
> I'm working on the patch for STM32MP13.

...
 
> The version of HASH implemented in the STM32MP13 provides new algorithms (SHA512, SHA3, ...).
> Because of that, the constant HASH_CSR_REGISTER_NUMBER increases (from 54 to 103).
> Hence, the size of stm32_hash_state is equal to 688 which is bigger than HASH_MAX_STATESIZE(=512)
> and the driver fails to register the algorithms.
> 
> Is there any reasons why HASH_MAX_STATESIZE is set to 512 ?
> I only see it used to define static arrays, so maybe it could be set to 1024.

Thanks for reaching out.  Please fix your emails so that they
are plain-text only, otherwise you won't be able to send any
patches to the list.

I have just sent out two patches to remove this limit.

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