All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] crypto: rsa-pkcs1pad - Fix akcipher request allocation
@ 2016-07-14 10:25 Salvatore Benedetto
  2016-07-15  3:39 ` Tadeusz Struk
  0 siblings, 1 reply; 3+ messages in thread
From: Salvatore Benedetto @ 2016-07-14 10:25 UTC (permalink / raw)
  To: herbert, davem, andrew.zaborowski; +Cc: linux-crypto, Salvatore Benedetto

Embedding the akcipher_request in pkcs1pad_request don't take
into account the context space required by the akcipher object.
This result in memory corruption (and kernel panic) as the
akcipher object will overwrite anything after akcipher_request
structure.

Fix it by dinamically allocating the structure with akcipher_request_alloc.

Software akcipher implementation is working only because it is
synchronous and it does not use a request context.

Signed-off-by: Salvatore Benedetto <salvatore.benedetto@intel.com>
---
 crypto/rsa-pkcs1pad.c | 75 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 24 deletions(-)

diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index 880d3db..d663ab6 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -101,7 +101,7 @@ struct pkcs1pad_inst_ctx {
 };
 
 struct pkcs1pad_request {
-	struct akcipher_request child_req;
+	struct akcipher_request *child_req;
 
 	struct scatterlist in_sg[2], out_sg[1];
 	uint8_t *in_buf, *out_buf;
@@ -192,7 +192,7 @@ static int pkcs1pad_encrypt_sign_complete(struct akcipher_request *req, int err)
 	if (err)
 		goto out;
 
-	len = req_ctx->child_req.dst_len;
+	len = req_ctx->child_req->dst_len;
 	pad_len = ctx->key_size - len;
 
 	/* Four billion to one */
@@ -215,6 +215,7 @@ out:
 	req->dst_len = ctx->key_size;
 
 	kfree(req_ctx->in_buf);
+	akcipher_request_free(req_ctx->child_req);
 
 	return err;
 }
@@ -277,15 +278,21 @@ static int pkcs1pad_encrypt(struct akcipher_request *req)
 	pkcs1pad_sg_set_buf(req_ctx->out_sg, req_ctx->out_buf,
 			ctx->key_size, NULL);
 
-	akcipher_request_set_tfm(&req_ctx->child_req, ctx->child);
-	akcipher_request_set_callback(&req_ctx->child_req, req->base.flags,
+	req_ctx->child_req = akcipher_request_alloc(ctx->child, GFP_KERNEL);
+	if (!req_ctx->child_req) {
+		kfree(req_ctx->out_buf);
+		kfree(req_ctx->in_buf);
+		return -ENOMEM;
+	}
+	akcipher_request_set_tfm(req_ctx->child_req, ctx->child);
+	akcipher_request_set_callback(req_ctx->child_req, req->base.flags,
 			pkcs1pad_encrypt_sign_complete_cb, req);
 
 	/* Reuse output buffer */
-	akcipher_request_set_crypt(&req_ctx->child_req, req_ctx->in_sg,
+	akcipher_request_set_crypt(req_ctx->child_req, req_ctx->in_sg,
 				   req->dst, ctx->key_size - 1, req->dst_len);
 
-	err = crypto_akcipher_encrypt(&req_ctx->child_req);
+	err = crypto_akcipher_encrypt(req_ctx->child_req);
 	if (err != -EINPROGRESS &&
 			(err != -EBUSY ||
 			 !(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)))
@@ -308,7 +315,7 @@ static int pkcs1pad_decrypt_complete(struct akcipher_request *req, int err)
 	if (err)
 		goto done;
 
-	if (req_ctx->child_req.dst_len != ctx->key_size - 1) {
+	if (req_ctx->child_req->dst_len != ctx->key_size - 1) {
 		err = -EINVAL;
 		goto done;
 	}
@@ -317,18 +324,18 @@ static int pkcs1pad_decrypt_complete(struct akcipher_request *req, int err)
 		err = -EINVAL;
 		goto done;
 	}
-	for (pos = 1; pos < req_ctx->child_req.dst_len; pos++)
+	for (pos = 1; pos < req_ctx->child_req->dst_len; pos++)
 		if (req_ctx->out_buf[pos] == 0x00)
 			break;
-	if (pos < 9 || pos == req_ctx->child_req.dst_len) {
+	if (pos < 9 || pos == req_ctx->child_req->dst_len) {
 		err = -EINVAL;
 		goto done;
 	}
 	pos++;
 
-	if (req->dst_len < req_ctx->child_req.dst_len - pos)
+	if (req->dst_len < req_ctx->child_req->dst_len - pos)
 		err = -EOVERFLOW;
-	req->dst_len = req_ctx->child_req.dst_len - pos;
+	req->dst_len = req_ctx->child_req->dst_len - pos;
 
 	if (!err)
 		sg_copy_from_buffer(req->dst,
@@ -337,6 +344,7 @@ static int pkcs1pad_decrypt_complete(struct akcipher_request *req, int err)
 
 done:
 	kzfree(req_ctx->out_buf);
+	akcipher_request_free(req_ctx->child_req);
 
 	return err;
 }
@@ -373,16 +381,22 @@ static int pkcs1pad_decrypt(struct akcipher_request *req)
 	pkcs1pad_sg_set_buf(req_ctx->out_sg, req_ctx->out_buf,
 			    ctx->key_size, NULL);
 
-	akcipher_request_set_tfm(&req_ctx->child_req, ctx->child);
-	akcipher_request_set_callback(&req_ctx->child_req, req->base.flags,
+	req_ctx->child_req = akcipher_request_alloc(ctx->child, GFP_KERNEL);
+	if (!req_ctx->child_req) {
+		kfree(req_ctx->out_buf);
+		return -ENOMEM;
+	}
+
+	akcipher_request_set_tfm(req_ctx->child_req, ctx->child);
+	akcipher_request_set_callback(req_ctx->child_req, req->base.flags,
 			pkcs1pad_decrypt_complete_cb, req);
 
 	/* Reuse input buffer, output to a new buffer */
-	akcipher_request_set_crypt(&req_ctx->child_req, req->src,
+	akcipher_request_set_crypt(req_ctx->child_req, req->src,
 				   req_ctx->out_sg, req->src_len,
 				   ctx->key_size);
 
-	err = crypto_akcipher_decrypt(&req_ctx->child_req);
+	err = crypto_akcipher_decrypt(req_ctx->child_req);
 	if (err != -EINPROGRESS &&
 			(err != -EBUSY ||
 			 !(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)))
@@ -420,6 +434,12 @@ static int pkcs1pad_sign(struct akcipher_request *req)
 	if (!req_ctx->in_buf)
 		return -ENOMEM;
 
+	req_ctx->child_req = akcipher_request_alloc(ctx->child, GFP_KERNEL);
+	if (!req_ctx->child_req) {
+		kfree(req_ctx->in_buf);
+		return -ENOMEM;
+	}
+
 	ps_end = ctx->key_size - digest_size - req->src_len - 2;
 	req_ctx->in_buf[0] = 0x01;
 	memset(req_ctx->in_buf + 1, 0xff, ps_end - 1);
@@ -431,15 +451,15 @@ static int pkcs1pad_sign(struct akcipher_request *req)
 	pkcs1pad_sg_set_buf(req_ctx->in_sg, req_ctx->in_buf,
 			ctx->key_size - 1 - req->src_len, req->src);
 
-	akcipher_request_set_tfm(&req_ctx->child_req, ctx->child);
-	akcipher_request_set_callback(&req_ctx->child_req, req->base.flags,
+	akcipher_request_set_tfm(req_ctx->child_req, ctx->child);
+	akcipher_request_set_callback(req_ctx->child_req, req->base.flags,
 			pkcs1pad_encrypt_sign_complete_cb, req);
 
 	/* Reuse output buffer */
-	akcipher_request_set_crypt(&req_ctx->child_req, req_ctx->in_sg,
+	akcipher_request_set_crypt(req_ctx->child_req, req_ctx->in_sg,
 				   req->dst, ctx->key_size - 1, req->dst_len);
 
-	err = crypto_akcipher_sign(&req_ctx->child_req);
+	err = crypto_akcipher_sign(req_ctx->child_req);
 	if (err != -EINPROGRESS &&
 			(err != -EBUSY ||
 			 !(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)))
@@ -464,7 +484,7 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err)
 		goto done;
 
 	err = -EINVAL;
-	dst_len = req_ctx->child_req.dst_len;
+	dst_len = req_ctx->child_req->dst_len;
 	if (dst_len < ctx->key_size - 1)
 		goto done;
 
@@ -507,6 +527,7 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err)
 				out_buf + pos, req->dst_len);
 done:
 	kzfree(req_ctx->out_buf);
+	akcipher_request_free(req_ctx->child_req);
 
 	return err;
 }
@@ -551,16 +572,22 @@ static int pkcs1pad_verify(struct akcipher_request *req)
 	pkcs1pad_sg_set_buf(req_ctx->out_sg, req_ctx->out_buf,
 			    ctx->key_size, NULL);
 
-	akcipher_request_set_tfm(&req_ctx->child_req, ctx->child);
-	akcipher_request_set_callback(&req_ctx->child_req, req->base.flags,
+	req_ctx->child_req = akcipher_request_alloc(ctx->child, GFP_KERNEL);
+	if (!req_ctx->child_req) {
+		kfree(req_ctx->out_buf);
+		return -ENOMEM;
+	}
+
+	akcipher_request_set_tfm(req_ctx->child_req, ctx->child);
+	akcipher_request_set_callback(req_ctx->child_req, req->base.flags,
 			pkcs1pad_verify_complete_cb, req);
 
 	/* Reuse input buffer, output to a new buffer */
-	akcipher_request_set_crypt(&req_ctx->child_req, req->src,
+	akcipher_request_set_crypt(req_ctx->child_req, req->src,
 				   req_ctx->out_sg, req->src_len,
 				   ctx->key_size);
 
-	err = crypto_akcipher_verify(&req_ctx->child_req);
+	err = crypto_akcipher_verify(req_ctx->child_req);
 	if (err != -EINPROGRESS &&
 			(err != -EBUSY ||
 			 !(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)))
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] crypto: rsa-pkcs1pad - Fix akcipher request allocation
  2016-07-14 10:25 [PATCH] crypto: rsa-pkcs1pad - Fix akcipher request allocation Salvatore Benedetto
@ 2016-07-15  3:39 ` Tadeusz Struk
  2016-07-18 10:23   ` Herbert Xu
  0 siblings, 1 reply; 3+ messages in thread
From: Tadeusz Struk @ 2016-07-15  3:39 UTC (permalink / raw)
  To: Salvatore Benedetto, herbert, davem, andrew.zaborowski; +Cc: linux-crypto

Hi Salvatore,
On 07/14/2016 03:25 AM, Salvatore Benedetto wrote:
> Embedding the akcipher_request in pkcs1pad_request don't take
> into account the context space required by the akcipher object.

I think we do take into account the sub request context. See line 675.
The only thing that is wrong is that the child_req should be at
the end of the structure. This is build tested only.

---8<---
From: Tadeusz Struk <tadeusz.struk@intel.com>
Subject: [PATCH] crypto: rsa-pkcs1pad - fix rsa-pkcs1pad request struct

To allow for child request context the struct akcipher_request child_req
needs to be at the end of the structure.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 crypto/rsa-pkcs1pad.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index 880d3db..877019a 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -101,10 +101,9 @@ struct pkcs1pad_inst_ctx {
 };
 
 struct pkcs1pad_request {
-	struct akcipher_request child_req;
-
 	struct scatterlist in_sg[2], out_sg[1];
 	uint8_t *in_buf, *out_buf;
+	struct akcipher_request child_req;
 };
 
 static int pkcs1pad_set_pub_key(struct crypto_akcipher *tfm, const void *key,

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] crypto: rsa-pkcs1pad - Fix akcipher request allocation
  2016-07-15  3:39 ` Tadeusz Struk
