[PATCH 0/3] crypto: talitos fixes for 3.6-rc7

[PATCH 0/3] crypto: talitos fixes for 3.6-rc7

[Horia Geanta]

Hi Herbert,

I know these patches come late and might not be included in 3.6.
But two of them are already sitting in cryptodev tree for some time.
(Don't know where my head was :/).

These patches are needed since:
e46e9a crypto: testmgr - add aead cbc aes hmac sha1,256,512 test vectors
made its way in 3.6-rc2 and is exposing two shortcomings of talitos:
- inability to handle non-contiguous assoc data and iv - fixed by 2/3
- inability to handle zero-length assoc data - fixed by 3/3

Patch 1/3 is the same as:
605425 crypto: talitos - fix icv management on outbound direction
from cryptodev tree.

Patch 2/3 is a rewritten version (so it applies cleanly) of:
79fd31 crypto: talitos - support for assoc data provided as scatterlist
from cryptodev tree.

Patch 3/3 is a new patch that covers the unusual case when assoc data length
is zero. It fixes for talitos what
9b2f4c crypto: authenc - Fix crash with zero-length assoc data
from crypto tree fixes for native crypto.
(http://www.mail-archive.com/linux-crypto@vger.kernel.org/msg07662.html)

AFAICT, patches apply cleanly both on crypto and 3.6-rc6 ToT.

Thanks,
Horia

Horia Geanta (3):
  crypto: talitos - fix icv management on outbound direction
  crypto: talitos - support for assoc data provided as scatterlist
  crypto: talitos - corrrectly handle zero-length assoc data

 drivers/crypto/talitos.c |  189 +++++++++++++++++++++++++++++++++-------------
 1 files changed, 137 insertions(+), 52 deletions(-)

-- 
1.7.7.6

[PATCH 1/3] crypto: talitos - fix icv management on outbound direction

[Horia Geanta]

For IPsec encryption, in the case when:
-the input buffer is fragmented (edesc->src_nents > 0)
-the output buffer is not fragmented (edesc->dst_nents = 0)
the ICV is not output in the link table, but after the encrypted payload.

Copying the ICV must be avoided in this case; consequently the condition
edesc->dma_len > 0 must be more specific, i.e. must depend on the type
of the output buffer - fragmented or not.

Testing was performed by modifying testmgr to support src != dst,
since currently native kernel IPsec does in-place encryption
(src == dst).

Signed-off-by: Horia Geanta <horia.geanta@freescale.com>
---
 drivers/crypto/talitos.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index efff788..9d56763 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -825,7 +825,7 @@ static void ipsec_esp_encrypt_done(struct device *dev,
 	ipsec_esp_unmap(dev, edesc, areq);
 
 	/* copy the generated ICV to dst */
-	if (edesc->dma_len) {
+	if (edesc->dst_nents) {
 		icvdata = &edesc->link_tbl[edesc->src_nents +
 					   edesc->dst_nents + 2];
 		sg = sg_last(areq->dst, edesc->dst_nents);
-- 
1.7.7.6

[PATCH 2/3] crypto: talitos - support for assoc data provided as scatterlist

[Horia Geanta]

Generate a link table in case assoc data is a scatterlist.
While at it, add support for handling non-contiguous assoc data and iv.

Signed-off-by: Horia Geanta <horia.geanta@freescale.com>
---
 drivers/crypto/talitos.c |  177 ++++++++++++++++++++++++++++++++-------------
 1 files changed, 126 insertions(+), 51 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 9d56763..9975718 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -714,8 +714,11 @@ badkey:
 
 /*
  * talitos_edesc - s/w-extended descriptor
+ * @assoc_nents: number of segments in associated data scatterlist
  * @src_nents: number of segments in input scatterlist
  * @dst_nents: number of segments in output scatterlist
+ * @assoc_chained: whether assoc is chained or not
+ * @iv_dma: dma address of iv for checking continuity and link table
  * @dma_len: length of dma mapped link_tbl space
  * @dma_link_tbl: bus physical address of link_tbl
  * @desc: h/w descriptor
@@ -726,10 +729,13 @@ badkey:
  * of link_tbl data
  */
 struct talitos_edesc {
+	int assoc_nents;
 	int src_nents;
 	int dst_nents;
+	bool assoc_chained;
 	int src_is_chained;
 	int dst_is_chained;
+	dma_addr_t iv_dma;
 	int dma_len;
 	dma_addr_t dma_link_tbl;
 	struct talitos_desc desc;
@@ -797,7 +803,13 @@ static void ipsec_esp_unmap(struct device *dev,
 	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[2], DMA_TO_DEVICE);
 	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[0], DMA_TO_DEVICE);
 
-	dma_unmap_sg(dev, areq->assoc, 1, DMA_TO_DEVICE);
+	if (edesc->assoc_chained)
+		talitos_unmap_sg_chain(dev, areq->assoc, DMA_TO_DEVICE);
+	else
+		/* assoc_nents counts also for IV in non-contiguous cases */
+		dma_unmap_sg(dev, areq->assoc,
+			     edesc->assoc_nents ? edesc->assoc_nents - 1 : 1,
+			     DMA_TO_DEVICE);
 
 	talitos_sg_unmap(dev, edesc, areq->src, areq->dst);
 
@@ -827,7 +839,8 @@ static void ipsec_esp_encrypt_done(struct device *dev,
 	/* copy the generated ICV to dst */
 	if (edesc->dst_nents) {
 		icvdata = &edesc->link_tbl[edesc->src_nents +
-					   edesc->dst_nents + 2];
+					   edesc->dst_nents + 2 +
+					   edesc->assoc_nents];
 		sg = sg_last(areq->dst, edesc->dst_nents);
 		memcpy((char *)sg_virt(sg) + sg->length - ctx->authsize,
 		       icvdata, ctx->authsize);
@@ -857,7 +870,8 @@ static void ipsec_esp_decrypt_swauth_done(struct device *dev,
 		/* auth check */
 		if (edesc->dma_len)
 			icvdata = &edesc->link_tbl[edesc->src_nents +
-						   edesc->dst_nents + 2];
+						   edesc->dst_nents + 2 +
+						   edesc->assoc_nents];
 		else
 			icvdata = &edesc->link_tbl[0];
 
@@ -932,10 +946,9 @@ static int sg_to_link_tbl(struct scatterlist *sg, int sg_count,
  * fill in and submit ipsec_esp descriptor
  */
 static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
-		     u8 *giv, u64 seq,
-		     void (*callback) (struct device *dev,
-				       struct talitos_desc *desc,
-				       void *context, int error))
+		     u64 seq, void (*callback) (struct device *dev,
+						struct talitos_desc *desc,
+						void *context, int error))
 {
 	struct crypto_aead *aead = crypto_aead_reqtfm(areq);
 	struct talitos_ctx *ctx = crypto_aead_ctx(aead);
@@ -950,12 +963,42 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	/* hmac key */
 	map_single_talitos_ptr(dev, &desc->ptr[0], ctx->authkeylen, &ctx->key,
 			       0, DMA_TO_DEVICE);
+
 	/* hmac data */
-	map_single_talitos_ptr(dev, &desc->ptr[1], areq->assoclen + ivsize,
-			       sg_virt(areq->assoc), 0, DMA_TO_DEVICE);
+	desc->ptr[1].len = cpu_to_be16(areq->assoclen + ivsize);
+	if (edesc->assoc_nents) {
+		int tbl_off = edesc->src_nents + edesc->dst_nents + 2;
+		struct talitos_ptr *tbl_ptr = &edesc->link_tbl[tbl_off];
+
+		to_talitos_ptr(&desc->ptr[1], edesc->dma_link_tbl + tbl_off *
+			       sizeof(struct talitos_ptr));
+		desc->ptr[1].j_extent = DESC_PTR_LNKTBL_JUMP;
+
+		/* assoc_nents - 1 entries for assoc, 1 for IV */
+		sg_count = sg_to_link_tbl(areq->assoc, edesc->assoc_nents - 1,
+					  areq->assoclen, tbl_ptr);
+
+		/* add IV to link table */
+		tbl_ptr += sg_count - 1;
+		tbl_ptr->j_extent = 0;
+		tbl_ptr++;
+		to_talitos_ptr(tbl_ptr, edesc->iv_dma);
+		tbl_ptr->len = cpu_to_be16(ivsize);
+		tbl_ptr->j_extent = DESC_PTR_LNKTBL_RETURN;
+
+		dma_sync_single_for_device(dev, edesc->dma_link_tbl,
+					   edesc->dma_len, DMA_BIDIRECTIONAL);
+	} else {
+		to_talitos_ptr(&desc->ptr[1], sg_dma_address(areq->assoc));
+		desc->ptr[1].j_extent = 0;
+	}
+
 	/* cipher iv */
-	map_single_talitos_ptr(dev, &desc->ptr[2], ivsize, giv ?: areq->iv, 0,
-			       DMA_TO_DEVICE);
+	to_talitos_ptr(&desc->ptr[2], edesc->iv_dma);
+	desc->ptr[2].len = cpu_to_be16(ivsize);
+	desc->ptr[2].j_extent = 0;
+	/* Sync needed for the aead_givencrypt case */
+	dma_sync_single_for_device(dev, edesc->iv_dma, ivsize, DMA_TO_DEVICE);
 
 	/* cipher key */
 	map_single_talitos_ptr(dev, &desc->ptr[3], ctx->enckeylen,
@@ -1012,26 +1055,25 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	if (sg_count == 1) {
 		to_talitos_ptr(&desc->ptr[5], sg_dma_address(areq->dst));
 	} else {
-		struct talitos_ptr *link_tbl_ptr =
-			&edesc->link_tbl[edesc->src_nents + 1];
+		int tbl_off = edesc->src_nents + 1;
+		struct talitos_ptr *tbl_ptr = &edesc->link_tbl[tbl_off];
 
 		to_talitos_ptr(&desc->ptr[5], edesc->dma_link_tbl +
-			       (edesc->src_nents + 1) *
-			       sizeof(struct talitos_ptr));
+			       tbl_off * sizeof(struct talitos_ptr));
 		sg_count = sg_to_link_tbl(areq->dst, sg_count, cryptlen,
-					  link_tbl_ptr);
+					  tbl_ptr);
 
 		/* Add an entry to the link table for ICV data */
-		link_tbl_ptr += sg_count - 1;
-		link_tbl_ptr->j_extent = 0;
-		sg_count++;
-		link_tbl_ptr++;
-		link_tbl_ptr->j_extent = DESC_PTR_LNKTBL_RETURN;
-		link_tbl_ptr->len = cpu_to_be16(authsize);
+		tbl_ptr += sg_count - 1;
+		tbl_ptr->j_extent = 0;
+		tbl_ptr++;
+		tbl_ptr->j_extent = DESC_PTR_LNKTBL_RETURN;
+		tbl_ptr->len = cpu_to_be16(authsize);
 
 		/* icv data follows link tables */
-		to_talitos_ptr(link_tbl_ptr, edesc->dma_link_tbl +
-			       (edesc->src_nents + edesc->dst_nents + 2) *
+		to_talitos_ptr(tbl_ptr, edesc->dma_link_tbl +
+			       (tbl_off + edesc->dst_nents + 1 +
+				edesc->assoc_nents) *
 			       sizeof(struct talitos_ptr));
 		desc->ptr[5].j_extent |= DESC_PTR_LNKTBL_JUMP;
 		dma_sync_single_for_device(ctx->dev, edesc->dma_link_tbl,
@@ -1132,17 +1174,23 @@ static size_t sg_copy_end_to_buffer(struct scatterlist *sgl, unsigned int nents,
  * allocate and map the extended descriptor
  */
 static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
+						 struct scatterlist *assoc,
 						 struct scatterlist *src,
 						 struct scatterlist *dst,
+						 u8 *iv,
 						 int hash_result,
+						 unsigned int assoclen,
 						 unsigned int cryptlen,
 						 unsigned int authsize,
+						 unsigned int ivsize,
 						 int icv_stashing,
 						 u32 cryptoflags)
 {
 	struct talitos_edesc *edesc;
-	int src_nents, dst_nents, alloc_len, dma_len;
-	int src_chained, dst_chained = 0;
+	int assoc_nents = 0, src_nents, dst_nents, alloc_len, dma_len;
+	int src_chained = 0, dst_chained = 0;
+	bool assoc_chained = false;
+	dma_addr_t iv_dma = 0;
 	gfp_t flags = cryptoflags & CRYPTO_TFM_REQ_MAY_SLEEP ? GFP_KERNEL :
 		      GFP_ATOMIC;
 
@@ -1151,6 +1199,25 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (iv)
+		iv_dma = dma_map_single(dev, iv, ivsize, DMA_TO_DEVICE);
+
+	if (assoc) {
+		/*
+		 * Currently it is assumed that iv is provided whenever assoc
+		 * is.
+		 */
+		BUG_ON(!iv);
+
+		assoc_nents = sg_count(assoc, assoclen, &assoc_chained);
+		talitos_map_sg(dev, assoc, assoc_nents, DMA_TO_DEVICE,
+			       assoc_chained);
+		assoc_nents = (assoc_nents == 1) ? 0 : assoc_nents;
+
+		if (assoc_nents || sg_dma_address(assoc) + assoclen != iv_dma)
+			assoc_nents = assoc_nents ? assoc_nents + 1 : 2;
+	}
+
 	src_nents = sg_count(src, cryptlen + authsize, &src_chained);
 	src_nents = (src_nents == 1) ? 0 : src_nents;
 
@@ -1172,9 +1239,9 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 	 * and the ICV data itself
 	 */
 	alloc_len = sizeof(struct talitos_edesc);
-	if (src_nents || dst_nents) {
-		dma_len = (src_nents + dst_nents + 2) *
-				 sizeof(struct talitos_ptr) + authsize;
+	if (assoc_nents || src_nents || dst_nents) {
+		dma_len = (src_nents + dst_nents + 2 + assoc_nents) *
+			  sizeof(struct talitos_ptr) + authsize;
 		alloc_len += dma_len;
 	} else {
 		dma_len = 0;
@@ -1183,14 +1250,20 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 
 	edesc = kmalloc(alloc_len, GFP_DMA | flags);
 	if (!edesc) {
+		talitos_unmap_sg_chain(dev, assoc, DMA_TO_DEVICE);
+		if (iv_dma)
+			dma_unmap_single(dev, iv_dma, ivsize, DMA_TO_DEVICE);
 		dev_err(dev, "could not allocate edescriptor\n");
 		return ERR_PTR(-ENOMEM);
 	}
 
+	edesc->assoc_nents = assoc_nents;
 	edesc->src_nents = src_nents;
 	edesc->dst_nents = dst_nents;
+	edesc->assoc_chained = assoc_chained;
 	edesc->src_is_chained = src_chained;
 	edesc->dst_is_chained = dst_chained;
+	edesc->iv_dma = iv_dma;
 	edesc->dma_len = dma_len;
 	if (dma_len)
 		edesc->dma_link_tbl = dma_map_single(dev, &edesc->link_tbl[0],
@@ -1200,14 +1273,16 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 	return edesc;
 }
 
-static struct talitos_edesc *aead_edesc_alloc(struct aead_request *areq,
+static struct talitos_edesc *aead_edesc_alloc(struct aead_request *areq, u8 *iv,
 					      int icv_stashing)
 {
 	struct crypto_aead *authenc = crypto_aead_reqtfm(areq);
 	struct talitos_ctx *ctx = crypto_aead_ctx(authenc);
+	unsigned int ivsize = crypto_aead_ivsize(authenc);
 
-	return talitos_edesc_alloc(ctx->dev, areq->src, areq->dst, 0,
-				   areq->cryptlen, ctx->authsize, icv_stashing,
+	return talitos_edesc_alloc(ctx->dev, areq->assoc, areq->src, areq->dst,
+				   iv, 0, areq->assoclen, areq->cryptlen,
+				   ctx->authsize, ivsize, icv_stashing,
 				   areq->base.flags);
 }
 
@@ -1218,14 +1293,14 @@ static int aead_encrypt(struct aead_request *req)
 	struct talitos_edesc *edesc;
 
 	/* allocate extended descriptor */
-	edesc = aead_edesc_alloc(req, 0);
+	edesc = aead_edesc_alloc(req, req->iv, 0);
 	if (IS_ERR(edesc))
 		return PTR_ERR(edesc);
 
 	/* set encrypt */
 	edesc->desc.hdr = ctx->desc_hdr_template | DESC_HDR_MODE0_ENCRYPT;
 
-	return ipsec_esp(edesc, req, NULL, 0, ipsec_esp_encrypt_done);
+	return ipsec_esp(edesc, req, 0, ipsec_esp_encrypt_done);
 }
 
 static int aead_decrypt(struct aead_request *req)
@@ -1241,7 +1316,7 @@ static int aead_decrypt(struct aead_request *req)
 	req->cryptlen -= authsize;
 
 	/* allocate extended descriptor */
-	edesc = aead_edesc_alloc(req, 1);
+	edesc = aead_edesc_alloc(req, req->iv, 1);
 	if (IS_ERR(edesc))
 		return PTR_ERR(edesc);
 
@@ -1257,9 +1332,7 @@ static int aead_decrypt(struct aead_request *req)
 		/* reset integrity check result bits */
 		edesc->desc.hdr_lo = 0;
 
-		return ipsec_esp(edesc, req, NULL, 0,
-				 ipsec_esp_decrypt_hwauth_done);
-
+		return ipsec_esp(edesc, req, 0, ipsec_esp_decrypt_hwauth_done);
 	}
 
 	/* Have to check the ICV with software */
@@ -1268,7 +1341,8 @@ static int aead_decrypt(struct aead_request *req)
 	/* stash incoming ICV for later cmp with ICV generated by the h/w */
 	if (edesc->dma_len)
 		icvdata = &edesc->link_tbl[edesc->src_nents +
-					   edesc->dst_nents + 2];
+					   edesc->dst_nents + 2 +
+					   edesc->assoc_nents];
 	else
 		icvdata = &edesc->link_tbl[0];
 
@@ -1277,7 +1351,7 @@ static int aead_decrypt(struct aead_request *req)
 	memcpy(icvdata, (char *)sg_virt(sg) + sg->length - ctx->authsize,
 	       ctx->authsize);
 
-	return ipsec_esp(edesc, req, NULL, 0, ipsec_esp_decrypt_swauth_done);
+	return ipsec_esp(edesc, req, 0, ipsec_esp_decrypt_swauth_done);
 }
 
 static int aead_givencrypt(struct aead_givcrypt_request *req)
@@ -1288,7 +1362,7 @@ static int aead_givencrypt(struct aead_givcrypt_request *req)
 	struct talitos_edesc *edesc;
 
 	/* allocate extended descriptor */
-	edesc = aead_edesc_alloc(areq, 0);
+	edesc = aead_edesc_alloc(areq, req->giv, 0);
 	if (IS_ERR(edesc))
 		return PTR_ERR(edesc);
 
@@ -1299,8 +1373,7 @@ static int aead_givencrypt(struct aead_givcrypt_request *req)
 	/* avoid consecutive packets going out with same IV */
 	*(__be64 *)req->giv ^= cpu_to_be64(req->seq);
 
-	return ipsec_esp(edesc, areq, req->giv, req->seq,
-			 ipsec_esp_encrypt_done);
+	return ipsec_esp(edesc, areq, req->seq, ipsec_esp_encrypt_done);
 }
 
 static int ablkcipher_setkey(struct crypto_ablkcipher *cipher,
@@ -1356,7 +1429,7 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 	struct device *dev = ctx->dev;
 	struct talitos_desc *desc = &edesc->desc;
 	unsigned int cryptlen = areq->nbytes;
-	unsigned int ivsize;
+	unsigned int ivsize = crypto_ablkcipher_ivsize(cipher);
 	int sg_count, ret;
 
 	/* first DWORD empty */
@@ -1365,9 +1438,9 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 	desc->ptr[0].j_extent = 0;
 
 	/* cipher iv */
-	ivsize = crypto_ablkcipher_ivsize(cipher);
-	map_single_talitos_ptr(dev, &desc->ptr[1], ivsize, areq->info, 0,
-			       DMA_TO_DEVICE);
+	to_talitos_ptr(&desc->ptr[1], edesc->iv_dma);
+	desc->ptr[1].len = cpu_to_be16(ivsize);
+	desc->ptr[1].j_extent = 0;
 
 	/* cipher key */
 	map_single_talitos_ptr(dev, &desc->ptr[2], ctx->keylen,
@@ -1450,9 +1523,11 @@ static struct talitos_edesc *ablkcipher_edesc_alloc(struct ablkcipher_request *
 {
 	struct crypto_ablkcipher *cipher = crypto_ablkcipher_reqtfm(areq);
 	struct talitos_ctx *ctx = crypto_ablkcipher_ctx(cipher);
+	unsigned int ivsize = crypto_ablkcipher_ivsize(cipher);
 
-	return talitos_edesc_alloc(ctx->dev, areq->src, areq->dst, 0,
-				   areq->nbytes, 0, 0, areq->base.flags);
+	return talitos_edesc_alloc(ctx->dev, NULL, areq->src, areq->dst,
+				   areq->info, 0, 0, areq->nbytes, 0, ivsize, 0,
+				   areq->base.flags);
 }
 
 static int ablkcipher_encrypt(struct ablkcipher_request *areq)
@@ -1631,8 +1706,8 @@ static struct talitos_edesc *ahash_edesc_alloc(struct ahash_request *areq,
 	struct talitos_ctx *ctx = crypto_ahash_ctx(tfm);
 	struct talitos_ahash_req_ctx *req_ctx = ahash_request_ctx(areq);
 
-	return talitos_edesc_alloc(ctx->dev, req_ctx->psrc, NULL, 1,
-				   nbytes, 0, 0, areq->base.flags);
+	return talitos_edesc_alloc(ctx->dev, NULL, req_ctx->psrc, NULL, NULL, 1,
+				   0, nbytes, 0, 0, 0, areq->base.flags);
 }
 
 static int ahash_init(struct ahash_request *areq)
-- 
1.7.7.6

[PATCH 3/3] crypto: talitos - corrrectly handle zero-length assoc data

[Horia Geanta]

talitos does not handle well zero-length assoc data.
>From dmesg:
talitos ffe30000.crypto: master data transfer error
talitos ffe30000.crypto: gather return/length error

Check whether assoc data is provided by inspecting assoclen,
not assoc pointer.
This is needed in order to pass testmgr tests.

Signed-off-by: Horia Geanta <horia.geanta@freescale.com>
---
 drivers/crypto/talitos.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 9975718..3ebc1bf 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -805,7 +805,7 @@ static void ipsec_esp_unmap(struct device *dev,
 
 	if (edesc->assoc_chained)
 		talitos_unmap_sg_chain(dev, areq->assoc, DMA_TO_DEVICE);
-	else
+	else if (areq->assoclen)
 		/* assoc_nents counts also for IV in non-contiguous cases */
 		dma_unmap_sg(dev, areq->assoc,
 			     edesc->assoc_nents ? edesc->assoc_nents - 1 : 1,
@@ -989,7 +989,11 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 		dma_sync_single_for_device(dev, edesc->dma_link_tbl,
 					   edesc->dma_len, DMA_BIDIRECTIONAL);
 	} else {
-		to_talitos_ptr(&desc->ptr[1], sg_dma_address(areq->assoc));
+		if (areq->assoclen)
+			to_talitos_ptr(&desc->ptr[1],
+				       sg_dma_address(areq->assoc));
+		else
+			to_talitos_ptr(&desc->ptr[1], edesc->iv_dma);
 		desc->ptr[1].j_extent = 0;
 	}
 
@@ -1199,10 +1203,10 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (iv)
+	if (ivsize)
 		iv_dma = dma_map_single(dev, iv, ivsize, DMA_TO_DEVICE);
 
-	if (assoc) {
+	if (assoclen) {
 		/*
 		 * Currently it is assumed that iv is provided whenever assoc
 		 * is.
@@ -1250,9 +1254,16 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 
 	edesc = kmalloc(alloc_len, GFP_DMA | flags);
 	if (!edesc) {
-		talitos_unmap_sg_chain(dev, assoc, DMA_TO_DEVICE);
+		if (assoc_chained)
+			talitos_unmap_sg_chain(dev, assoc, DMA_TO_DEVICE);
+		else if (assoclen)
+			dma_unmap_sg(dev, assoc,
+				     assoc_nents ? assoc_nents - 1 : 1,
+				     DMA_TO_DEVICE);
+
 		if (iv_dma)
 			dma_unmap_single(dev, iv_dma, ivsize, DMA_TO_DEVICE);
+
 		dev_err(dev, "could not allocate edescriptor\n");
 		return ERR_PTR(-ENOMEM);
 	}
-- 
1.7.7.6

[PATCH] crypto: talitos - corrrectly handle zero-length assoc data

[Horia Geanta]

talitos does not handle well zero-length assoc data. From dmesg:
talitos ffe30000.crypto: master data transfer error
talitos ffe30000.crypto: gather return/length error

Check whether assoc data is provided by inspecting assoclen,
not assoc pointer.
This is needed in order to pass testmgr tests.

Signed-off-by: Horia Geanta <horia.geanta@freescale.com>
---
This patch was submitted late in the 3.6 cycle, but has not
showed up - neither in 3.6 nor in 3.7. Please apply.

 drivers/crypto/talitos.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index f6f7c68..af3e7dc 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -788,7 +788,7 @@ static void ipsec_esp_unmap(struct device *dev,
 
 	if (edesc->assoc_chained)
 		talitos_unmap_sg_chain(dev, areq->assoc, DMA_TO_DEVICE);
-	else
+	else if (areq->assoclen)
 		/* assoc_nents counts also for IV in non-contiguous cases */
 		dma_unmap_sg(dev, areq->assoc,
 			     edesc->assoc_nents ? edesc->assoc_nents - 1 : 1,
@@ -971,7 +971,11 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 		dma_sync_single_for_device(dev, edesc->dma_link_tbl,
 					   edesc->dma_len, DMA_BIDIRECTIONAL);
 	} else {
-		to_talitos_ptr(&desc->ptr[1], sg_dma_address(areq->assoc));
+		if (areq->assoclen)
+			to_talitos_ptr(&desc->ptr[1],
+				       sg_dma_address(areq->assoc));
+		else
+			to_talitos_ptr(&desc->ptr[1], edesc->iv_dma);
 		desc->ptr[1].j_extent = 0;
 	}
 
@@ -1120,10 +1124,10 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (iv)
+	if (ivsize)
 		iv_dma = dma_map_single(dev, iv, ivsize, DMA_TO_DEVICE);
 
-	if (assoc) {
+	if (assoclen) {
 		/*
 		 * Currently it is assumed that iv is provided whenever assoc
 		 * is.
@@ -1171,9 +1175,16 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 
 	edesc = kmalloc(alloc_len, GFP_DMA | flags);
 	if (!edesc) {
-		talitos_unmap_sg_chain(dev, assoc, DMA_TO_DEVICE);
+		if (assoc_chained)
+			talitos_unmap_sg_chain(dev, assoc, DMA_TO_DEVICE);
+		else if (assoclen)
+			dma_unmap_sg(dev, assoc,
+				     assoc_nents ? assoc_nents - 1 : 1,
+				     DMA_TO_DEVICE);
+
 		if (iv_dma)
 			dma_unmap_single(dev, iv_dma, ivsize, DMA_TO_DEVICE);
+
 		dev_err(dev, "could not allocate edescriptor\n");
 		return ERR_PTR(-ENOMEM);
 	}
-- 
1.7.7.6

Re: [PATCH] crypto: talitos - corrrectly handle zero-length assoc data

[Herbert Xu]

On Tue, Nov 19, 2013 at 02:57:49PM +0200, Horia Geanta wrote:
> talitos does not handle well zero-length assoc data. From dmesg:
> talitos ffe30000.crypto: master data transfer error
> talitos ffe30000.crypto: gather return/length error
> 
> Check whether assoc data is provided by inspecting assoclen,
> not assoc pointer.
> This is needed in order to pass testmgr tests.
> 
> Signed-off-by: Horia Geanta <horia.geanta@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

Re: [PATCH 0/3] crypto: talitos fixes for 3.6-rc7

[Herbert Xu]

On Wed, Sep 19, 2012 at 09:53:37PM +0300, Horia Geanta wrote:
> Hi Herbert,
> 
> I know these patches come late and might not be included in 3.6.
> But two of them are already sitting in cryptodev tree for some time.
> (Don't know where my head was :/).

I'm hesitant to put these patches in at this stage.  For the
problems fixed by them do we have simpler fixes for 3.6? For
example, by reverting changesets that introduced the issue if
it's a regression, or disabling the buggy functionality?

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 0/3] crypto: talitos fixes for 3.6-rc7

[Horia Geanta]

On 9/27/2012 8:25 AM, Herbert Xu wrote:
> On Wed, Sep 19, 2012 at 09:53:37PM +0300, Horia Geanta wrote:
>> Hi Herbert,
>>
>> I know these patches come late and might not be included in 3.6.
>> But two of them are already sitting in cryptodev tree for some time.
>> (Don't know where my head was :/).
>
> I'm hesitant to put these patches in at this stage.  For the

That's understandable.

> problems fixed by them do we have simpler fixes for 3.6? For
> example, by reverting changesets that introduced the issue if

Reverting
e46e9a crypto: testmgr - add aead cbc aes hmac sha1,256,512 test vectors
(added in 3.6-rc2) would also do fine.

Of course, it would be added back in during 3.7 merge window, with the 
other talitos-specific patches currently sitting in cryptodev.

What I'm trying to say here is that allowing the patch to go in 3.6 
without all the other is incorrect. Sorry for not posting all of them as 
a patchset.

Again, this patch (e46e9a) is not the culprit, it merely exposes the 
deficiencies in the talitos driver.
AFAICT, these cases (zero assoc data, noncontiguous iv and assoc data) 
have never been handled correctly in the driver.

> it's a regression, or disabling the buggy functionality?

Don't think so, it's core functionality.

>
> Thanks,
>

Re: [PATCH 0/3] crypto: talitos fixes for 3.6-rc7

[Herbert Xu]

On Thu, Sep 27, 2012 at 11:01:50AM +0300, Horia Geanta wrote:
>
> Again, this patch (e46e9a) is not the culprit, it merely exposes the
> deficiencies in the talitos driver.
> AFAICT, these cases (zero assoc data, noncontiguous iv and assoc
> data) have never been handled correctly in the driver.

OK, without the patches does the driver crash the self-test or
merely fail the self-test?

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 0/3] crypto: talitos fixes for 3.6-rc7

[Horia Geanta]

On 9/27/2012 11:14 AM, Herbert Xu wrote:
> On Thu, Sep 27, 2012 at 11:01:50AM +0300, Horia Geanta wrote:
>>
>> Again, this patch (e46e9a) is not the culprit, it merely exposes the
>> deficiencies in the talitos driver.
>> AFAICT, these cases (zero assoc data, noncontiguous iv and assoc
>> data) have never been handled correctly in the driver.
>
> OK, without the patches does the driver crash the self-test or
> merely fail the self-test?

It fails.
Crash occurs when setting the fips=1 boot param.

Re: [PATCH 0/3] crypto: talitos fixes for 3.6-rc7

[Herbert Xu]

On Thu, Sep 27, 2012 at 11:42:15AM +0300, Horia Geanta wrote:
> On 9/27/2012 11:14 AM, Herbert Xu wrote:
> >On Thu, Sep 27, 2012 at 11:01:50AM +0300, Horia Geanta wrote:
> >>
> >>Again, this patch (e46e9a) is not the culprit, it merely exposes the
> >>deficiencies in the talitos driver.
> >>AFAICT, these cases (zero assoc data, noncontiguous iv and assoc
> >>data) have never been handled correctly in the driver.
> >
> >OK, without the patches does the driver crash the self-test or
> >merely fail the self-test?
> 
> It fails.
> Crash occurs when setting the fips=1 boot param.

OK, in that case I think we can just leave it as it is.

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