[PATCH 0/3] crypto: inside-secure - add more AEAD ciphersuites

[PATCH 0/3] crypto: inside-secure - add more AEAD ciphersuites

[Pascal van Leeuwen]

This patch set adds support for the following additional AEAD suites:
- authenc(hmac(sha1),cbc(des3_ede))
- authenc(hmac(sha1),rfc3686(ctr(aes)))
- authenc(hmac(sha224),rfc3686(ctr(aes)))
- authenc(hmac(sha256),rfc3686(ctr(aes)))
- authenc(hmac(sha384),rfc3686(ctr(aes)))
- authenc(hmac(sha512),rfc3686(ctr(aes)))

It has been verified on an FPGA devboard and Macchiatobin (Armada 8K)

Pascal van Leeuwen (3):
  crypto: inside-secure - add support for
    authenc(hmac(sha1),cbc(des3_ede))
  crypto: inside-secure - added support for rfc3686(ctr(aes))
  crypto: inside-secure - add support for
    authenc(hmac(sha*),rfc3686(ctr(aes))) suites

 drivers/crypto/inside-secure/safexcel.c        |  30 +-
 drivers/crypto/inside-secure/safexcel.h        |  38 +--
 drivers/crypto/inside-secure/safexcel_cipher.c | 456 ++++++++++++++++++++++---
 3 files changed, 428 insertions(+), 96 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))

[Pascal van Leeuwen]

Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 drivers/crypto/inside-secure/safexcel.c        |  1 +
 drivers/crypto/inside-secure/safexcel.h        |  1 +
 drivers/crypto/inside-secure/safexcel_cipher.c | 89 ++++++++++++++++++++------
 3 files changed, 72 insertions(+), 19 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index 8e8c01d..c3bb177 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -1004,6 +1004,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid,
 	&safexcel_alg_authenc_hmac_sha256_cbc_aes,
 	&safexcel_alg_authenc_hmac_sha384_cbc_aes,
 	&safexcel_alg_authenc_hmac_sha512_cbc_aes,
+	&safexcel_alg_authenc_hmac_sha1_cbc_des3_ede,
 };
 
 static int safexcel_register_algorithms(struct safexcel_crypto_priv *priv)
diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h
index b68fec3..765f481 100644
--- a/drivers/crypto/inside-secure/safexcel.h
+++ b/drivers/crypto/inside-secure/safexcel.h
@@ -765,5 +765,6 @@ int safexcel_hmac_setkey(const char *alg, const u8 *key, unsigned int keylen,
 extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha256_cbc_aes;
 extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_cbc_aes;
 extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha512_cbc_aes;
+extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede;
 
 #endif
diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index ea122dd..5eed890 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -183,14 +183,16 @@ static int safexcel_skcipher_aes_setkey(struct crypto_skcipher *ctfm,
 	return 0;
 }
 
-static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key,
-				    unsigned int len)
+static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
+				unsigned int len)
 {
 	struct crypto_tfm *tfm = crypto_aead_tfm(ctfm);
 	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
 	struct safexcel_ahash_export_state istate, ostate;
 	struct safexcel_crypto_priv *priv = ctx->priv;
 	struct crypto_authenc_keys keys;
+	u32 flags;
+	int err;
 
 	if (crypto_authenc_extractkeys(&keys, key, len) != 0)
 		goto badkey;
@@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key,
 		goto badkey;
 
 	/* Encryption key */
+	if (ctx->alg == SAFEXCEL_3DES) {
+		flags = crypto_aead_get_flags(ctfm);
+		err = __des3_verify_key(&flags, keys.enckey);
+		crypto_aead_set_flags(ctfm, flags);
+
+		if (unlikely(err))
+			return err;
+	}
+
 	if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma &&
 	    memcmp(ctx->key, keys.enckey, keys.enckeylen))
 		ctx->base.needs_inv = true;
@@ -1240,7 +1251,7 @@ struct safexcel_alg_template safexcel_alg_ecb_des3_ede = {
 	},
 };
 
-static int safexcel_aead_encrypt(struct aead_request *req)
+static int safexcel_aead_encrypt_aes(struct aead_request *req)
 {
 	struct safexcel_cipher_req *creq = aead_request_ctx(req);
 
@@ -1248,7 +1259,7 @@ static int safexcel_aead_encrypt(struct aead_request *req)
 			CONTEXT_CONTROL_CRYPTO_MODE_CBC, SAFEXCEL_AES);
 }
 
