[PATCH] crypto: caam - fix rfc4106 encap shared descriptor

[PATCH] crypto: caam - fix rfc4106 encap shared descriptor

[Horia Geantă]

The encap shared descriptor was changed to use the new IV convention.
In the process some commands were shifted, making the output length
zero, caam effectively writing garbage in dst.

While here, update the decap descriptor to execute the "write" commands
before the "read"s (as it previously was).
This makes sure the input fifo is drained before becoming full.

Fixes: 46218750d523 ("crypto: caam - Use new IV convention")
Signed-off-by: Horia Geantă <horia.geanta@freescale.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@freescale.com>
---
 drivers/crypto/caam/caamalg.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 3c50a5082127..b08ae6983d1f 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -989,19 +989,19 @@ static int rfc4106_set_sh_desc(struct crypto_aead *aead)
 	/* Will read cryptlen bytes */
 	append_math_sub(desc, VARSEQINLEN, SEQINLEN, REG0, CAAM_CMD_SZ);
 
-	/* Read payload data */
-	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF |
-			     FIFOLD_TYPE_MSG | FIFOLD_TYPE_LAST1);
-
 	/* Skip assoc data */
 	append_seq_fifo_store(desc, 0, FIFOST_TYPE_SKIP | FIFOLDST_VLF);
 
 	/* cryptlen = seqoutlen - assoclen */
-	append_math_sub(desc, VARSEQOUTLEN, SEQINLEN, REG0, CAAM_CMD_SZ);
+	append_math_sub(desc, VARSEQOUTLEN, VARSEQINLEN, REG0, CAAM_CMD_SZ);
 
 	/* Write encrypted data */
 	append_seq_fifo_store(desc, 0, FIFOST_TYPE_MESSAGE_DATA | FIFOLDST_VLF);
 
+	/* Read payload data */
+	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF |
+			     FIFOLD_TYPE_MSG | FIFOLD_TYPE_LAST1);
+
 	/* Write ICV */
 	append_seq_store(desc, ctx->authsize, LDST_CLASS_1_CCB |
 			 LDST_SRCDST_BYTE_CONTEXT);
@@ -1060,10 +1060,6 @@ static int rfc4106_set_sh_desc(struct crypto_aead *aead)
 	/* Will read cryptlen bytes */
 	append_math_sub(desc, VARSEQINLEN, SEQOUTLEN, REG3, CAAM_CMD_SZ);
 
-	/* Read encrypted data */
-	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF |
-			     FIFOLD_TYPE_MSG | FIFOLD_TYPE_FLUSH1);
-
 	/* Skip assoc data */
 	append_seq_fifo_store(desc, 0, FIFOST_TYPE_SKIP | FIFOLDST_VLF);
 
@@ -1073,6 +1069,10 @@ static int rfc4106_set_sh_desc(struct crypto_aead *aead)
 	/* Store payload data */
 	append_seq_fifo_store(desc, 0, FIFOST_TYPE_MESSAGE_DATA | FIFOLDST_VLF);
 
+	/* Read encrypted data */
+	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF |
+			     FIFOLD_TYPE_MSG | FIFOLD_TYPE_FLUSH1);
+
 	/* Read ICV */
 	append_seq_fifo_load(desc, ctx->authsize, FIFOLD_CLASS_CLASS1 |
 			     FIFOLD_TYPE_ICV | FIFOLD_TYPE_LAST1);
-- 
2.4.4

Re: [PATCH] crypto: caam - fix rfc4106 encap shared descriptor

[Herbert Xu]

On Thu, Jul 30, 2015 at 04:39:26PM +0300, Horia Geantă wrote:
> The encap shared descriptor was changed to use the new IV convention.
> In the process some commands were shifted, making the output length
> zero, caam effectively writing garbage in dst.

Thanks.

> While here, update the decap descriptor to execute the "write" commands
> before the "read"s (as it previously was).
> This makes sure the input fifo is drained before becoming full.

