linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input
@ 2019-05-15 11:25 Iuliana Prodan
  2019-05-15 11:25 ` [PATCH v2 2/2] crypto: caam - strip input without changing crypto request Iuliana Prodan
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Iuliana Prodan @ 2019-05-15 11:25 UTC (permalink / raw)
  To: Herbert Xu, Horia Geanta, Aymen Sghaier
  Cc: David S. Miller, linux-crypto, linux-kernel, linux-imx

The problem is with the input data size sent to CAAM for encrypt/decrypt.
Pkcs1pad is failing due to pkcs1 padding done in SW starting with0x01
instead of 0x00 0x01.
CAAM expects an input of modulus size. For this we strip the leading
zeros in case the size is more than modulus or pad the input with zeros
until the modulus size is reached.

Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
---
Changes since V1:
	- remove not needed initialization of a variable;
	- free resources on error path.
---
 drivers/crypto/caam/caampkc.c | 84 +++++++++++++++++++++++++++++++++++--------
 drivers/crypto/caam/caampkc.h |  2 ++
 2 files changed, 72 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
index fe24485..e356413 100644
--- a/drivers/crypto/caam/caampkc.c
+++ b/drivers/crypto/caam/caampkc.c
@@ -24,6 +24,10 @@
 				 sizeof(struct rsa_priv_f2_pdb))
 #define DESC_RSA_PRIV_F3_LEN	(2 * CAAM_CMD_SZ + \
 				 sizeof(struct rsa_priv_f3_pdb))
+#define CAAM_RSA_MAX_INPUT_SIZE	512 /* for a 4096-bit modulus */
+
+/* buffer filled with zeros, used for padding */
+static u8 *zero_buffer;
 
 static void rsa_io_unmap(struct device *dev, struct rsa_edesc *edesc,
 			 struct akcipher_request *req)
@@ -168,6 +172,13 @@ static void rsa_priv_f3_done(struct device *dev, u32 *desc, u32 err,
 	akcipher_request_complete(req, err);
 }
 
+/**
+ * Count leading zeros, need it to strip, from a given scatterlist
+ *
+ * @sgl   : scatterlist to count zeros from
+ * @nbytes: number of zeros, in bytes, to strip
+ * @flags : operation flags
+ */
 static int caam_rsa_count_leading_zeros(struct scatterlist *sgl,
 					unsigned int nbytes,
 					unsigned int flags)
@@ -187,7 +198,8 @@ static int caam_rsa_count_leading_zeros(struct scatterlist *sgl,
 	lzeros = 0;
 	len = 0;
 	while (nbytes > 0) {
-		while (len && !*buff) {
+		/* do not strip more than given bytes */
+		while (len && !*buff && lzeros < nbytes) {
 			lzeros++;
 			len--;
 			buff++;
@@ -218,6 +230,7 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 	struct caam_rsa_ctx *ctx = akcipher_tfm_ctx(tfm);
 	struct device *dev = ctx->dev;
 	struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
+	struct caam_rsa_key *key = &ctx->key;
 	struct rsa_edesc *edesc;
 	gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
 		       GFP_KERNEL : GFP_ATOMIC;
@@ -225,20 +238,37 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 	int sgc;
 	int sec4_sg_index, sec4_sg_len = 0, sec4_sg_bytes;
 	int src_nents, dst_nents;
+	unsigned int diff_size = 0;
 	int lzeros;
 
-	lzeros = caam_rsa_count_leading_zeros(req->src, req->src_len, sg_flags);
-	if (lzeros < 0)
-		return ERR_PTR(lzeros);
-
-	req->src_len -= lzeros;
-	req->src = scatterwalk_ffwd(req_ctx->src, req->src, lzeros);
+	if (req->src_len > key->n_sz) {
+		/*
+		 * strip leading zeros and
+		 * return the number of zeros to skip
+		 */
+		lzeros = caam_rsa_count_leading_zeros(req->src, req->src_len -
+						      key->n_sz, sg_flags);
+		if (lzeros < 0)
+			return ERR_PTR(lzeros);
+
+		req->src_len -= lzeros;
+		req->src = scatterwalk_ffwd(req_ctx->src, req->src, lzeros);
+	} else {
+		/*
+		 * input src is less then n key modulus,
+		 * so there will be zero padding
+		 */
+		diff_size = key->n_sz - req->src_len;
+	}
 
 	src_nents = sg_nents_for_len(req->src, req->src_len);
 	dst_nents = sg_nents_for_len(req->dst, req->dst_len);
 
-	if (src_nents > 1)
-		sec4_sg_len = src_nents;
+	if (!diff_size && src_nents == 1)
+		sec4_sg_len = 0; /* no need for an input hw s/g table */
+	else
+		sec4_sg_len = src_nents + !!diff_size;
+	sec4_sg_index = sec4_sg_len;
 	if (dst_nents > 1)
 		sec4_sg_len += dst_nents;
 
@@ -263,12 +293,14 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 	}
 
 	edesc->sec4_sg = (void *)edesc + sizeof(*edesc) + desclen;
+	if (diff_size)
+		dma_to_sec4_sg_one(edesc->sec4_sg, ctx->padding_dma, diff_size,
+				   0);
+
+	if (sec4_sg_index)
+		sg_to_sec4_sg_last(req->src, src_nents, edesc->sec4_sg +
+				   !!diff_size, 0);
 
-	sec4_sg_index = 0;
-	if (src_nents > 1) {
-		sg_to_sec4_sg_last(req->src, src_nents, edesc->sec4_sg, 0);
-		sec4_sg_index += src_nents;
-	}
 	if (dst_nents > 1)
 		sg_to_sec4_sg_last(req->dst, dst_nents,
 				   edesc->sec4_sg + sec4_sg_index, 0);
@@ -289,6 +321,10 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 
 	edesc->sec4_sg_bytes = sec4_sg_bytes;
 
+	print_hex_dump_debug("caampkc sec4_sg@" __stringify(__LINE__) ": ",
+			     DUMP_PREFIX_ADDRESS, 16, 4, edesc->sec4_sg,
+			     edesc->sec4_sg_bytes, 1);
+
 	return edesc;
 
 sec4_sg_fail:
@@ -978,6 +1014,15 @@ static int caam_rsa_init_tfm(struct crypto_akcipher *tfm)
 		return PTR_ERR(ctx->dev);
 	}
 
+	ctx->padding_dma = dma_map_single(ctx->dev, zero_buffer,
+					  CAAM_RSA_MAX_INPUT_SIZE - 1,
+					  DMA_TO_DEVICE);
+	if (dma_mapping_error(ctx->dev, ctx->padding_dma)) {
+		dev_err(ctx->dev, "unable to map padding\n");
+		caam_jr_free(ctx->dev);
+		return -ENOMEM;
+	}
+
 	return 0;
 }
 
@@ -987,6 +1032,8 @@ static void caam_rsa_exit_tfm(struct crypto_akcipher *tfm)
 	struct caam_rsa_ctx *ctx = akcipher_tfm_ctx(tfm);
 	struct caam_rsa_key *key = &ctx->key;
 
+	dma_unmap_single(ctx->dev, ctx->padding_dma, CAAM_RSA_MAX_INPUT_SIZE -
+			 1, DMA_TO_DEVICE);
 	caam_rsa_free_key(key);
 	caam_jr_free(ctx->dev);
 }
@@ -1058,6 +1105,14 @@ static int __init caam_pkc_init(void)
 		goto out_put_dev;
 	}
 
+	/* allocate zero buffer, used for padding input */
+	zero_buffer = kzalloc(CAAM_RSA_MAX_INPUT_SIZE - 1, GFP_DMA |
+			      GFP_KERNEL);
+	if (!zero_buffer) {
+		err = -ENOMEM;
+		goto out_put_dev;
+	}
+
 	err = crypto_register_akcipher(&caam_rsa);
 	if (err)
 		dev_warn(ctrldev, "%s alg registration failed\n",
@@ -1072,6 +1127,7 @@ static int __init caam_pkc_init(void)
 
 static void __exit caam_pkc_exit(void)
 {
+	kfree(zero_buffer);
 	crypto_unregister_akcipher(&caam_rsa);
 }
 
diff --git a/drivers/crypto/caam/caampkc.h b/drivers/crypto/caam/caampkc.h
index 82645bc..5ac7201 100644
--- a/drivers/crypto/caam/caampkc.h
+++ b/drivers/crypto/caam/caampkc.h
@@ -89,10 +89,12 @@ struct caam_rsa_key {
  * caam_rsa_ctx - per session context.
  * @key         : RSA key in DMA zone
  * @dev         : device structure
+ * @padding_dma : dma address of padding, for adding it to the input
  */
 struct caam_rsa_ctx {
 	struct caam_rsa_key key;
 	struct device *dev;
+	dma_addr_t padding_dma;
 };
 
 /**
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] crypto: caam - strip input without changing crypto request
  2019-05-15 11:25 [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input Iuliana Prodan
@ 2019-05-15 11:25 ` Iuliana Prodan
  2019-05-15 12:59   ` Horia Geanta
  2019-05-15 12:45 ` [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input Horia Geanta
  2019-05-23  6:12 ` Herbert Xu
  2 siblings, 1 reply; 10+ messages in thread
From: Iuliana Prodan @ 2019-05-15 11:25 UTC (permalink / raw)
  To: Herbert Xu, Horia Geanta, Aymen Sghaier
  Cc: David S. Miller, linux-crypto, linux-kernel, linux-imx

For rsa/pkcs1pad(rsa-caam, sha256), CAAM expects an input of modulus size.
For this we strip the leading zeros in case the size is more than modulus.
This commit avoids modifying the crypto request while striping zeros from
input, to comply with the crypto API requirement. This is done by adding
a fixup input pointer and length.

Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
---
Changes since V1:
	- changed the commit message and summary.
---
 drivers/crypto/caam/caampkc.c | 39 ++++++++++++++++++++++++++-------------
 drivers/crypto/caam/caampkc.h |  7 ++++++-
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
index e356413..41591f8 100644
--- a/drivers/crypto/caam/caampkc.c
+++ b/drivers/crypto/caam/caampkc.c
@@ -32,8 +32,10 @@ static u8 *zero_buffer;
 static void rsa_io_unmap(struct device *dev, struct rsa_edesc *edesc,
 			 struct akcipher_request *req)
 {
+	struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
+
 	dma_unmap_sg(dev, req->dst, edesc->dst_nents, DMA_FROM_DEVICE);
-	dma_unmap_sg(dev, req->src, edesc->src_nents, DMA_TO_DEVICE);
+	dma_unmap_sg(dev, req_ctx->fixup_src, edesc->src_nents, DMA_TO_DEVICE);
 
 	if (edesc->sec4_sg_bytes)
 		dma_unmap_single(dev, edesc->sec4_sg_dma, edesc->sec4_sg_bytes,
@@ -251,17 +253,21 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 		if (lzeros < 0)
 			return ERR_PTR(lzeros);
 
-		req->src_len -= lzeros;
-		req->src = scatterwalk_ffwd(req_ctx->src, req->src, lzeros);
+		req_ctx->fixup_src = scatterwalk_ffwd(req_ctx->src, req->src,
+						      lzeros);
+		req_ctx->fixup_src_len = req->src_len - lzeros;
 	} else {
 		/*
 		 * input src is less then n key modulus,
 		 * so there will be zero padding
 		 */
 		diff_size = key->n_sz - req->src_len;
+		req_ctx->fixup_src = req->src;
+		req_ctx->fixup_src_len = req->src_len;
 	}
 
-	src_nents = sg_nents_for_len(req->src, req->src_len);
+	src_nents = sg_nents_for_len(req_ctx->fixup_src,
+				     req_ctx->fixup_src_len);
 	dst_nents = sg_nents_for_len(req->dst, req->dst_len);
 
 	if (!diff_size && src_nents == 1)
@@ -280,7 +286,7 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 	if (!edesc)
 		return ERR_PTR(-ENOMEM);
 
-	sgc = dma_map_sg(dev, req->src, src_nents, DMA_TO_DEVICE);
+	sgc = dma_map_sg(dev, req_ctx->fixup_src, src_nents, DMA_TO_DEVICE);
 	if (unlikely(!sgc)) {
 		dev_err(dev, "unable to map source\n");
 		goto src_fail;
@@ -298,8 +304,8 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 				   0);
 
 	if (sec4_sg_index)
-		sg_to_sec4_sg_last(req->src, src_nents, edesc->sec4_sg +
-				   !!diff_size, 0);
+		sg_to_sec4_sg_last(req_ctx->fixup_src, src_nents,
+				   edesc->sec4_sg + !!diff_size, 0);
 
 	if (dst_nents > 1)
 		sg_to_sec4_sg_last(req->dst, dst_nents,
@@ -330,7 +336,7 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 sec4_sg_fail:
 	dma_unmap_sg(dev, req->dst, dst_nents, DMA_FROM_DEVICE);
 dst_fail:
-	dma_unmap_sg(dev, req->src, src_nents, DMA_TO_DEVICE);
+	dma_unmap_sg(dev, req_ctx->fixup_src, src_nents, DMA_TO_DEVICE);
 src_fail:
 	kfree(edesc);
 	return ERR_PTR(-ENOMEM);
@@ -340,6 +346,7 @@ static int set_rsa_pub_pdb(struct akcipher_request *req,
 			   struct rsa_edesc *edesc)
 {
 	struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
+	struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
 	struct caam_rsa_ctx *ctx = akcipher_tfm_ctx(tfm);
 	struct caam_rsa_key *key = &ctx->key;
 	struct device *dev = ctx->dev;
@@ -364,7 +371,7 @@ static int set_rsa_pub_pdb(struct akcipher_request *req,
 		pdb->f_dma = edesc->sec4_sg_dma;
 		sec4_sg_index += edesc->src_nents;
 	} else {
-		pdb->f_dma = sg_dma_address(req->src);
+		pdb->f_dma = sg_dma_address(req_ctx->fixup_src);
 	}
 
 	if (edesc->dst_nents > 1) {
@@ -376,7 +383,7 @@ static int set_rsa_pub_pdb(struct akcipher_request *req,
 	}
 
 	pdb->sgf |= (key->e_sz << RSA_PDB_E_SHIFT) | key->n_sz;
-	pdb->f_len = req->src_len;
+	pdb->f_len = req_ctx->fixup_src_len;
 
 	return 0;
 }
@@ -409,7 +416,9 @@ static int set_rsa_priv_f1_pdb(struct akcipher_request *req,
 		pdb->g_dma = edesc->sec4_sg_dma;
 		sec4_sg_index += edesc->src_nents;
 	} else {
-		pdb->g_dma = sg_dma_address(req->src);
+		struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
+
+		pdb->g_dma = sg_dma_address(req_ctx->fixup_src);
 	}
 
 	if (edesc->dst_nents > 1) {
@@ -472,7 +481,9 @@ static int set_rsa_priv_f2_pdb(struct akcipher_request *req,
 		pdb->g_dma = edesc->sec4_sg_dma;
 		sec4_sg_index += edesc->src_nents;
 	} else {
-		pdb->g_dma = sg_dma_address(req->src);
+		struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
+
+		pdb->g_dma = sg_dma_address(req_ctx->fixup_src);
 	}
 
 	if (edesc->dst_nents > 1) {
@@ -559,7 +570,9 @@ static int set_rsa_priv_f3_pdb(struct akcipher_request *req,
 		pdb->g_dma = edesc->sec4_sg_dma;
 		sec4_sg_index += edesc->src_nents;
 	} else {
-		pdb->g_dma = sg_dma_address(req->src);
+		struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
+
+		pdb->g_dma = sg_dma_address(req_ctx->fixup_src);
 	}
 
 	if (edesc->dst_nents > 1) {
diff --git a/drivers/crypto/caam/caampkc.h b/drivers/crypto/caam/caampkc.h
index 5ac7201..2c488c9 100644
--- a/drivers/crypto/caam/caampkc.h
+++ b/drivers/crypto/caam/caampkc.h
@@ -95,14 +95,19 @@ struct caam_rsa_ctx {
 	struct caam_rsa_key key;
 	struct device *dev;
 	dma_addr_t padding_dma;
+
 };
 
 /**
  * caam_rsa_req_ctx - per request context.
- * @src: input scatterlist (stripped of leading zeros)
+ * @src           : input scatterlist (stripped of leading zeros)
+ * @fixup_src     : input scatterlist (that might be stripped of leading zeros)
+ * @fixup_src_len : length of the fixup_src input scatterlist
  */
 struct caam_rsa_req_ctx {
 	struct scatterlist src[2];
+	struct scatterlist *fixup_src;
+	unsigned int fixup_src_len;
 };
 
 /**
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input
  2019-05-15 11:25 [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input Iuliana Prodan
  2019-05-15 11:25 ` [PATCH v2 2/2] crypto: caam - strip input without changing crypto request Iuliana Prodan
@ 2019-05-15 12:45 ` Horia Geanta
  2019-05-23  6:12 ` Herbert Xu
  2 siblings, 0 replies; 10+ messages in thread
From: Horia Geanta @ 2019-05-15 12:45 UTC (permalink / raw)
  To: Iuliana Prodan, Herbert Xu, Aymen Sghaier
  Cc: David S. Miller, linux-crypto, linux-kernel, dl-linux-imx

On 5/15/2019 2:25 PM, Iuliana Prodan wrote:
> The problem is with the input data size sent to CAAM for encrypt/decrypt.
> Pkcs1pad is failing due to pkcs1 padding done in SW starting with0x01
> instead of 0x00 0x01.
> CAAM expects an input of modulus size. For this we strip the leading
> zeros in case the size is more than modulus or pad the input with zeros
> until the modulus size is reached.
> 
> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>

Thanks,
Horia

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] crypto: caam - strip input without changing crypto request
  2019-05-15 11:25 ` [PATCH v2 2/2] crypto: caam - strip input without changing crypto request Iuliana Prodan
@ 2019-05-15 12:59   ` Horia Geanta
  2019-05-15 14:00     ` Iuliana Prodan
  0 siblings, 1 reply; 10+ messages in thread
From: Horia Geanta @ 2019-05-15 12:59 UTC (permalink / raw)
  To: Iuliana Prodan, Herbert Xu, Aymen Sghaier
  Cc: David S. Miller, linux-crypto, linux-kernel, dl-linux-imx

On 5/15/2019 2:25 PM, Iuliana Prodan wrote:
> For rsa/pkcs1pad(rsa-caam, sha256), CAAM expects an input of modulus size.
Only for sha256?

> For this we strip the leading zeros in case the size is more than modulus.
> This commit avoids modifying the crypto request while striping zeros from
                                                        ^^^ stripping

Horia

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] crypto: caam - strip input without changing crypto request
  2019-05-15 12:59   ` Horia Geanta