-static int safexcel_aead_decrypt(struct aead_request *req)
+static int safexcel_aead_decrypt_aes(struct aead_request *req)
 {
 	struct safexcel_cipher_req *creq = aead_request_ctx(req);
 
@@ -1287,9 +1298,9 @@ static int safexcel_aead_sha1_cra_init(struct crypto_tfm *tfm)
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
 	.alg.aead = {
-		.setkey = safexcel_aead_aes_setkey,
-		.encrypt = safexcel_aead_encrypt,
-		.decrypt = safexcel_aead_decrypt,
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
 		.ivsize = AES_BLOCK_SIZE,
 		.maxauthsize = SHA1_DIGEST_SIZE,
 		.base = {
@@ -1321,9 +1332,9 @@ static int safexcel_aead_sha256_cra_init(struct crypto_tfm *tfm)
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha256_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
 	.alg.aead = {
-		.setkey = safexcel_aead_aes_setkey,
-		.encrypt = safexcel_aead_encrypt,
-		.decrypt = safexcel_aead_decrypt,
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
 		.ivsize = AES_BLOCK_SIZE,
 		.maxauthsize = SHA256_DIGEST_SIZE,
 		.base = {
@@ -1355,9 +1366,9 @@ static int safexcel_aead_sha224_cra_init(struct crypto_tfm *tfm)
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha224_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
 	.alg.aead = {
-		.setkey = safexcel_aead_aes_setkey,
-		.encrypt = safexcel_aead_encrypt,
-		.decrypt = safexcel_aead_decrypt,
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
 		.ivsize = AES_BLOCK_SIZE,
 		.maxauthsize = SHA224_DIGEST_SIZE,
 		.base = {
@@ -1389,9 +1400,9 @@ static int safexcel_aead_sha512_cra_init(struct crypto_tfm *tfm)
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha512_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
 	.alg.aead = {
-		.setkey = safexcel_aead_aes_setkey,
-		.encrypt = safexcel_aead_encrypt,
-		.decrypt = safexcel_aead_decrypt,
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
 		.ivsize = AES_BLOCK_SIZE,
 		.maxauthsize = SHA512_DIGEST_SIZE,
 		.base = {
@@ -1423,9 +1434,9 @@ static int safexcel_aead_sha384_cra_init(struct crypto_tfm *tfm)
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
 	.alg.aead = {
-		.setkey = safexcel_aead_aes_setkey,
-		.encrypt = safexcel_aead_encrypt,
-		.decrypt = safexcel_aead_decrypt,
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
 		.ivsize = AES_BLOCK_SIZE,
 		.maxauthsize = SHA384_DIGEST_SIZE,
 		.base = {
@@ -1443,3 +1454,43 @@ struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_cbc_aes = {
 		},
 	},
 };
+
+static int safexcel_aead_encrypt_3des(struct aead_request *req)
+{
+	struct safexcel_cipher_req *creq = aead_request_ctx(req);
+
+	return safexcel_queue_req(&req->base, creq, SAFEXCEL_ENCRYPT,
+			CONTEXT_CONTROL_CRYPTO_MODE_CBC, SAFEXCEL_3DES);
+}
+
+static int safexcel_aead_decrypt_3des(struct aead_request *req)
+{
+	struct safexcel_cipher_req *creq = aead_request_ctx(req);
+
+	return safexcel_queue_req(&req->base, creq, SAFEXCEL_DECRYPT,
+			CONTEXT_CONTROL_CRYPTO_MODE_CBC, SAFEXCEL_3DES);
+}
+
+struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = {
+	.type = SAFEXCEL_ALG_TYPE_AEAD,
+	.alg.aead = {
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_3des,
+		.decrypt = safexcel_aead_decrypt_3des,
+		.ivsize = DES3_EDE_BLOCK_SIZE,
+		.maxauthsize = SHA1_DIGEST_SIZE,
+		.base = {
+			.cra_name = "authenc(hmac(sha1),cbc(des3_ede))",
+			.cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede",
+			.cra_priority = 300,
+			.cra_flags = CRYPTO_ALG_ASYNC |
+				     CRYPTO_ALG_KERN_DRIVER_ONLY,
+			.cra_blocksize = DES3_EDE_BLOCK_SIZE,
+			.cra_ctxsize = sizeof(struct safexcel_cipher_ctx),
+			.cra_alignmask = 0,
+			.cra_init = safexcel_aead_sha1_cra_init,
+			.cra_exit = safexcel_aead_cra_exit,
+			.cra_module = THIS_MODULE,
+		},
+	},
+};
-- 
1.8.3.1

[PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Pascal van Leeuwen]

Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 drivers/crypto/inside-secure/safexcel.c        |  15 +--
 drivers/crypto/inside-secure/safexcel.h        |  32 +----
 drivers/crypto/inside-secure/safexcel_cipher.c | 155 +++++++++++++++++++++----
 3 files changed, 138 insertions(+), 64 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index c3bb177..26f086b 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -506,17 +506,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
 		      EIP197_PE_EIP96_TOKEN_CTRL_POST_REUSE_CTX;
 		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_TOKEN_CTRL(pe));
 
-		/* H/W capabilities selection */
-		val = EIP197_FUNCTION_RSVD;
-		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
-		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
-		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
-		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
-		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
-		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
-		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
-		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
-		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
+		/* H/W capabilities selection: just enable everything */
+		writel(EIP197_FUNCTION_ALL,
+		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
 	}
 
 	/* Command Descriptor Rings prepare */
@@ -987,6 +979,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid,
 	&safexcel_alg_cbc_des3_ede,
 	&safexcel_alg_ecb_aes,
 	&safexcel_alg_cbc_aes,
+	&safexcel_alg_ctr_aes,
 	&safexcel_alg_md5,
 	&safexcel_alg_sha1,
 	&safexcel_alg_sha224,
diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h
index 765f481..af71120 100644
--- a/drivers/crypto/inside-secure/safexcel.h
+++ b/drivers/crypto/inside-secure/safexcel.h
@@ -279,35 +279,7 @@
 #define EIP197_PE_EIP96_TOKEN_CTRL_POST_REUSE_CTX	BIT(20)
 
 /* EIP197_PE_EIP96_FUNCTION_EN */
-#define EIP197_FUNCTION_RSVD			(BIT(6) | BIT(15) | BIT(20) | BIT(23))
-#define EIP197_PROTOCOL_HASH_ONLY		BIT(0)
-#define EIP197_PROTOCOL_ENCRYPT_ONLY		BIT(1)
-#define EIP197_PROTOCOL_HASH_ENCRYPT		BIT(2)
-#define EIP197_PROTOCOL_HASH_DECRYPT		BIT(3)
-#define EIP197_PROTOCOL_ENCRYPT_HASH		BIT(4)
-#define EIP197_PROTOCOL_DECRYPT_HASH		BIT(5)
-#define EIP197_ALG_ARC4				BIT(7)
-#define EIP197_ALG_AES_ECB			BIT(8)
-#define EIP197_ALG_AES_CBC			BIT(9)
-#define EIP197_ALG_AES_CTR_ICM			BIT(10)
-#define EIP197_ALG_AES_OFB			BIT(11)
-#define EIP197_ALG_AES_CFB			BIT(12)
-#define EIP197_ALG_DES_ECB			BIT(13)
-#define EIP197_ALG_DES_CBC			BIT(14)
-#define EIP197_ALG_DES_OFB			BIT(16)
-#define EIP197_ALG_DES_CFB			BIT(17)
-#define EIP197_ALG_3DES_ECB			BIT(18)
-#define EIP197_ALG_3DES_CBC			BIT(19)
-#define EIP197_ALG_3DES_OFB			BIT(21)
-#define EIP197_ALG_3DES_CFB			BIT(22)
-#define EIP197_ALG_MD5				BIT(24)
-#define EIP197_ALG_HMAC_MD5			BIT(25)
-#define EIP197_ALG_SHA1				BIT(26)
-#define EIP197_ALG_HMAC_SHA1			BIT(27)
-#define EIP197_ALG_SHA2				BIT(28)
-#define EIP197_ALG_HMAC_SHA2			BIT(29)
-#define EIP197_ALG_AES_XCBC_MAC			BIT(30)
-#define EIP197_ALG_GCM_HASH			BIT(31)
+#define EIP197_FUNCTION_ALL			0xffffffff
 
 /* EIP197_PE_EIP96_CONTEXT_CTRL */
 #define EIP197_CONTEXT_SIZE(n)			(n)
@@ -356,6 +328,7 @@ struct safexcel_context_record {
 /* control1 */
 #define CONTEXT_CONTROL_CRYPTO_MODE_ECB		(0 << 0)
 #define CONTEXT_CONTROL_CRYPTO_MODE_CBC		(1 << 0)
+#define CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD	(6 << 0)
 #define CONTEXT_CONTROL_IV0			BIT(5)
 #define CONTEXT_CONTROL_IV1			BIT(6)
 #define CONTEXT_CONTROL_IV2			BIT(7)
@@ -748,6 +721,7 @@ int safexcel_hmac_setkey(const char *alg, const u8 *key, unsigned int keylen,
 extern struct safexcel_alg_template safexcel_alg_cbc_des3_ede;
 extern struct safexcel_alg_template safexcel_alg_ecb_aes;
 extern struct safexcel_alg_template safexcel_alg_cbc_aes;
+extern struct safexcel_alg_template safexcel_alg_ctr_aes;
 extern struct safexcel_alg_template safexcel_alg_md5;
 extern struct safexcel_alg_template safexcel_alg_sha1;
 extern struct safexcel_alg_template safexcel_alg_sha224;
diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index 5eed890..91945b1 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -40,6 +40,7 @@ struct safexcel_cipher_ctx {
 	bool aead;
 
 	__le32 key[8];
+	u32 nonce;
 	unsigned int key_len;
 
 	/* All the below is AEAD specific */
@@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 				    u32 length)
 {
 	struct safexcel_token *token;
-	u32 offset = 0, block_sz = 0;
+	u32 block_sz = 0;
 
-	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
+	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
 		switch (ctx->alg) {
 		case SAFEXCEL_DES:
 			block_sz = DES_BLOCK_SIZE;
@@ -80,11 +81,20 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 			break;
 		}
 
-		offset = block_sz / sizeof(u32);
-		memcpy(cdesc->control_data.token, iv, block_sz);
+		if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) {
+			/* 32 bit nonce */
+			cdesc->control_data.token[0] = ctx->nonce;
+			/* 64 bit IV part */
+			memcpy(&cdesc->control_data.token[1], iv, 8);
+			/* 32 bit counter, start at 1 (big endian!) */
+			cdesc->control_data.token[3] = cpu_to_be32(1);
+		} else {
+			memcpy(cdesc->control_data.token, iv, block_sz);
+		}
 	}
 
-	token = (struct safexcel_token *)(cdesc->control_data.token + offset);
+	/* skip over worst case IV of 4 dwords, no need to be exact */
+	token = (struct safexcel_token *)(cdesc->control_data.token + 4);
 
 	token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
 	token[0].packet_length = length;
@@ -101,33 +111,35 @@ static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 				u32 cryptlen, u32 assoclen, u32 digestsize)
 {
 	struct safexcel_token *token;
-	unsigned offset = 0;
+	u32 block_sz = 0;
 
-	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
-		offset = AES_BLOCK_SIZE / sizeof(u32);
-		memcpy(cdesc->control_data.token, iv, AES_BLOCK_SIZE);
+	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
+		switch (ctx->alg) {
+		case SAFEXCEL_DES:
+			block_sz = DES_BLOCK_SIZE;
+			cdesc->control_data.options |= EIP197_OPTION_2_TOKEN_IV_CMD;
+			break;
+		case SAFEXCEL_3DES:
+			block_sz = DES3_EDE_BLOCK_SIZE;
+			cdesc->control_data.options |= EIP197_OPTION_2_TOKEN_IV_CMD;
+			break;
+		case SAFEXCEL_AES:
+			block_sz = AES_BLOCK_SIZE;
+			cdesc->control_data.options |= EIP197_OPTION_4_TOKEN_IV_CMD;
+			break;
+		}
 
-		cdesc->control_data.options |= EIP197_OPTION_4_TOKEN_IV_CMD;
+		memcpy(cdesc->control_data.token, iv, block_sz);
 	}
 
-	token = (struct safexcel_token *)(cdesc->control_data.token + offset);
-
 	if (direction == SAFEXCEL_DECRYPT)
 		cryptlen -= digestsize;
 
-	token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
-	token[0].packet_length = assoclen;
-	token[0].instructions = EIP197_TOKEN_INS_TYPE_HASH;
-
-	token[1].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
-	token[1].packet_length = cryptlen;
-	token[1].stat = EIP197_TOKEN_STAT_LAST_HASH;
-	token[1].instructions = EIP197_TOKEN_INS_LAST |
-				EIP197_TOKEN_INS_TYPE_CRYPTO |
-				EIP197_TOKEN_INS_TYPE_HASH |
-				EIP197_TOKEN_INS_TYPE_OUTPUT;
-
 	if (direction == SAFEXCEL_ENCRYPT) {
+		/* align end of instruction sequence to end of token */
+		token = (struct safexcel_token *)(cdesc->control_data.token +
+			 EIP197_MAX_TOKENS - 3);
+
 		token[2].opcode = EIP197_TOKEN_OPCODE_INSERT;
 		token[2].packet_length = digestsize;
 		token[2].stat = EIP197_TOKEN_STAT_LAST_HASH |
@@ -135,6 +147,10 @@ static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 		token[2].instructions = EIP197_TOKEN_INS_TYPE_OUTPUT |
 					EIP197_TOKEN_INS_INSERT_HASH_DIGEST;
 	} else {
+		/* align end of instruction sequence to end of token */
+		token = (struct safexcel_token *)(cdesc->control_data.token +
+			 EIP197_MAX_TOKENS - 4);
+
 		token[2].opcode = EIP197_TOKEN_OPCODE_RETRIEVE;
 		token[2].packet_length = digestsize;
 		token[2].stat = EIP197_TOKEN_STAT_LAST_HASH |
@@ -148,6 +164,19 @@ static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 				EIP197_TOKEN_STAT_LAST_PACKET;
 		token[3].instructions = EIP197_TOKEN_INS_TYPE_OUTPUT;
 	}
+
+	token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
+	token[0].packet_length = assoclen;
+	token[0].instructions = EIP197_TOKEN_INS_TYPE_HASH;
+
+	token[1].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
+	token[1].packet_length = cryptlen;
+	token[1].stat = EIP197_TOKEN_STAT_LAST_HASH;
+	token[1].instructions = EIP197_TOKEN_INS_LAST |
+				EIP197_TOKEN_INS_TYPE_CRYPTO |
+				EIP197_TOKEN_INS_TYPE_HASH |
+				EIP197_TOKEN_INS_TYPE_OUTPUT;
+
 }
 
 static int safexcel_skcipher_aes_setkey(struct crypto_skcipher *ctfm,
@@ -1044,6 +1073,84 @@ struct safexcel_alg_template safexcel_alg_cbc_aes = {
 	},
 };
 
+static int safexcel_ctr_aes_encrypt(struct skcipher_request *req)
+{
+	return safexcel_queue_req(&req->base, skcipher_request_ctx(req),
+			SAFEXCEL_ENCRYPT, CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD,
+			SAFEXCEL_AES);
+}
+
+static int safexcel_ctr_aes_decrypt(struct skcipher_request *req)
+{
+	return safexcel_queue_req(&req->base, skcipher_request_ctx(req),
+			SAFEXCEL_DECRYPT, CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD,
+			SAFEXCEL_AES);
+}
+
+static int safexcel_skcipher_aesctr_setkey(struct crypto_skcipher *ctfm,
+					   const u8 *key, unsigned int len)
+{
+	struct crypto_tfm *tfm = crypto_skcipher_tfm(ctfm);
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
+	struct safexcel_crypto_priv *priv = ctx->priv;
+	struct crypto_aes_ctx aes;
+	int ret, i;
+	unsigned int keylen;
+
+	/* last 4 bytes of key are the nonce! */
+	ctx->nonce = *(u32 *)(key + len - 4);
+	/* exclude the nonce here */
+	keylen = len - 4;
+	ret = crypto_aes_expand_key(&aes, key, keylen);
+	if (ret) {
+		crypto_skcipher_set_flags(ctfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+		return ret;
+	}
+
+	if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma) {
+		for (i = 0; i < keylen / sizeof(u32); i++) {
+			if (ctx->key[i] != cpu_to_le32(aes.key_enc[i])) {
+				ctx->base.needs_inv = true;
+				break;
+			}
+		}
+	}
+
+	for (i = 0; i < keylen / sizeof(u32); i++)
+		ctx->key[i] = cpu_to_le32(aes.key_enc[i]);
+
+	ctx->key_len = keylen;
+
+	memzero_explicit(&aes, sizeof(aes));
+	return 0;
+}
+
+struct safexcel_alg_template safexcel_alg_ctr_aes = {
+	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
+	.alg.skcipher = {
+		.setkey = safexcel_skcipher_aesctr_setkey,
+		.encrypt = safexcel_ctr_aes_encrypt,
+		.decrypt = safexcel_ctr_aes_decrypt,
+		/* Add 4 to include the 4 byte nonce! */
+		.min_keysize = AES_MIN_KEY_SIZE + 4,
+		.max_keysize = AES_MAX_KEY_SIZE + 4,
+		.ivsize = 8,
+		.base = {
+			.cra_name = "rfc3686(ctr(aes))",
+			.cra_driver_name = "safexcel-ctr-aes",
+			.cra_priority = 300,
+			.cra_flags = CRYPTO_ALG_ASYNC |
+				     CRYPTO_ALG_KERN_DRIVER_ONLY,
+			.cra_blocksize = 1,
+			.cra_ctxsize = sizeof(struct safexcel_cipher_ctx),
+			.cra_alignmask = 0,
+			.cra_init = safexcel_skcipher_cra_init,
+			.cra_exit = safexcel_skcipher_cra_exit,
+			.cra_module = THIS_MODULE,
+		},
+	},
+};
+
 static int safexcel_cbc_des_encrypt(struct skcipher_request *req)
 {
 	return safexcel_queue_req(&req->base, skcipher_request_ctx(req),
-- 
1.8.3.1

[PATCH 3/3] crypto: inside-secure - add support for authenc(hmac(sha*),rfc3686(ctr(aes))) suites

[Pascal van Leeuwen]

This patch adds support for the following AEAD ciphersuites:
- authenc(hmac(sha1),rfc3686(ctr(aes)))
- authenc(hmac(sha224),rfc3686(ctr(aes)))
- authenc(hmac(sha256),rfc3686(ctr(aes)))
- authenc(hmac(sha384),rfc3686(ctr(aes)))
- authenc(hmac(sha512),rfc3686(ctr(aes)))

Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 drivers/crypto/inside-secure/safexcel.c        |  14 +-
 drivers/crypto/inside-secure/safexcel.h        |   5 +
 drivers/crypto/inside-secure/safexcel_cipher.c | 276 +++++++++++++++++++++----
 3 files changed, 250 insertions(+), 45 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index 26f086b..ca84119 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -697,17 +697,18 @@ inline int safexcel_rdesc_check_errors(struct safexcel_crypto_priv *priv,
 	if (rdesc->buffer_overflow)
 		dev_err(priv->dev, "Buffer overflow detected");
 
-	if (rdesc->result_data.error_code & 0x4067) {
-		/* Fatal error (bits 0,1,2,5,6 & 14) */
+	if (rdesc->result_data.error_code & 0x4066) {
+		/* Fatal error (bits 1,2,5,6 & 14) */
 		dev_err(priv->dev,
 			"result descriptor error (%x)",
 			rdesc->result_data.error_code);
 		return -EIO;
 	} else if (rdesc->result_data.error_code &
-		   (BIT(7) | BIT(4) | BIT(3))) {
+		   (BIT(7) | BIT(4) | BIT(3) | BIT(0))) {
 		/*
 		 * Give priority over authentication fails:
-		 * Blocksize & overflow errors, something wrong with the input!
+		 * Blocksize, length & overflow errors,
+		 * something wrong with the input!
 		 */
 		return -EINVAL;
 	} else if (rdesc->result_data.error_code & BIT(9)) {
@@ -998,6 +999,11 @@ static int safexcel_request_ring_irq(void *pdev, int irqid,
 	&safexcel_alg_authenc_hmac_sha384_cbc_aes,
 	&safexcel_alg_authenc_hmac_sha512_cbc_aes,
 	&safexcel_alg_authenc_hmac_sha1_cbc_des3_ede,
+	&safexcel_alg_authenc_hmac_sha1_ctr_aes,
+	&safexcel_alg_authenc_hmac_sha224_ctr_aes,
+	&safexcel_alg_authenc_hmac_sha256_ctr_aes,
+	&safexcel_alg_authenc_hmac_sha384_ctr_aes,
+	&safexcel_alg_authenc_hmac_sha512_ctr_aes,
 };
 
 static int safexcel_register_algorithms(struct safexcel_crypto_priv *priv)
diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h
index af71120..36657c3 100644
--- a/drivers/crypto/inside-secure/safexcel.h
+++ b/drivers/crypto/inside-secure/safexcel.h
@@ -740,5 +740,10 @@ int safexcel_hmac_setkey(const char *alg, const u8 *key, unsigned int keylen,
 extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_cbc_aes;
 extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha512_cbc_aes;
 extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede;
+extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_ctr_aes;
+extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha224_ctr_aes;
+extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha256_ctr_aes;
+extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_ctr_aes;
+extern struct safexcel_alg_template safexcel_alg_authenc_hmac_sha512_ctr_aes;
 
 #endif
diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index 91945b1..1783483 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -58,11 +58,9 @@ struct safexcel_cipher_req {
 	int  nr_src, nr_dst;
 };
 
-static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
-				    struct safexcel_command_desc *cdesc,
-				    u32 length)
+static void safexcel_cipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
+				  struct safexcel_command_desc *cdesc)
 {
-	struct safexcel_token *token;
 	u32 block_sz = 0;
 
 	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
@@ -92,6 +90,15 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 			memcpy(cdesc->control_data.token, iv, block_sz);
 		}
 	}
+}
+
+static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
+				    struct safexcel_command_desc *cdesc,
+				    u32 length)
+{
+	struct safexcel_token *token;
+
+	safexcel_cipher_token(ctx, iv, cdesc);
 
 	/* skip over worst case IV of 4 dwords, no need to be exact */
 	token = (struct safexcel_token *)(cdesc->control_data.token + 4);
@@ -111,26 +118,8 @@ static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 				u32 cryptlen, u32 assoclen, u32 digestsize)
 {
 	struct safexcel_token *token;
-	u32 block_sz = 0;
 
-	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
-		switch (ctx->alg) {
-		case SAFEXCEL_DES:
-			block_sz = DES_BLOCK_SIZE;
-			cdesc->control_data.options |= EIP197_OPTION_2_TOKEN_IV_CMD;
-			break;
-		case SAFEXCEL_3DES:
-			block_sz = DES3_EDE_BLOCK_SIZE;
-			cdesc->control_data.options |= EIP197_OPTION_2_TOKEN_IV_CMD;
-			break;
-		case SAFEXCEL_AES:
-			block_sz = AES_BLOCK_SIZE;
-			cdesc->control_data.options |= EIP197_OPTION_4_TOKEN_IV_CMD;
-			break;
-		}
-
-		memcpy(cdesc->control_data.token, iv, block_sz);
-	}
+	safexcel_cipher_token(ctx, iv, cdesc);
 
 	if (direction == SAFEXCEL_DECRYPT)
 		cryptlen -= digestsize;
@@ -165,18 +154,27 @@ static void safexcel_aead_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
 		token[3].instructions = EIP197_TOKEN_INS_TYPE_OUTPUT;
 	}
 
-	token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
-	token[0].packet_length = assoclen;
-	token[0].instructions = EIP197_TOKEN_INS_TYPE_HASH;
-
-	token[1].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
-	token[1].packet_length = cryptlen;
-	token[1].stat = EIP197_TOKEN_STAT_LAST_HASH;
-	token[1].instructions = EIP197_TOKEN_INS_LAST |
-				EIP197_TOKEN_INS_TYPE_CRYPTO |
-				EIP197_TOKEN_INS_TYPE_HASH |
-				EIP197_TOKEN_INS_TYPE_OUTPUT;
+	if (unlikely(!cryptlen)) {
+		token[1].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
+		token[1].packet_length = assoclen;
+		token[1].stat = EIP197_TOKEN_STAT_LAST_HASH;
+		token[1].instructions = EIP197_TOKEN_INS_LAST |
+					EIP197_TOKEN_INS_TYPE_HASH;
+	} else {
+		if (likely(assoclen)) {
+			token[0].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
+			token[0].packet_length = assoclen;
+			token[0].instructions = EIP197_TOKEN_INS_TYPE_HASH;
+		}
 
+		token[1].opcode = EIP197_TOKEN_OPCODE_DIRECTION;
+		token[1].packet_length = cryptlen;
+		token[1].stat = EIP197_TOKEN_STAT_LAST_HASH;
+		token[1].instructions = EIP197_TOKEN_INS_LAST |
+					EIP197_TOKEN_INS_TYPE_CRYPTO |
+					EIP197_TOKEN_INS_TYPE_HASH |
+					EIP197_TOKEN_INS_TYPE_OUTPUT;
+	}
 }
 
 static int safexcel_skcipher_aes_setkey(struct crypto_skcipher *ctfm,
@@ -220,23 +218,43 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
 	struct safexcel_ahash_export_state istate, ostate;
 	struct safexcel_crypto_priv *priv = ctx->priv;
 	struct crypto_authenc_keys keys;
+	struct crypto_aes_ctx aes;
 	u32 flags;
-	int err;
+	int err = -EINVAL;
 
 	if (crypto_authenc_extractkeys(&keys, key, len) != 0)
 		goto badkey;
 
-	if (keys.enckeylen > sizeof(ctx->key))
-		goto badkey;
+	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) {
+		/* 20 is minimum AES key: 16 bytes + 4 bytes nonce */
+		if (keys.enckeylen < 20)
+			goto badkey;
+		/* last 4 bytes of key are the nonce! */
+		ctx->nonce = *(u32 *)(keys.enckey + keys.enckeylen - 4);
+		/* exclude the nonce here */
+		keys.enckeylen -= 4;
+	}
 
 	/* Encryption key */
-	if (ctx->alg == SAFEXCEL_3DES) {
+	switch (ctx->alg) {
+	case SAFEXCEL_3DES:
+		if (keys.enckeylen != 24)
+			goto badkey;
 		flags = crypto_aead_get_flags(ctfm);
 		err = __des3_verify_key(&flags, keys.enckey);
 		crypto_aead_set_flags(ctfm, flags);
 
 		if (unlikely(err))
-			return err;
+			goto badkey_expflags;
+		break;
+	case SAFEXCEL_AES:
+		err = crypto_aes_expand_key(&aes, keys.enckey, keys.enckeylen);
+		if (unlikely(err))
+			goto badkey;
+		break;
+	default:
+		dev_err(priv->dev, "aead: unsupported cipher algorithm\n");
+		goto badkey;
 	}
 
 	if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma &&
@@ -295,8 +313,9 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
 
 badkey:
 	crypto_aead_set_flags(ctfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
+badkey_expflags:
 	memzero_explicit(&keys, sizeof(keys));
-	return -EINVAL;
+	return err;
 }
 
 static int safexcel_context_control(struct safexcel_cipher_ctx *ctx,
@@ -1386,6 +1405,7 @@ static int safexcel_aead_cra_init(struct crypto_tfm *tfm)
 
 	ctx->priv = tmpl->priv;
 
+	ctx->alg  = SAFEXCEL_AES; /* default */
 	ctx->aead = true;
 	ctx->base.send = safexcel_aead_send;
 	ctx->base.handle_result = safexcel_aead_handle_result;
@@ -1562,6 +1582,15 @@ struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_cbc_aes = {
 	},
 };
 
+static int safexcel_aead_sha1_des3_cra_init(struct crypto_tfm *tfm)
+{
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
+
+	safexcel_aead_sha1_cra_init(tfm);
+	ctx->alg = SAFEXCEL_3DES; /* override default */
+	return 0;
+}
+
 static int safexcel_aead_encrypt_3des(struct aead_request *req)
 {
 	struct safexcel_cipher_req *creq = aead_request_ctx(req);
@@ -1595,7 +1624,172 @@ struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = {
 			.cra_blocksize = DES3_EDE_BLOCK_SIZE,
 			.cra_ctxsize = sizeof(struct safexcel_cipher_ctx),
 			.cra_alignmask = 0,
-			.cra_init = safexcel_aead_sha1_cra_init,
+			.cra_init = safexcel_aead_sha1_des3_cra_init,
+			.cra_exit = safexcel_aead_cra_exit,
+			.cra_module = THIS_MODULE,
+		},
+	},
+};
+
+static int safexcel_aead_sha1_ctr_cra_init(struct crypto_tfm *tfm)
+{
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
+
+	safexcel_aead_sha1_cra_init(tfm);
+	ctx->mode = CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD; /* override default */
+	return 0;
+}
+
+struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_ctr_aes = {
+	.type = SAFEXCEL_ALG_TYPE_AEAD,
+	.alg.aead = {
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
+		.ivsize = 8,
+		.maxauthsize = SHA1_DIGEST_SIZE,
+		.base = {
+			.cra_name = "authenc(hmac(sha1),rfc3686(ctr(aes)))",
+			.cra_driver_name = "safexcel-authenc-hmac-sha1-ctr-aes",
+			.cra_priority = 300,
+			.cra_flags = CRYPTO_ALG_ASYNC |
+				     CRYPTO_ALG_KERN_DRIVER_ONLY,
+			.cra_blocksize = 1,
+			.cra_ctxsize = sizeof(struct safexcel_cipher_ctx),
+			.cra_alignmask = 0,
+			.cra_init = safexcel_aead_sha1_ctr_cra_init,
+			.cra_exit = safexcel_aead_cra_exit,
+			.cra_module = THIS_MODULE,
+		},
+	},
+};
+
+static int safexcel_aead_sha256_ctr_cra_init(struct crypto_tfm *tfm)
+{
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
+
+	safexcel_aead_sha256_cra_init(tfm);
+	ctx->mode = CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD; /* override default */
+	return 0;
+}
+
+struct safexcel_alg_template safexcel_alg_authenc_hmac_sha256_ctr_aes = {
+	.type = SAFEXCEL_ALG_TYPE_AEAD,
+	.alg.aead = {
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
+		.ivsize = 8,
+		.maxauthsize = SHA256_DIGEST_SIZE,
+		.base = {
+			.cra_name = "authenc(hmac(sha256),rfc3686(ctr(aes)))",
+			.cra_driver_name = "safexcel-authenc-hmac-sha256-ctr-aes",
+			.cra_priority = 300,
+			.cra_flags = CRYPTO_ALG_ASYNC |
+				     CRYPTO_ALG_KERN_DRIVER_ONLY,
+			.cra_blocksize = 1,
+			.cra_ctxsize = sizeof(struct safexcel_cipher_ctx),
+			.cra_alignmask = 0,
+			.cra_init = safexcel_aead_sha256_ctr_cra_init,
+			.cra_exit = safexcel_aead_cra_exit,
+			.cra_module = THIS_MODULE,
+		},
+	},
+};
+
+static int safexcel_aead_sha224_ctr_cra_init(struct crypto_tfm *tfm)
+{
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
+
+	safexcel_aead_sha224_cra_init(tfm);
+	ctx->mode = CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD; /* override default */
+	return 0;
+}
+
+struct safexcel_alg_template safexcel_alg_authenc_hmac_sha224_ctr_aes = {
+	.type = SAFEXCEL_ALG_TYPE_AEAD,
+	.alg.aead = {
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
+		.ivsize = 8,
+		.maxauthsize = SHA224_DIGEST_SIZE,
+		.base = {
+			.cra_name = "authenc(hmac(sha224),rfc3686(ctr(aes)))",
+			.cra_driver_name = "safexcel-authenc-hmac-sha224-ctr-aes",
+			.cra_priority = 300,
+			.cra_flags = CRYPTO_ALG_ASYNC |
+				     CRYPTO_ALG_KERN_DRIVER_ONLY,
+			.cra_blocksize = 1,
+			.cra_ctxsize = sizeof(struct safexcel_cipher_ctx),
+			.cra_alignmask = 0,
+			.cra_init = safexcel_aead_sha224_ctr_cra_init,
+			.cra_exit = safexcel_aead_cra_exit,
+			.cra_module = THIS_MODULE,
+		},
+	},
+};
+
+static int safexcel_aead_sha512_ctr_cra_init(struct crypto_tfm *tfm)
+{
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
+
+	safexcel_aead_sha512_cra_init(tfm);
+	ctx->mode = CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD; /* override default */
+	return 0;
+}
+
+struct safexcel_alg_template safexcel_alg_authenc_hmac_sha512_ctr_aes = {
+	.type = SAFEXCEL_ALG_TYPE_AEAD,
+	.alg.aead = {
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
+		.ivsize = 8,
+		.maxauthsize = SHA512_DIGEST_SIZE,
+		.base = {
+			.cra_name = "authenc(hmac(sha512),rfc3686(ctr(aes)))",
+			.cra_driver_name = "safexcel-authenc-hmac-sha512-ctr-aes",
+			.cra_priority = 300,
+			.cra_flags = CRYPTO_ALG_ASYNC |
+				     CRYPTO_ALG_KERN_DRIVER_ONLY,
+			.cra_blocksize = 1,
+			.cra_ctxsize = sizeof(struct safexcel_cipher_ctx),
+			.cra_alignmask = 0,
+			.cra_init = safexcel_aead_sha512_ctr_cra_init,
+			.cra_exit = safexcel_aead_cra_exit,
+			.cra_module = THIS_MODULE,
+		},
+	},
+};
+
+static int safexcel_aead_sha384_ctr_cra_init(struct crypto_tfm *tfm)
+{
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
+
+	safexcel_aead_sha384_cra_init(tfm);
+	ctx->mode = CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD; /* override default */
+	return 0;
+}
+
+struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_ctr_aes = {
+	.type = SAFEXCEL_ALG_TYPE_AEAD,
+	.alg.aead = {
+		.setkey = safexcel_aead_setkey,
+		.encrypt = safexcel_aead_encrypt_aes,
+		.decrypt = safexcel_aead_decrypt_aes,
+		.ivsize = 8,
+		.maxauthsize = SHA384_DIGEST_SIZE,
+		.base = {
+			.cra_name = "authenc(hmac(sha384),rfc3686(ctr(aes)))",
+			.cra_driver_name = "safexcel-authenc-hmac-sha384-ctr-aes",
+			.cra_priority = 300,
+			.cra_flags = CRYPTO_ALG_ASYNC |
+				     CRYPTO_ALG_KERN_DRIVER_ONLY,
+			.cra_blocksize = 1,
+			.cra_ctxsize = sizeof(struct safexcel_cipher_ctx),
+			.cra_alignmask = 0,
+			.cra_init = safexcel_aead_sha384_ctr_cra_init,
 			.cra_exit = safexcel_aead_cra_exit,
 			.cra_module = THIS_MODULE,
 		},
-- 
1.8.3.1

Re: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))

[Antoine Tenart]

Hi Pascal,

On Fri, Jul 05, 2019 at 08:49:22AM +0200, Pascal van Leeuwen wrote:
> Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>

Could you provide a commit message, explaining briefly what the patch is
doing?

> @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key,
>  		goto badkey;
>  
>  	/* Encryption key */
> +	if (ctx->alg == SAFEXCEL_3DES) {
> +		flags = crypto_aead_get_flags(ctfm);
> +		err = __des3_verify_key(&flags, keys.enckey);
> +		crypto_aead_set_flags(ctfm, flags);

You could use directly des3_verify_key() which does exactly this.

> +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = {
> +	.type = SAFEXCEL_ALG_TYPE_AEAD,

You either missed to fill .engines member of this struct, or this series
is based on another one not merged yet.

> +	.alg.aead = {
> +		.setkey = safexcel_aead_setkey,
> +		.encrypt = safexcel_aead_encrypt_3des,
> +		.decrypt = safexcel_aead_decrypt_3des,
> +		.ivsize = DES3_EDE_BLOCK_SIZE,
> +		.maxauthsize = SHA1_DIGEST_SIZE,
> +		.base = {
> +			.cra_name = "authenc(hmac(sha1),cbc(des3_ede))",
> +			.cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede",

You could drop "_ede" here, or s/_/-/.

Apart from those small comments, the patch looks good.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/3] crypto: inside-secure - add more AEAD ciphersuites

[Herbert Xu]

Pascal van Leeuwen <pascalvanl@gmail.com> wrote:
> This patch set adds support for the following additional AEAD suites:
> - authenc(hmac(sha1),cbc(des3_ede))
> - authenc(hmac(sha1),rfc3686(ctr(aes)))
> - authenc(hmac(sha224),rfc3686(ctr(aes)))
> - authenc(hmac(sha256),rfc3686(ctr(aes)))
> - authenc(hmac(sha384),rfc3686(ctr(aes)))
> - authenc(hmac(sha512),rfc3686(ctr(aes)))
> 
> It has been verified on an FPGA devboard and Macchiatobin (Armada 8K)
> 
> Pascal van Leeuwen (3):
>  crypto: inside-secure - add support for
>    authenc(hmac(sha1),cbc(des3_ede))
>  crypto: inside-secure - added support for rfc3686(ctr(aes))
>  crypto: inside-secure - add support for
>    authenc(hmac(sha*),rfc3686(ctr(aes))) suites
> 
> drivers/crypto/inside-secure/safexcel.c        |  30 +-
> drivers/crypto/inside-secure/safexcel.h        |  38 +--
> drivers/crypto/inside-secure/safexcel_cipher.c | 456 ++++++++++++++++++++++---
> 3 files changed, 428 insertions(+), 96 deletions(-)

All 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

Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Antoine Tenart]

Hi Pascal,

On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>

Could you add a commit message?

> -		/* H/W capabilities selection */
> -		val = EIP197_FUNCTION_RSVD;
> -		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
> -		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
> -		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
> -		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
> -		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
> -		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
> -		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
> -		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
> -		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> +		/* H/W capabilities selection: just enable everything */
> +		writel(EIP197_FUNCTION_ALL,
> +		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));

This should be in a separate patch. I'm also not sure about it, as
controlling exactly what algs are enabled in the h/w could prevent
misconfiguration issues in the control descriptors.

> @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
>  				    u32 length)
> -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {

I think it's better for maintenance and readability to have something
like:

  if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
      ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)

> +struct safexcel_alg_template safexcel_alg_ctr_aes = {
> +	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,

Same comment as in patch 1 about the .engines member of the struct.

> +	.alg.skcipher = {
> +		.setkey = safexcel_skcipher_aesctr_setkey,
> +		.encrypt = safexcel_ctr_aes_encrypt,
> +		.decrypt = safexcel_ctr_aes_decrypt,
> +		/* Add 4 to include the 4 byte nonce! */
> +		.min_keysize = AES_MIN_KEY_SIZE + 4,
> +		.max_keysize = AES_MAX_KEY_SIZE + 4,

You could use CTR_RFC3686_NONCE_SIZE here (maybe in other places in the
patch as well).

> +		.ivsize = 8,

And CTR_RFC3686_IV_SIZE here.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))

[Pascal Van Leeuwen]

Antoine,

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Antoine Tenart
> Sent: Friday, July 26, 2019 2:20 PM
> To: Pascal van Leeuwen <pascalvanl@gmail.com>
> Cc: linux-crypto@vger.kernel.org; antoine.tenart@bootlin.com; herbert@gondor.apana.org.au; davem@davemloft.net; Pascal Van
> Leeuwen <pvanleeuwen@verimatrix.com>
> Subject: Re: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))
> 
> Hi Pascal,
> 
> On Fri, Jul 05, 2019 at 08:49:22AM +0200, Pascal van Leeuwen wrote:
> > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
> 
> Could you provide a commit message, explaining briefly what the patch is
> doing?
> 
I initially figured that to be redundant if the subject already covered it completely.
But now that I think of it, it's possible the subject does not end up in the commit
at all ... if that is the case, would it work if I just copy-paste the relevant part of the
subject message? Or do I need to be more verbose?

> > @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key,
> >  		goto badkey;
> >
> >  	/* Encryption key */
> > +	if (ctx->alg == SAFEXCEL_3DES) {
> > +		flags = crypto_aead_get_flags(ctfm);
> > +		err = __des3_verify_key(&flags, keys.enckey);
> > +		crypto_aead_set_flags(ctfm, flags);
> 
> You could use directly des3_verify_key() which does exactly this.
> 
Actually, I couldn't due to des3_verify_key expecting a struct crypto_skcipher as input,
and not a struct crypto_aead, that's why I had to do it this way ...

> > +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = {
> > +	.type = SAFEXCEL_ALG_TYPE_AEAD,
> 
> You either missed to fill .engines member of this struct, or this series
> is based on another one not merged yet.
> 
Yes, that happened in the patchset of which v2 did not make it to the mailing list ...

> > +	.alg.aead = {
> > +		.setkey = safexcel_aead_setkey,
> > +		.encrypt = safexcel_aead_encrypt_3des,
> > +		.decrypt = safexcel_aead_decrypt_3des,
> > +		.ivsize = DES3_EDE_BLOCK_SIZE,
> > +		.maxauthsize = SHA1_DIGEST_SIZE,
> > +		.base = {
> > +			.cra_name = "authenc(hmac(sha1),cbc(des3_ede))",
> > +			.cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede",
> 
> You could drop "_ede" here, or s/_/-/.
> 
Agree the underscore should not be there.
Our HW does not support any other form of 3DES so EDE doesn't
really add much here, therefore I will just remove "_ede" entirely.

> Apart from those small comments, the patch looks good.
> 
> Thanks!
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))

[Antoine Tenart]

Hi Pascal,

On Fri, Jul 26, 2019 at 12:57:21PM +0000, Pascal Van Leeuwen wrote:
> > On Fri, Jul 05, 2019 at 08:49:22AM +0200, Pascal van Leeuwen wrote:
> > > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
> > 
> > Could you provide a commit message, explaining briefly what the patch is
> > doing?
> > 
> I initially figured that to be redundant if the subject already covered it completely.
> But now that I think of it, it's possible the subject does not end up in the commit
> at all ... if that is the case, would it work if I just copy-paste the relevant part of the
> subject message? Or do I need to be more verbose?

The subject will be the commit title. I know sometimes the commit
message is trivial or redundant, but it's still a good practice to
always have one (and many maintainers will ask for one). Even if it's
only two lines :)

> > > @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key,
> > >  		goto badkey;
> > >
> > >  	/* Encryption key */
> > > +	if (ctx->alg == SAFEXCEL_3DES) {
> > > +		flags = crypto_aead_get_flags(ctfm);
> > > +		err = __des3_verify_key(&flags, keys.enckey);
> > > +		crypto_aead_set_flags(ctfm, flags);
> > 
> > You could use directly des3_verify_key() which does exactly this.
> > 
> Actually, I couldn't due to des3_verify_key expecting a struct crypto_skcipher as input,
> and not a struct crypto_aead, that's why I had to do it this way ...

I see. Maybe a good way would be to provide a function taking
'struct crypto_aead' as an argument so that not every single driver
reimplement the same logic. But this can come later if needed.

> > > +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = {
> > > +	.type = SAFEXCEL_ALG_TYPE_AEAD,
> > 
> > You either missed to fill .engines member of this struct, or this series
> > is based on another one not merged yet.
> > 
> Yes, that happened in the patchset of which v2 did not make it to the mailing list ...

:)

So in general if there's a dependency you should say so in the cover
letter.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Pascal Van Leeuwen]

Antoine,

> Hi Pascal,
> 
> On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
> 
> Could you add a commit message?
> 
I can do that. Just didn't know it was really needed.

> > -		/* H/W capabilities selection */
> > -		val = EIP197_FUNCTION_RSVD;
> > -		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
> > -		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
> > -		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
> > -		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
> > -		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
> > -		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
> > -		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
> > -		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
> > -		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> > +		/* H/W capabilities selection: just enable everything */
> > +		writel(EIP197_FUNCTION_ALL,
> > +		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> 
> This should be in a separate patch.
>
It was added specifically to  get this functionality working as CTR mode was not
enabled.  So I don't see why it should be a seperate patch, really?
Instead of adding CTR mode to the list (which would have been perfectly valid
in this context anyway), I just chose to enable everything instead.

> I'm also not sure about it, as
> controlling exactly what algs are enabled in the h/w could prevent
> misconfiguration issues in the control descriptors.
> 
That's not really what it's supposed to be used for. It's supposed to be used for
disabling algorithms the application is not ALLOWED to use e.g. because they are
not deemed secure enough (in case you have to comply, with,  say, FIPS or so).

As for misconfiguration: you may just as well hit another enabled algorithm, the
odds of which will increase as I add more. And I'm very busy adding ALL of them,
by which time you wouldn't want to disable any anyway.

> > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> >  				    u32 length)
> > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> 
> I think it's better for maintenance and readability to have something
> like:
> 
>   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
>       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> 
Not really. I *really* want to execute that for any mode other than ECB,
ECB being the *only* mode that does not require an IV (which I know
for a fact, being the architect and all :-).
And I don't believe a long list of modes that *do* require an IV would 
be  more readable or easy to maintain than this single compare ...

> > +struct safexcel_alg_template safexcel_alg_ctr_aes = {
> > +	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
> 
> Same comment as in patch 1 about the .engines member of the struct.
> 
Same answer: depends on the disappearing patch I will resend shortly.

> > +	.alg.skcipher = {
> > +		.setkey = safexcel_skcipher_aesctr_setkey,
> > +		.encrypt = safexcel_ctr_aes_encrypt,
> > +		.decrypt = safexcel_ctr_aes_decrypt,
> > +		/* Add 4 to include the 4 byte nonce! */
> > +		.min_keysize = AES_MIN_KEY_SIZE + 4,
> > +		.max_keysize = AES_MAX_KEY_SIZE + 4,
> 
> You could use CTR_RFC3686_NONCE_SIZE here (maybe in other places in the
> patch as well).
> 
Makes sense. I did not know such a constant existed already.

> > +		.ivsize = 8,
> 
> And CTR_RFC3686_IV_SIZE here.
> 
Idem

> Thanks!
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

RE: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))

[Pascal Van Leeuwen]

Antoine,

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Antoine Tenart
> Sent: Friday, July 26, 2019 3:07 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org;
> herbert@gondor.apana.org.au; davem@davemloft.net
> Subject: Re: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))
> 
> Hi Pascal,
> 
> On Fri, Jul 26, 2019 at 12:57:21PM +0000, Pascal Van Leeuwen wrote:
> > > On Fri, Jul 05, 2019 at 08:49:22AM +0200, Pascal van Leeuwen wrote:
> > > > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
> > >
> > > Could you provide a commit message, explaining briefly what the patch is
> > > doing?
> > >
> > I initially figured that to be redundant if the subject already covered it completely.
> > But now that I think of it, it's possible the subject does not end up in the commit
> > at all ... if that is the case, would it work if I just copy-paste the relevant part of the
> > subject message? Or do I need to be more verbose?
> 
> The subject will be the commit title. I know sometimes the commit
> message is trivial or redundant, but it's still a good practice to
> always have one (and many maintainers will ask for one). Even if it's
> only two lines :)
> 
Ok, good to know. I'm still learning how this works. I'll try and remember ;-)

> > > > @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key,
> > > >  		goto badkey;
> > > >
> > > >  	/* Encryption key */
> > > > +	if (ctx->alg == SAFEXCEL_3DES) {
> > > > +		flags = crypto_aead_get_flags(ctfm);
> > > > +		err = __des3_verify_key(&flags, keys.enckey);
> > > > +		crypto_aead_set_flags(ctfm, flags);
> > >
> > > You could use directly des3_verify_key() which does exactly this.
> > >
> > Actually, I couldn't due to des3_verify_key expecting a struct crypto_skcipher as input,
> > and not a struct crypto_aead, that's why I had to do it this way ...
> 
> I see. Maybe a good way would be to provide a function taking
> 'struct crypto_aead' as an argument so that not every single driver
> reimplement the same logic. But this can come later if needed.
> 
Agree. But being a newby and all, I did not dare to touch des.h itself ...

> > > > +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = {
> > > > +	.type = SAFEXCEL_ALG_TYPE_AEAD,
> > >
> > > You either missed to fill .engines member of this struct, or this series
> > > is based on another one not merged yet.
> > >
> > Yes, that happened in the patchset of which v2 did not make it to the mailing list ...
> 
> :)
> 
> So in general if there's a dependency you should say so in the cover
> letter.
> 
I'll try to do that insofar I actually realise such a dependency exists ...

> Thanks!
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Antoine Tenart]

Hi Pascal,

On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> 
> > > -		/* H/W capabilities selection */
> > > -		val = EIP197_FUNCTION_RSVD;
> > > -		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
> > > -		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
> > > -		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
> > > -		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
> > > -		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
> > > -		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
> > > -		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
> > > -		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
> > > -		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> > > +		/* H/W capabilities selection: just enable everything */
> > > +		writel(EIP197_FUNCTION_ALL,
> > > +		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> > 
> > This should be in a separate patch.
> >
> It was added specifically to  get this functionality working as CTR mode was not
> enabled.  So I don't see why it should be a seperate patch, really?
> Instead of adding CTR mode to the list (which would have been perfectly valid
> in this context anyway), I just chose to enable everything instead.

Because it's not the same thing to enable everything and to add one extra
alg. This makes bissecting the driver harder because one would not except
this change in this kind of patch.

> > I'm also not sure about it, as
> > controlling exactly what algs are enabled in the h/w could prevent
> > misconfiguration issues in the control descriptors.
> > 
> That's not really what it's supposed to be used for. It's supposed to be used for
> disabling algorithms the application is not ALLOWED to use e.g. because they are
> not deemed secure enough (in case you have to comply, with,  say, FIPS or so).

OK, that answer my question and this makes sense.

> > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> > >  				    u32 length)
> > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > 
> > I think it's better for maintenance and readability to have something
> > like:
> > 
> >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > 
> Not really. I *really* want to execute that for any mode other than ECB,
> ECB being the *only* mode that does not require an IV (which I know
> for a fact, being the architect and all :-).
> And I don't believe a long list of modes that *do* require an IV would 
> be  more readable or easy to maintain than this single compare ...

That's where I disagree as you need extra knowledge to be aware of this.
Being explicit removes any possible question one may ask. But that's a
small point really :)

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Pascal Van Leeuwen]

