Patchwork [RESEND] crypto: caam - strip input zeros from RSA input buffer

login
register
mail settings
Submitter Horia Geantă
Date April 16, 2018, 1:07 p.m.
Message ID <20180416130705.28365-1-horia.geanta@nxp.com>
Download mbox | patch
Permalink /patch/10342957/
State Accepted
Delegated to: Herbert Xu
Headers show

Comments

Horia Geantă - April 16, 2018, 1:07 p.m.
Sometimes the provided RSA input buffer provided is not stripped
of leading zeros. This could cause its size to be bigger than that
of the modulus, making the HW complain:

caam_jr 2142000.jr1: 40000789: DECO: desc idx 7:
Protocol Size Error - A protocol has seen an error in size. When
running RSA, pdb size N < (size of F) when no formatting is used; or
pdb size N < (F + 11) when formatting is used.

Fix the problem by stripping off the leading zero from input data
before feeding it to the CAAM accelerator.

Fixes: 8c419778ab57e ("crypto: caam - add support for RSA algorithm")
Cc: <stable@vger.kernel.org> # 4.8+
Reported-by: Martin Townsend <mtownsend1973@gmail.com>
Link: https://lkml.kernel.org/r/CABatt_ytYORYKtApcB4izhNanEKkGFi9XAQMjHi_n-8YWoCRiw@mail.gmail.com
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
(Hopefully this one will reach the mailing list.
Sorry for the noise, problems with SMTP.)

 drivers/crypto/caam/caampkc.c | 54 +++++++++++++++++++++++++++++++++++++++++++
 drivers/crypto/caam/caampkc.h |  8 +++++++
 2 files changed, 62 insertions(+)