@ 2019-05-15 14:00     ` Iuliana Prodan
  0 siblings, 0 replies; 10+ messages in thread
From: Iuliana Prodan @ 2019-05-15 14:00 UTC (permalink / raw)
  To: Horia Geanta, Herbert Xu, Aymen Sghaier
  Cc: David S. Miller, linux-crypto, linux-kernel, dl-linux-imx

On 5/15/2019 3:59 PM, Horia Geanta wrote:
> On 5/15/2019 2:25 PM, Iuliana Prodan wrote:
>> For rsa/pkcs1pad(rsa-caam, sha256), CAAM expects an input of modulus size.
> Only for sha256?
No, is not just sha256. Is a combination of rsa-caam and a hash 
algorithm. I'll send a new version to fix the commit message.
> 
>> For this we strip the leading zeros in case the size is more than modulus.
>> This commit avoids modifying the crypto request while striping zeros from
>                                                          ^^^ stripping
> 
> Horia
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input
  2019-05-15 11:25 [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input Iuliana Prodan
  2019-05-15 11:25 ` [PATCH v2 2/2] crypto: caam - strip input without changing crypto request Iuliana Prodan
  2019-05-15 12:45 ` [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input Horia Geanta
@ 2019-05-23  6:12 ` Herbert Xu
  2019-05-23  8:08   ` Iuliana Prodan
  2019-05-23 10:02   ` Horia Geanta
  2 siblings, 2 replies; 10+ messages in thread