Antoine,

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Antoine Tenart
> Sent: Friday, July 26, 2019 3:47 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org;
> herbert@gondor.apana.org.au; davem@davemloft.net
> Subject: Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))
> 
> Hi Pascal,
> 
> On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> >
> > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> > > >  				    u32 length)
> > > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > >
> > > I think it's better for maintenance and readability to have something
> > > like:
> > >
> > >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> > >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > >
> > Not really. I *really* want to execute that for any mode other than ECB,
> > ECB being the *only* mode that does not require an IV (which I know
> > for a fact, being the architect and all :-).
> > And I don't believe a long list of modes that *do* require an IV would
> > be  more readable or easy to maintain than this single compare ...
> 
> That's where I disagree as you need extra knowledge to be aware of this.
> Being explicit removes any possible question one may ask. But that's a
> small point really :)
> 
Well, while we're disagreeing ... I disagree with your assertion that you
would need more knowledge to know which modes do NOT need an IV
than to know which modes DO need an IV. There's really no fundamental
difference, it's two sides of the exact same coin ... you need that
knowledge either way. 

That being said:

1) This code is executed for each individual cipher call, i.e. it's in the
critical path. Having just 1 compare-and-branch there is better for
performance than having many.

2) Generally, all else being equal, having less code is easier to maintain
than having more code. 