Actually, I deliberately did it that way because there was an errata
which said that doing a skipping load concurrently with a skipping
store may cause a hang.  Is this not the case?

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: [PATCH] crypto: caam - fix rfc4106 encap shared descriptor

[Horia Geantă]

On 7/30/2015 4:46 PM, Herbert Xu wrote:
> On Thu, Jul 30, 2015 at 04:39:26PM +0300, Horia Geantă wrote:
>> The encap shared descriptor was changed to use the new IV convention.
>> In the process some commands were shifted, making the output length
>> zero, caam effectively writing garbage in dst.
> 
> Thanks.
> 
>> While here, update the decap descriptor to execute the "write" commands
>> before the "read"s (as it previously was).
>> This makes sure the input fifo is drained before becoming full.
> 
> Actually, I deliberately did it that way because there was an errata
> which said that doing a skipping load concurrently with a skipping
> store may cause a hang.  Is this not the case?
> 
Indeed, there is:
A-005473 - Using SEQ FIFO LOAD SKIP and SEQ FIFO STORE SKIP
simultaneously will cause the DECO to hang.

However, the skip commands are not consecutive, there's a math command
between them (both for encap and decap descriptors).

Horia

Re: [PATCH] crypto: caam - fix rfc4106 encap shared descriptor

[Herbert Xu]

On Thu, Jul 30, 2015 at 04:59:01PM +0300, Horia Geantă wrote:
>
> Indeed, there is:
> A-005473 - Using SEQ FIFO LOAD SKIP and SEQ FIFO STORE SKIP
> simultaneously will cause the DECO to hang.
> 
> However, the skip commands are not consecutive, there's a math command
> between them (both for encap and decap descriptors).

Cool, I didn't know that inserting a math command could also
work around the hang.

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] crypto: caam - fix rfc4106 encap shared descriptor

[Horia Geantă]

On 7/30/2015 5:03 PM, Herbert Xu wrote:
> On Thu, Jul 30, 2015 at 04:59:01PM +0300, Horia Geantă wrote:
>>
>> Indeed, there is:
>> A-005473 - Using SEQ FIFO LOAD SKIP and SEQ FIFO STORE SKIP
>> simultaneously will cause the DECO to hang.
>>
>> However, the skip commands are not consecutive, there's a math command
>> between them (both for encap and decap descriptors).
> 
> Cool, I didn't know that inserting a math command could also
> work around the hang.

We double checked this with the HW design team.
While tests pass, we're not safe with the approach, since the issue is
timing sensible and depends on S/G tables format, whether they are
previously fetched etc.

I'll send a v2 shortly, using the proposed erratum workaround.

Thanks,
Horia

[PATCH v2] crypto: caam - fix rfc4106 encap shared descriptor

[Horia Geantă]

The encap shared descriptor was changed to use the new IV convention.
In the process some commands were shifted, making the output length
zero, caam effectively writing garbage in dst.

While here, update the decap descriptor to execute the "write" commands
before the "read"s (as it previously was).
This makes sure the input fifo is drained before becoming full.

Fixes: 46218750d523 ("crypto: caam - Use new IV convention")
Signed-off-by: Horia Geantă <horia.geanta@freescale.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@freescale.com>
---

v2 - added erratum workaround

 drivers/crypto/caam/caamalg.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 3c50a5082127..e49373409582 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -87,8 +87,8 @@
 #define DESC_GCM_DEC_LEN		(DESC_GCM_BASE + 12 * CAAM_CMD_SZ)
 
 #define DESC_RFC4106_BASE		(3 * CAAM_CMD_SZ)
-#define DESC_RFC4106_ENC_LEN		(DESC_RFC4106_BASE + 12 * CAAM_CMD_SZ)
-#define DESC_RFC4106_DEC_LEN		(DESC_RFC4106_BASE + 12 * CAAM_CMD_SZ)
+#define DESC_RFC4106_ENC_LEN		(DESC_RFC4106_BASE + 13 * CAAM_CMD_SZ)
+#define DESC_RFC4106_DEC_LEN		(DESC_RFC4106_BASE + 13 * CAAM_CMD_SZ)
 
 #define DESC_RFC4543_BASE		(3 * CAAM_CMD_SZ)
 #define DESC_RFC4543_ENC_LEN		(DESC_RFC4543_BASE + 11 * CAAM_CMD_SZ)