From: Herbert Xu @ 2019-05-23  6:12 UTC (permalink / raw)
  To: Iuliana Prodan
  Cc: Horia Geanta, Aymen Sghaier, David S. Miller, linux-crypto,
	linux-kernel, linux-imx

On Wed, May 15, 2019 at 02:25:45PM +0300, Iuliana Prodan wrote:
>
> @@ -1058,6 +1105,14 @@ static int __init caam_pkc_init(void)
>  		goto out_put_dev;
>  	}
>  
> +	/* allocate zero buffer, used for padding input */
> +	zero_buffer = kzalloc(CAAM_RSA_MAX_INPUT_SIZE - 1, GFP_DMA |
> +			      GFP_KERNEL);
> +	if (!zero_buffer) {
> +		err = -ENOMEM;
> +		goto out_put_dev;
> +	}
> +
>  	err = crypto_register_akcipher(&caam_rsa);
>  	if (err)
>  		dev_warn(ctrldev, "%s alg registration failed\n",

This patch does not apply on top of the caam patch-series from Horia.
You're also going to leak zero_buffer if crypto_register_akcipher
fails.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input
  2019-05-23  6:12 ` Herbert Xu
@ 2019-05-23  8:08   ` Iuliana Prodan
  2019-05-23 10:02   ` Horia Geanta
  1 sibling, 0 replies; 10+ messages in thread
From: Iuliana Prodan @ 2019-05-23  8:08 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Horia Geanta, Aymen Sghaier, David S. Miller, linux-crypto,
	linux-kernel, dl-linux-imx

On 5/23/2019 9:12 AM, Herbert Xu wrote:
> On Wed, May 15, 2019 at 02:25:45PM +0300, Iuliana Prodan wrote:
>>
>> @@ -1058,6 +1105,14 @@ static int __init caam_pkc_init(void)
>>   		goto out_put_dev;
>>   	}
>>   
>> +	/* allocate zero buffer, used for padding input */
>> +	zero_buffer = kzalloc(CAAM_RSA_MAX_INPUT_SIZE - 1, GFP_DMA |
>> +			      GFP_KERNEL);
>> +	if (!zero_buffer) {
>> +		err = -ENOMEM;
>> +		goto out_put_dev;
>> +	}
>> +
>>   	err = crypto_register_akcipher(&caam_rsa);
>>   	if (err)
>>   		dev_warn(ctrldev, "%s alg registration failed\n",
> 
> This patch does not apply on top of the caam patch-series from Horia.
> You're also going to leak zero_buffer if crypto_register_akcipher
> fails.
> 
> Cheers,
> 

I'll fix the conflicts and also the leak of zero_buffer if 
crypto_register_akcipher fails and send a new version.

Regards,
Iulia

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input
  2019-05-23  6:12 ` Herbert Xu
  2019-05-23  8:08   ` Iuliana Prodan
@ 2019-05-23 10:02   ` Horia Geanta
  2019-05-23 12:38     ` Herbert Xu
  1 sibling, 1 reply; 10+ messages in thread
From: Horia Geanta @ 2019-05-23 10:02 UTC (permalink / raw)
  To: Herbert Xu, Iuliana Prodan
  Cc: Aymen Sghaier, David S. Miller, linux-crypto, linux-kernel, dl-linux-imx

On 5/23/2019 9:12 AM, Herbert Xu wrote:
> On Wed, May 15, 2019 at 02:25:45PM +0300, Iuliana Prodan wrote:
>>
>> @@ -1058,6 +1105,14 @@ static int __init caam_pkc_init(void)
>>  		goto out_put_dev;
>>  	}
>>  
>> +	/* allocate zero buffer, used for padding input */
>> +	zero_buffer = kzalloc(CAAM_RSA_MAX_INPUT_SIZE - 1, GFP_DMA |
>> +			      GFP_KERNEL);
>> +	if (!zero_buffer) {
>> +		err = -ENOMEM;
>> +		goto out_put_dev;
>> +	}
>> +
>>  	err = crypto_register_akcipher(&caam_rsa);
>>  	if (err)
>>  		dev_warn(ctrldev, "%s alg registration failed\n",
> 
> This patch does not apply on top of the caam patch-series from Horia.
The patch was considered a fix, and thus developed on top of crypto-2.6.
I guess you are implicitly asking to resubmit based on cryptodev-2.6, correct?

> You're also going to leak zero_buffer if crypto_register_akcipher
> fails.
> 
When crypto_register_akcipher fails, it merely prints a warning and falls
through (does not immediately return), thus there's no leak.

Thanks,
Horia

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input
  2019-05-23 10:02   ` Horia Geanta
@ 2019-05-23 12:38     ` Herbert Xu
  2019-05-24  6:52       ` Horia Geanta
  0 siblings, 1 reply; 10+ messages in thread
From: Herbert Xu @ 2019-05-23 12:38 UTC (permalink / raw)
  To: Horia Geanta
  Cc: Iuliana Prodan, Aymen Sghaier, David S. Miller, linux-crypto,
	linux-kernel, dl-linux-imx

On Thu, May 23, 2019 at 10:02:41AM +0000, Horia Geanta wrote:
>
> When crypto_register_akcipher fails, it merely prints a warning and falls
> through (does not immediately return), thus there's no leak.

How can this work? Wouldn't the exit path then unregister a bunch of
unregistered algorithms and crash?

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input
  2019-05-23 12:38     ` Herbert Xu
@ 2019-05-24  6:52       ` Horia Geanta
  0 siblings, 0 replies; 10+ messages in thread
From: Horia Geanta @ 2019-05-24  6:52 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Iuliana Prodan, Aymen Sghaier, David S. Miller, linux-crypto,
	linux-kernel, dl-linux-imx

On 5/23/2019 3:38 PM, Herbert Xu wrote:
> On Thu, May 23, 2019 at 10:02:41AM +0000, Horia Geanta wrote:
>>
>> When crypto_register_akcipher fails, it merely prints a warning and falls
>> through (does not immediately return), thus there's no leak.
> 
> How can this work? Wouldn't the exit path then unregister a bunch of
> unregistered algorithms and crash?
> 
You're actually right.
zero_buffer is leaked in case crypto_register_akcipher fails.

Besides this, there is an existing issue (independent of current patch) with
algorithm registration: algorithms (in fact, rsa) are unregistered even if
registration might have failed.
This should be addressed separately.

Thanks,
Horia

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-05-24  6:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-15 11:25 [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input Iuliana Prodan
2019-05-15 11:25 ` [PATCH v2 2/2] crypto: caam - strip input without changing crypto request Iuliana Prodan
2019-05-15 12:59   ` Horia Geanta
2019-05-15 14:00     ` Iuliana Prodan
2019-05-15 12:45 ` [PATCH v2 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input Horia Geanta
2019-05-23  6:12 ` Herbert Xu
2019-05-23  8:08   ` Iuliana Prodan
2019-05-23 10:02   ` Horia Geanta
2019-05-23 12:38     ` Herbert Xu
2019-05-24  6:52       ` Horia Geanta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).