3) Stuffing an IV in the token while you don't need it is harmless (apart
from wasting some cycles). NOT stuffing an IV in the token while you DO
need it is fatal. i.e. the single compare always errs on the safe side ...

4) If there is anything unclear about an otherwise fine code construct,
then you clarify it by adding a comment, not by rewriting it to be inefficient
and redundant ;-)

> Thanks!
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Antoine Tenart]

On Fri, Jul 26, 2019 at 02:29:48PM +0000, Pascal Van Leeuwen wrote:
> > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Antoine Tenart
> > On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> > >
> > > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> > > > >  				    u32 length)
> > > > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > > >
> > > > I think it's better for maintenance and readability to have something
> > > > like:
> > > >
> > > >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> > > >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > > >
> > > Not really. I *really* want to execute that for any mode other than ECB,
> > > ECB being the *only* mode that does not require an IV (which I know
> > > for a fact, being the architect and all :-).
> > > And I don't believe a long list of modes that *do* require an IV would
> > > be  more readable or easy to maintain than this single compare ...
> > 
> > That's where I disagree as you need extra knowledge to be aware of this.
> > Being explicit removes any possible question one may ask. But that's a
> > small point really :)
> > 
> Well, while we're disagreeing ... I disagree with your assertion that you
> would need more knowledge to know which modes do NOT need an IV
> than to know which modes DO need an IV. There's really no fundamental
> difference, it's two sides of the exact same coin ... you need that
> knowledge either way. 