@@ -989,19 +989,22 @@ static int rfc4106_set_sh_desc(struct crypto_aead *aead)
 	/* Will read cryptlen bytes */
 	append_math_sub(desc, VARSEQINLEN, SEQINLEN, REG0, CAAM_CMD_SZ);
 
-	/* Read payload data */
-	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF |
-			     FIFOLD_TYPE_MSG | FIFOLD_TYPE_LAST1);
+	/* Workaround for erratum A-005473 (simultaneous SEQ FIFO skips) */
+	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLD_TYPE_MSG);
 
 	/* Skip assoc data */
 	append_seq_fifo_store(desc, 0, FIFOST_TYPE_SKIP | FIFOLDST_VLF);
 
 	/* cryptlen = seqoutlen - assoclen */
-	append_math_sub(desc, VARSEQOUTLEN, SEQINLEN, REG0, CAAM_CMD_SZ);
+	append_math_sub(desc, VARSEQOUTLEN, VARSEQINLEN, REG0, CAAM_CMD_SZ);
 
 	/* Write encrypted data */
 	append_seq_fifo_store(desc, 0, FIFOST_TYPE_MESSAGE_DATA | FIFOLDST_VLF);
 
+	/* Read payload data */
+	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF |
+			     FIFOLD_TYPE_MSG | FIFOLD_TYPE_LAST1);
+
 	/* Write ICV */
 	append_seq_store(desc, ctx->authsize, LDST_CLASS_1_CCB |
 			 LDST_SRCDST_BYTE_CONTEXT);
@@ -1060,9 +1063,8 @@ static int rfc4106_set_sh_desc(struct crypto_aead *aead)
 	/* Will read cryptlen bytes */
 	append_math_sub(desc, VARSEQINLEN, SEQOUTLEN, REG3, CAAM_CMD_SZ);
 
-	/* Read encrypted data */
-	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF |
-			     FIFOLD_TYPE_MSG | FIFOLD_TYPE_FLUSH1);
+	/* Workaround for erratum A-005473 (simultaneous SEQ FIFO skips) */
+	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLD_TYPE_MSG);
 
 	/* Skip assoc data */
 	append_seq_fifo_store(desc, 0, FIFOST_TYPE_SKIP | FIFOLDST_VLF);
@@ -1073,6 +1075,10 @@ static int rfc4106_set_sh_desc(struct crypto_aead *aead)
 	/* Store payload data */
 	append_seq_fifo_store(desc, 0, FIFOST_TYPE_MESSAGE_DATA | FIFOLDST_VLF);
 
+	/* Read encrypted data */
+	append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF |
+			     FIFOLD_TYPE_MSG | FIFOLD_TYPE_FLUSH1);
+
 	/* Read ICV */
 	append_seq_fifo_load(desc, ctx->authsize, FIFOLD_CLASS_CLASS1 |
 			     FIFOLD_TYPE_ICV | FIFOLD_TYPE_LAST1);
-- 
2.4.4

Re: [PATCH v2] crypto: caam - fix rfc4106 encap shared descriptor

[Herbert Xu]

On Thu, Jul 30, 2015 at 10:11:18PM +0300, Horia Geantă wrote:
> The encap shared descriptor was changed to use the new IV convention.
> In the process some commands were shifted, making the output length
> zero, caam effectively writing garbage in dst.
> 
> While here, update the decap descriptor to execute the "write" commands
> before the "read"s (as it previously was).
> This makes sure the input fifo is drained before becoming full.
> 
> Fixes: 46218750d523 ("crypto: caam - Use new IV convention")
> Signed-off-by: Horia Geantă <horia.geanta@freescale.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@freescale.com>

Patch applied.  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