Currently, inner IV/DIGEST data are only copied once into the hash
engines and not set explicitly before launching a request that is not a
first frag. This is an issue especially when multiple ahash reqs are
computed in parallel or chained with cipher request, as the state of the
request being computed is not updated into the hash engine. It leads to
non-deterministic corrupted digest results.
Fixes: commit 2786cee8e50b ("crypto: marvell - Move SRAM I/O operat...")
Signed-off-by: Romain Perier <[email protected]>
Cc: <[email protected]>
---
drivers/crypto/marvell/cesa.h | 3 ++-
drivers/crypto/marvell/hash.c | 29 ++++++++++++++++++++++++++++-
drivers/crypto/marvell/tdma.c | 9 ++++++++-
3 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/marvell/cesa.h b/drivers/crypto/marvell/cesa.h
index a768da7..b7872f6 100644
--- a/drivers/crypto/marvell/cesa.h
+++ b/drivers/crypto/marvell/cesa.h
@@ -273,7 +273,8 @@ struct mv_cesa_op_ctx {
#define CESA_TDMA_SRC_IN_SRAM BIT(30)
#define CESA_TDMA_END_OF_REQ BIT(29)
#define CESA_TDMA_BREAK_CHAIN BIT(28)
-#define CESA_TDMA_TYPE_MSK GENMASK(27, 0)
+#define CESA_TDMA_SET_STATE BIT(27)
+#define CESA_TDMA_TYPE_MSK GENMASK(26, 0)
#define CESA_TDMA_DUMMY 0
#define CESA_TDMA_DATA 1
#define CESA_TDMA_OP 2
diff --git a/drivers/crypto/marvell/hash.c b/drivers/crypto/marvell/hash.c
index 317cf02..4eb4564 100644
--- a/drivers/crypto/marvell/hash.c
+++ b/drivers/crypto/marvell/hash.c
@@ -285,8 +285,21 @@ static void mv_cesa_ahash_step(struct crypto_async_request *req)
struct ahash_request *ahashreq = ahash_request_cast(req);
struct mv_cesa_ahash_req *creq = ahash_request_ctx(ahashreq);
- if (mv_cesa_req_get_type(&creq->base) == CESA_DMA_REQ)
+ if (mv_cesa_req_get_type(&creq->base) == CESA_DMA_REQ) {
+
+ /* We must explicitely set the digest state. */
+ if (creq->base.chain.first->flags & CESA_TDMA_SET_STATE) {
+ struct mv_cesa_engine *engine = creq->base.engine;
+ int i;
+
+ /* Set the hash state in the IVDIG regs. */
+ for (i = 0; i < ARRAY_SIZE(creq->state); i++)
+ writel_relaxed(creq->state[i], engine->regs +
+ CESA_IVDIG(i));
+ }
+
mv_cesa_dma_step(&creq->base);
+ }
else
mv_cesa_ahash_std_step(ahashreq);
}
@@ -586,10 +599,14 @@ static int mv_cesa_ahash_dma_req_init(struct ahash_request *req)
unsigned int frag_len;
int ret;
u32 type;
+ bool set_state = false;
basereq->chain.first = NULL;
basereq->chain.last = NULL;
+ if (!mv_cesa_mac_op_is_first_frag(&creq->op_tmpl))
+ set_state = true;
+
if (creq->src_nents) {
ret = dma_map_sg(cesa_dev->dev, req->src, creq->src_nents,
DMA_TO_DEVICE);
@@ -683,6 +700,16 @@ static int mv_cesa_ahash_dma_req_init(struct ahash_request *req)
if (type != CESA_TDMA_RESULT)
basereq->chain.last->flags |= CESA_TDMA_BREAK_CHAIN;
+ if (set_state) {
+
+ /*
+ * Put the CESA_TDMA_SET_STATE flag on the first tdma desc to
+ * let the step logic know that the IVDIG registers should be
+ * explicitly set before launching a TDMA chain.
+ */
+ basereq->chain.first->flags |= CESA_TDMA_SET_STATE;
+ }
+
return 0;
err_free_tdma:
diff --git a/drivers/crypto/marvell/tdma.c b/drivers/crypto/marvell/tdma.c
index 4416b88..c76375f 100644
--- a/drivers/crypto/marvell/tdma.c
+++ b/drivers/crypto/marvell/tdma.c
@@ -109,7 +109,14 @@ void mv_cesa_tdma_chain(struct mv_cesa_engine *engine,
last->next = dreq->chain.first;
engine->chain.last = dreq->chain.last;
- if (!(last->flags & CESA_TDMA_BREAK_CHAIN))
+ /*
+ * Break the DMA chain if the CESA_TDMA_BREAK_CHAIN is set on
+ * the last element of the current chain, or if the request
+ * being queued needs the IV regs to be set before lauching
+ * the request.
+ */
+ if (!(last->flags & CESA_TDMA_BREAK_CHAIN) &&
+ !(dreq->chain.first->flags & CESA_TDMA_SET_STATE))
last->next_dma = dreq->chain.first->cur_dma;
}
}
--
2.9.3
On Wed, 14 Dec 2016 14:17:37 +0100
Romain Perier <[email protected]> wrote:
> Currently, inner IV/DIGEST data are only copied once into the hash
> engines and not set explicitly before launching a request that is not a
> first frag. This is an issue especially when multiple ahash reqs are
> computed in parallel or chained with cipher request, as the state of the
> request being computed is not updated into the hash engine. It leads to
> non-deterministic corrupted digest results.
>
> Fixes: commit 2786cee8e50b ("crypto: marvell - Move SRAM I/O operat...")
> Signed-off-by: Romain Perier <[email protected]>
> Cc: <[email protected]>
Just a minor comment (see below), otherwise
Acked-by: Boris Brezillon <[email protected]>
> ---
> drivers/crypto/marvell/cesa.h | 3 ++-
> drivers/crypto/marvell/hash.c | 29 ++++++++++++++++++++++++++++-
> drivers/crypto/marvell/tdma.c | 9 ++++++++-
> 3 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/marvell/cesa.h b/drivers/crypto/marvell/cesa.h
> index a768da7..b7872f6 100644
> --- a/drivers/crypto/marvell/cesa.h
> +++ b/drivers/crypto/marvell/cesa.h
> @@ -273,7 +273,8 @@ struct mv_cesa_op_ctx {
> #define CESA_TDMA_SRC_IN_SRAM BIT(30)
> #define CESA_TDMA_END_OF_REQ BIT(29)
> #define CESA_TDMA_BREAK_CHAIN BIT(28)
> -#define CESA_TDMA_TYPE_MSK GENMASK(27, 0)
> +#define CESA_TDMA_SET_STATE BIT(27)
> +#define CESA_TDMA_TYPE_MSK GENMASK(26, 0)
> #define CESA_TDMA_DUMMY 0
> #define CESA_TDMA_DATA 1
> #define CESA_TDMA_OP 2
> diff --git a/drivers/crypto/marvell/hash.c b/drivers/crypto/marvell/hash.c
> index 317cf02..4eb4564 100644
> --- a/drivers/crypto/marvell/hash.c
> +++ b/drivers/crypto/marvell/hash.c
> @@ -285,8 +285,21 @@ static void mv_cesa_ahash_step(struct crypto_async_request *req)
> struct ahash_request *ahashreq = ahash_request_cast(req);
> struct mv_cesa_ahash_req *creq = ahash_request_ctx(ahashreq);
>
> - if (mv_cesa_req_get_type(&creq->base) == CESA_DMA_REQ)
> + if (mv_cesa_req_get_type(&creq->base) == CESA_DMA_REQ) {
> +
> + /* We must explicitely set the digest state. */
> + if (creq->base.chain.first->flags & CESA_TDMA_SET_STATE) {
> + struct mv_cesa_engine *engine = creq->base.engine;
> + int i;
> +
> + /* Set the hash state in the IVDIG regs. */
> + for (i = 0; i < ARRAY_SIZE(creq->state); i++)
> + writel_relaxed(creq->state[i], engine->regs +
> + CESA_IVDIG(i));
> + }
> +
> mv_cesa_dma_step(&creq->base);
Nit: can you move the above code in a function called
mv_cesa_ahash_dma_step()?
> + }
> else
> mv_cesa_ahash_std_step(ahashreq);
I'm pretty sure checkpatch complains here ;-).
> }
> @@ -586,10 +599,14 @@ static int mv_cesa_ahash_dma_req_init(struct ahash_request *req)
> unsigned int frag_len;
> int ret;
> u32 type;
> + bool set_state = false;
>
> basereq->chain.first = NULL;
> basereq->chain.last = NULL;
>
> + if (!mv_cesa_mac_op_is_first_frag(&creq->op_tmpl))
> + set_state = true;
> +
> if (creq->src_nents) {
> ret = dma_map_sg(cesa_dev->dev, req->src, creq->src_nents,
> DMA_TO_DEVICE);
> @@ -683,6 +700,16 @@ static int mv_cesa_ahash_dma_req_init(struct ahash_request *req)
> if (type != CESA_TDMA_RESULT)
> basereq->chain.last->flags |= CESA_TDMA_BREAK_CHAIN;
>
> + if (set_state) {
> +
> + /*
> + * Put the CESA_TDMA_SET_STATE flag on the first tdma desc to
> + * let the step logic know that the IVDIG registers should be
> + * explicitly set before launching a TDMA chain.
> + */
> + basereq->chain.first->flags |= CESA_TDMA_SET_STATE;
> + }
> +
> return 0;
>
> err_free_tdma:
> diff --git a/drivers/crypto/marvell/tdma.c b/drivers/crypto/marvell/tdma.c
> index 4416b88..c76375f 100644
> --- a/drivers/crypto/marvell/tdma.c
> +++ b/drivers/crypto/marvell/tdma.c
> @@ -109,7 +109,14 @@ void mv_cesa_tdma_chain(struct mv_cesa_engine *engine,
> last->next = dreq->chain.first;
> engine->chain.last = dreq->chain.last;
>
> - if (!(last->flags & CESA_TDMA_BREAK_CHAIN))
> + /*
> + * Break the DMA chain if the CESA_TDMA_BREAK_CHAIN is set on
> + * the last element of the current chain, or if the request
> + * being queued needs the IV regs to be set before lauching
> + * the request.
> + */
> + if (!(last->flags & CESA_TDMA_BREAK_CHAIN) &&
> + !(dreq->chain.first->flags & CESA_TDMA_SET_STATE))
> last->next_dma = dreq->chain.first->cur_dma;
> }
> }
Hi,
Le 14/12/2016 ? 14:39, Boris Brezillon a ?crit :
> Nit: can you move the above code in a function called
> mv_cesa_ahash_dma_step()?
Sure
>
>> + }
>> else
>> mv_cesa_ahash_std_step(ahashreq);
>
> I'm pretty sure checkpatch complains here ;-).
Probably :P
I will fix these points,
Thanks,
Romain