The point is if you look for occurrences of, let's say
CONTEXT_CONTROL_CRYPTO_MODE_CBC, to see the code path it'll be way
easier if you have direct comparisons.

> 1) This code is executed for each individual cipher call, i.e. it's in the
> critical path. Having just 1 compare-and-branch there is better for
> performance than having many.

Not sure about what the impact really is.

> 2) Generally, all else being equal, having less code is easier to maintain
> than having more code. 

That really depends, having readable code is easier to maintain :)

> 4) If there is anything unclear about an otherwise fine code construct,
> then you clarify it by adding a comment, not by rewriting it to be inefficient
> and redundant ;-)

Fair point.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Tuesday, July 30, 2019 10:24 AM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen
> <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au;
> davem@davemloft.net
> Subject: Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))
> 
> On Fri, Jul 26, 2019 at 02:29:48PM +0000, Pascal Van Leeuwen wrote:
> > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On
> Behalf Of Antoine Tenart
> > > On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > > > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> > > >
> > > > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx
> *ctx, u8 *iv,
> > > > > >  				    u32 length)
> > > > > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > > > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > > > >
> > > > > I think it's better for maintenance and readability to have something
> > > > > like:
> > > > >
> > > > >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> > > > >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > > > >
> > > > Not really. I *really* want to execute that for any mode other than ECB,
> > > > ECB being the *only* mode that does not require an IV (which I know
> > > > for a fact, being the architect and all :-).
> > > > And I don't believe a long list of modes that *do* require an IV would
> > > > be  more readable or easy to maintain than this single compare ...
> > >
> > > That's where I disagree as you need extra knowledge to be aware of this.
> > > Being explicit removes any possible question one may ask. But that's a
> > > small point really :)
> > >
> > Well, while we're disagreeing ... I disagree with your assertion that you
> > would need more knowledge to know which modes do NOT need an IV
> > than to know which modes DO need an IV. There's really no fundamental
> > difference, it's two sides of the exact same coin ... you need that
> > knowledge either way.
> 
> The point is if you look for occurrences of, let's say
> CONTEXT_CONTROL_CRYPTO_MODE_CBC, to see the code path it'll be way
> easier if you have direct comparisons.
> 
Not a good enough reason considering the downsides of having less
performant code that is more difficult to maintain, IMHO (you will
have to keep adding modes as we have plenty more and may add even
more to the hardware later - and make sure the list is and stays
complete going forward). 
So how often do you really need to do that search?
Speaking as someone who has been adding new modes fairly recently
but never had any need for that ...

