diff mbox series

[1/4] crypto4xx: fix ctr-aes missing output IV

Message ID 4c860f87b9339da1d1f700ba6a56a7a5e2eb14da.1555932334.git.chunkeey@gmail.com (mailing list archive)
State Accepted
Delegated to: Herbert Xu
Headers show
Series [1/4] crypto4xx: fix ctr-aes missing output IV | expand

Commit Message

Christian Lamparter April 22, 2019, 11:25 a.m. UTC
Commit 8efd972ef96a ("crypto: testmgr - support checking skcipher output IV")
caused the crypto4xx driver to produce the following error:

| ctr-aes-ppc4xx encryption test failed (wrong output IV)
| on test vector 0, cfg="in-place"

This patch fixes this by reworking the crypto4xx_setkey_aes()
function to:

 - not save the iv for ECB (as per 18.2.38 CRYP0_SA_CMD_0:
   "This bit mut be cleared for DES ECB mode or AES ECB mode,
   when no IV is used.")

 - instruct the hardware to save the generated IV for all
   other modes of operations that have IV and then supply
   it back to the callee in pretty much the same way as we
   do it for cbc-aes already.

 - make it clear that the DIR_(IN|OUT)BOUND is the important
   bit that tells the hardware to encrypt or decrypt the data.
   (this is cosmetic - but it hopefully prevents me from
    getting confused again).

 - don't load any bogus hash when we don't use any hash
   operation to begin with.

Cc: stable@vger.kernel.org
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 drivers/crypto/amcc/crypto4xx_alg.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Christian Lamparter April 22, 2019, 4:27 p.m. UTC | #1
On 4/22/19, Sasha Levin <sashal@kernel.org> wrote:
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.0.9, v4.19.36, v4.14.113,
> v4.9.170, v4.4.178, v3.18.138.
>
> v5.0.9: Build OK!
> v4.19.36: Build OK!
> v4.14.113: Failed to apply! Possible dependencies:
>     fc340115ffb8 ("crypto: crypto4xx - properly set IV after de- and
> encrypt")
>
> v4.9.170: Failed to apply! Possible dependencies:
>     fc340115ffb8 ("crypto: crypto4xx - properly set IV after de- and
> encrypt")
>
> v4.4.178: Failed to apply! Possible dependencies:
>     fc340115ffb8 ("crypto: crypto4xx - properly set IV after de- and
> encrypt")
>
> v3.18.138: Failed to apply! Possible dependencies:
>     fc340115ffb8 ("crypto: crypto4xx - properly set IV after de- and
> encrypt")
>
>
> How should we proceed with this patch?

I only added AES-CTR support around a year ago.
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/98e87e3d933b8e504ea41b8857c038d2cd06cddc
(so, this is another dependency)

In my opinion back-porting the patch to 4.14 and older would not do
much since the mode is not available there. But, I'm glad that the
patches got a bit of attention.

Cheers,
Christian
Herbert Xu May 3, 2019, 6:08 a.m. UTC | #2
On Mon, Apr 22, 2019 at 01:25:58PM +0200, Christian Lamparter wrote:
> Commit 8efd972ef96a ("crypto: testmgr - support checking skcipher output IV")
> caused the crypto4xx driver to produce the following error:
> 
> | ctr-aes-ppc4xx encryption test failed (wrong output IV)
> | on test vector 0, cfg="in-place"
> 
> This patch fixes this by reworking the crypto4xx_setkey_aes()
> function to:
> 
>  - not save the iv for ECB (as per 18.2.38 CRYP0_SA_CMD_0:
>    "This bit mut be cleared for DES ECB mode or AES ECB mode,
>    when no IV is used.")
> 
>  - instruct the hardware to save the generated IV for all
>    other modes of operations that have IV and then supply
>    it back to the callee in pretty much the same way as we
>    do it for cbc-aes already.
> 
>  - make it clear that the DIR_(IN|OUT)BOUND is the important
>    bit that tells the hardware to encrypt or decrypt the data.
>    (this is cosmetic - but it hopefully prevents me from
>     getting confused again).
> 
>  - don't load any bogus hash when we don't use any hash
>    operation to begin with.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>  drivers/crypto/amcc/crypto4xx_alg.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

All applied.  Thanks.
diff mbox series

Patch

diff --git a/drivers/crypto/amcc/crypto4xx_alg.c b/drivers/crypto/amcc/crypto4xx_alg.c
index 4092c2aad8e2..3458c5a085d9 100644
--- a/drivers/crypto/amcc/crypto4xx_alg.c
+++ b/drivers/crypto/amcc/crypto4xx_alg.c
@@ -141,9 +141,10 @@  static int crypto4xx_setkey_aes(struct crypto_skcipher *cipher,
 	/* Setup SA */
 	sa = ctx->sa_in;
 
-	set_dynamic_sa_command_0(sa, SA_NOT_SAVE_HASH, (cm == CRYPTO_MODE_CBC ?
-				 SA_SAVE_IV : SA_NOT_SAVE_IV),
-				 SA_LOAD_HASH_FROM_SA, SA_LOAD_IV_FROM_STATE,
+	set_dynamic_sa_command_0(sa, SA_NOT_SAVE_HASH, (cm == CRYPTO_MODE_ECB ?
+				 SA_NOT_SAVE_IV : SA_SAVE_IV),
+				 SA_NOT_LOAD_HASH, (cm == CRYPTO_MODE_ECB ?
+				 SA_LOAD_IV_FROM_SA : SA_LOAD_IV_FROM_STATE),
 				 SA_NO_HEADER_PROC, SA_HASH_ALG_NULL,
 				 SA_CIPHER_ALG_AES, SA_PAD_TYPE_ZERO,
 				 SA_OP_GROUP_BASIC, SA_OPCODE_DECRYPT,
@@ -162,6 +163,11 @@  static int crypto4xx_setkey_aes(struct crypto_skcipher *cipher,
 	memcpy(ctx->sa_out, ctx->sa_in, ctx->sa_len * 4);
 	sa = ctx->sa_out;
 	sa->sa_command_0.bf.dir = DIR_OUTBOUND;
+	/*
+	 * SA_OPCODE_ENCRYPT is the same value as SA_OPCODE_DECRYPT.
+	 * it's the DIR_(IN|OUT)BOUND that matters
+	 */
+	sa->sa_command_0.bf.opcode = SA_OPCODE_ENCRYPT;
 
 	return 0;
 }