Fabio Estevam - April 16, 2018, 1:56 p.m.
On Mon, Apr 16, 2018 at 10:07 AM, Horia Geantă <horia.geanta@nxp.com> wrote:
> Sometimes the provided RSA input buffer provided is not stripped
> of leading zeros. This could cause its size to be bigger than that
> of the modulus, making the HW complain:
>
> caam_jr 2142000.jr1: 40000789: DECO: desc idx 7:
> Protocol Size Error - A protocol has seen an error in size. When
> running RSA, pdb size N < (size of F) when no formatting is used; or
> pdb size N < (F + 11) when formatting is used.
>
> Fix the problem by stripping off the leading zero from input data
> before feeding it to the CAAM accelerator.
>
> Fixes: 8c419778ab57e ("crypto: caam - add support for RSA algorithm")
> Cc: <stable@vger.kernel.org> # 4.8+
> Reported-by: Martin Townsend <mtownsend1973@gmail.com>
> Link: https://lkml.kernel.org/r/CABatt_ytYORYKtApcB4izhNanEKkGFi9XAQMjHi_n-8YWoCRiw@mail.gmail.com
> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>

Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
Tudor-Dan Ambarus - April 16, 2018, 3:44 p.m.
On 04/16/2018 04:07 PM, Horia Geantă wrote:
> Sometimes the provided RSA input buffer provided is not stripped
> of leading zeros. This could cause its size to be bigger than that
> of the modulus, making the HW complain:
> 
> caam_jr 2142000.jr1: 40000789: DECO: desc idx 7:
> Protocol Size Error - A protocol has seen an error in size. When
> running RSA, pdb size N < (size of F) when no formatting is used; or
> pdb size N < (F + 11) when formatting is used.
> 
> Fix the problem by stripping off the leading zero from input data
> before feeding it to the CAAM accelerator.
> 
> Fixes: 8c419778ab57e ("crypto: caam - add support for RSA algorithm")
> Cc: <stable@vger.kernel.org> # 4.8+
> Reported-by: Martin Townsend <mtownsend1973@gmail.com>
> Link: https://lkml.kernel.org/r/CABatt_ytYORYKtApcB4izhNanEKkGFi9XAQMjHi_n-8YWoCRiw@mail.gmail.com
> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
> ---
> (Hopefully this one will reach the mailing list.
> Sorry for the noise, problems with SMTP.)
> 
>   drivers/crypto/caam/caampkc.c | 54 +++++++++++++++++++++++++++++++++++++++++++
>   drivers/crypto/caam/caampkc.h |  8 +++++++
>   2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
> index 7a897209f181..979072b25eaa 100644
> --- a/drivers/crypto/caam/caampkc.c
> +++ b/drivers/crypto/caam/caampkc.c
> @@ -166,18 +166,71 @@ static void rsa_priv_f3_done(struct device *dev, u32 *desc, u32 err,
>   	akcipher_request_complete(req, err);
>   }
>   
> +static int caam_rsa_count_leading_zeros(struct scatterlist *sgl,
> +					unsigned int nbytes,
> +					unsigned int flags)
> +{
> +	struct sg_mapping_iter miter;
> +	int lzeros, ents;
> +	unsigned int len;
> +	unsigned int tbytes = nbytes;
> +	const u8 *buff;
> +
> +	ents = sg_nents_for_len(sgl, nbytes);
> +	if (ents < 0)
> +		return ents;
> +
> +	sg_miter_start(&miter, sgl, ents, SG_MITER_FROM_SG | flags);
> +
> +	lzeros = 0;
> +	len = 0;
> +	while (nbytes > 0) {
> +		while (len && !*buff) {
> +			lzeros++;
> +			len--;
> +			buff++;
> +		}
> +
> +		if (len && *buff)
> +			break;
> +
> +		sg_miter_next(&miter);
> +		buff = miter.addr;
> +		len = miter.length;
> +
> +		nbytes -= lzeros;
> +		lzeros = 0;
> +	}
> +
> +	miter.consumed = lzeros;
> +	sg_miter_stop(&miter);
> +	nbytes -= lzeros;
> +
> +	return tbytes - nbytes;
> +}
> +
>   static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
>   					 size_t desclen)
>   {
>   	struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(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 rsa_edesc *edesc;
>   	gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
>   		       GFP_KERNEL : GFP_ATOMIC;
> +	int sg_flags = (flags == GFP_ATOMIC) ? SG_MITER_ATOMIC : 0;
>   	int sgc;
>   	int sec4_sg_index, sec4_sg_len = 0, sec4_sg_bytes;
>   	int src_nents, dst_nents;
> +	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);

This is interesting, you are overwriting the request's src sg. I kept
wondering if one could have a problem if we are modifying its src sg. I
couldn't find any failing scenario, so:

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Patch

diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
index 7a897209f181..979072b25eaa 100644
--- a/drivers/crypto/caam/caampkc.c
+++ b/drivers/crypto/caam/caampkc.c
@@ -166,18 +166,71 @@  static void rsa_priv_f3_done(struct device *dev, u32 *desc, u32 err,
 	akcipher_request_complete(req, err);
 }
 
+static int caam_rsa_count_leading_zeros(struct scatterlist *sgl,
+					unsigned int nbytes,
+					unsigned int flags)
+{
+	struct sg_mapping_iter miter;
+	int lzeros, ents;
+	unsigned int len;
+	unsigned int tbytes = nbytes;
+	const u8 *buff;
+
+	ents = sg_nents_for_len(sgl, nbytes);
+	if (ents < 0)
+		return ents;
+
+	sg_miter_start(&miter, sgl, ents, SG_MITER_FROM_SG | flags);
+
+	lzeros = 0;
+	len = 0;
+	while (nbytes > 0) {
+		while (len && !*buff) {
+			lzeros++;
+			len--;
+			buff++;
+		}
+
+		if (len && *buff)
+			break;
+
+		sg_miter_next(&miter);
+		buff = miter.addr;
+		len = miter.length;
+
+		nbytes -= lzeros;
+		lzeros = 0;
+	}
+
+	miter.consumed = lzeros;
+	sg_miter_stop(&miter);
+	nbytes -= lzeros;
+
+	return tbytes - nbytes;
+}
+
 static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
 					 size_t desclen)
 {
 	struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(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 rsa_edesc *edesc;
 	gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
 		       GFP_KERNEL : GFP_ATOMIC;
+	int sg_flags = (flags == GFP_ATOMIC) ? SG_MITER_ATOMIC : 0;
 	int sgc;
 	int sec4_sg_index, sec4_sg_len = 0, sec4_sg_bytes;
 	int src_nents, dst_nents;
+	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);
 
 	src_nents = sg_nents_for_len(req->src, req->src_len);
 	dst_nents = sg_nents_for_len(req->dst, req->dst_len);
@@ -953,6 +1006,7 @@  static struct akcipher_alg caam_rsa = {
 	.max_size = caam_rsa_max_size,
 	.init = caam_rsa_init_tfm,
 	.exit = caam_rsa_exit_tfm,
+	.reqsize = sizeof(struct caam_rsa_req_ctx),
 	.base = {
 		.cra_name = "rsa",
 		.cra_driver_name = "rsa-caam",
diff --git a/drivers/crypto/caam/caampkc.h b/drivers/crypto/caam/caampkc.h
index fd145c46eae1..82645bcf8b27 100644
--- a/drivers/crypto/caam/caampkc.h
+++ b/drivers/crypto/caam/caampkc.h
@@ -95,6 +95,14 @@  struct caam_rsa_ctx {
 	struct device *dev;
 };
 
+/**
+ * caam_rsa_req_ctx - per request context.
+ * @src: input scatterlist (stripped of leading zeros)
+ */
+struct caam_rsa_req_ctx {
+	struct scatterlist src[2];
+};
+
 /**
  * rsa_edesc - s/w-extended rsa descriptor
  * @src_nents     : number of segments in input scatterlist