> > 1) This code is executed for each individual cipher call, i.e. it's in the
> > critical path. Having just 1 compare-and-branch there is better for
> > performance than having many.
> 
> Not sure about what the impact really is.
> 
Well, the driver is not exactly extracting maximum performance from 
our engine for small blocks at the moment, so I would say any cycle 
gained matters. Software overhead is the Achilles heel of hardware
acceleration. And if you don't accelerate, your hardware is defacto
useless. So anything in the critical path should be carefully
considered. (which is also why I object so strongly to all these API
corner cases we're forced to support, but now I digress ...)

You can debate about significance, but at some point it all adds up.
Better to not add the cycles - if it's no effort at all! - then to
have to optimize them away later. Speaking as someone who has spent
a major part of his professional life squeezing out those cycles to
meet very real and hard performance requirements.

> > 2) Generally, all else being equal, having less code is easier to maintain
> > than having more code.
> 
> That really depends, having readable code is easier to maintain :)
> 
But how would it be more readable? "Do this for any mode other than
ECB" seems pretty clear and readable to me? And is exactly what should
happen here. Also, less code of the same complexity level (i.e. the
same simple compares!) is still more readable than more code.

> > 4) If there is anything unclear about an otherwise fine code construct,
> > then you clarify it by adding a comment, not by rewriting it to be inefficient
> > and redundant ;-)
> 
> Fair point.
> 
> Thanks,
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com


Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