@ 2016-07-18 10:23   ` Herbert Xu
  0 siblings, 0 replies; 3+ messages in thread
From: Herbert Xu @ 2016-07-18 10:23 UTC (permalink / raw)
  To: Tadeusz Struk; +Cc: Salvatore Benedetto, davem, andrew.zaborowski, linux-crypto

On Thu, Jul 14, 2016 at 08:39:18PM -0700, Tadeusz Struk wrote:
> Hi Salvatore,
> On 07/14/2016 03:25 AM, Salvatore Benedetto wrote:
> > Embedding the akcipher_request in pkcs1pad_request don't take
> > into account the context space required by the akcipher object.
> 
> I think we do take into account the sub request context. See line 675.
> The only thing that is wrong is that the child_req should be at
> the end of the structure. This is build tested only.
> 
> ---8<---
> From: Tadeusz Struk <tadeusz.struk@intel.com>
> Subject: [PATCH] crypto: rsa-pkcs1pad - fix rsa-pkcs1pad request struct
> 
> To allow for child request context the struct akcipher_request child_req
> needs to be at the end of the structure.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-07-18 10:23 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-14 10:25 [PATCH] crypto: rsa-pkcs1pad - Fix akcipher request allocation Salvatore Benedetto
2016-07-15  3:39 ` Tadeusz Struk
2016-07-18 10:23   ` Herbert Xu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.