RE: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))

[Pascal Van Leeuwen]

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of
> Pascal Van Leeuwen
> Sent: Friday, July 26, 2019 2:57 PM
> To: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen <pascalvanl@gmail.com>
> Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
> Subject: RE: [PATCH 1/3] crypto: inside-secure - add support for
> authenc(hmac(sha1),cbc(des3_ede))
> 
> Antoine,
> 
> 
> > > +	.alg.aead = {
> > > +		.setkey = safexcel_aead_setkey,
> > > +		.encrypt = safexcel_aead_encrypt_3des,
> > > +		.decrypt = safexcel_aead_decrypt_3des,
> > > +		.ivsize = DES3_EDE_BLOCK_SIZE,
> > > +		.maxauthsize = SHA1_DIGEST_SIZE,
> > > +		.base = {
> > > +			.cra_name = "authenc(hmac(sha1),cbc(des3_ede))",
> > > +			.cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede",
> >
> > You could drop "_ede" here, or s/_/-/.
> >
> Agree the underscore should not be there.
> Our HW does not support any other form of 3DES so EDE doesn't
> really add much here, therefore I will just remove "_ede" entirely.
>
Actually, while looking into fixing this, I noticed that this 
naming style is actually consistent with the already existing
3des ecb and cbc ciphersuites, e.g.: "safexcel-cbc-des3_ebe",
so for consistency I would then suggest keeping it (or 
change the other 2 3des references at the same time, but I
don't know if that would break any legacy dependency).

> 
> > Apart from those small comments, the patch looks good.
> >
> > Thanks!
> > Antoine
> >
> > --
> > Antoine Ténart, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> 

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))

[Antoine Tenart]

On Tue, Jul 30, 2019 at 02:01:46PM +0000, Pascal Van Leeuwen wrote:
> > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of
> > Pascal Van Leeuwen
> > Sent: Friday, July 26, 2019 2:57 PM
> > To: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen <pascalvanl@gmail.com>
> > Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
> > Subject: RE: [PATCH 1/3] crypto: inside-secure - add support for
> > authenc(hmac(sha1),cbc(des3_ede))
> > 
> > > > +	.alg.aead = {
> > > > +		.setkey = safexcel_aead_setkey,
> > > > +		.encrypt = safexcel_aead_encrypt_3des,
> > > > +		.decrypt = safexcel_aead_decrypt_3des,
> > > > +		.ivsize = DES3_EDE_BLOCK_SIZE,
> > > > +		.maxauthsize = SHA1_DIGEST_SIZE,
> > > > +		.base = {
> > > > +			.cra_name = "authenc(hmac(sha1),cbc(des3_ede))",
> > > > +			.cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede",
> > >
> > > You could drop "_ede" here, or s/_/-/.
> > >
> > Agree the underscore should not be there.
> > Our HW does not support any other form of 3DES so EDE doesn't
> > really add much here, therefore I will just remove "_ede" entirely.
> >
> Actually, while looking into fixing this, I noticed that this 
> naming style is actually consistent with the already existing
> 3des ecb and cbc ciphersuites, e.g.: "safexcel-cbc-des3_ebe",
> so for consistency I would then suggest keeping it (or 
> change the other 2 3des references at the same time, but I
> don't know if that would break any legacy dependency).

Good catch :) As you said, you should keep it this way then.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com