[PATCH 1/3 V4] crypto: Fix the pointer voodoo in unaligned ahash

[PATCH 1/3 V4] crypto: Fix the pointer voodoo in unaligned ahash

[Marek Vasut]

Add documentation for the pointer voodoo that is happening in crypto/ahash.c
in ahash_op_unaligned(). This code is quite confusing, so add a beefy chunk
of documentation.

Moreover, make sure the mangled request is completely restored after finishing
this unaligned operation. This means restoring all of .result, .base.data
and .base.complete .

Also, remove the crypto_completion_t complete = ... line present in the
ahash_op_unaligned_done() function. This type actually declares a function
pointer, which is very confusing.

Finally, yet very important nonetheless, make sure the req->priv is free()'d
only after the original request is restored in ahash_op_unaligned_done().
The req->priv data must not be free()'d before that in ahash_op_unaligned_finish(),
since we would be accessing previously free()'d data in ahash_op_unaligned_done()
and cause corruption.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
---
 crypto/ahash.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 49 insertions(+), 7 deletions(-)

V2: Move the restoration of the ORIGINAL crypto request fields from
    ahash_op_unaligned_done() to ahash_op_unaligned_finish(). This way,
    we handle both Sync-HASH and Async-HASH cases properly:
     SYNC:  ahash_op_unaligned_finish() is called with err=0 . Data are
            copied from ADJUSTED request to ORIGINAL request, ORIGINAL
	    request is correctly restored and free'd.
     ASYNC: ahash_op_unaligned_finish() is called with err=-EINPROGRESS,
            returns. Later, ahash_op_unaligned_finish() is called again
	    from ahash_op_unaligned_done() callback. Data are copied
	    from ADJUSTED request to ORIGINAL request, ORIGINAL request
	    is correctly restored and free'd.
V3: Swap the names of req<->areq variables to be consistent with the rest
    of the crypto API again.
V4: Do not save (struct ahash_request).priv when adjusting the request,
    the .priv field is internal to Crypto API and the user must NOT EVER
    use it. Add a comment about this.

diff --git a/crypto/ahash.c b/crypto/ahash.c
index a92dc38..4fdb4d3 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -201,22 +201,34 @@ static void ahash_op_unaligned_finish(struct ahash_request *req, int err)
 		memcpy(priv->result, req->result,
 		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
 
+	/* Restore the original crypto request. */
+	req->result = priv->result;
+	req->base.complete = priv->complete;
+	req->base.data = priv->data;
+	req->priv = NULL;
+
+	/* Free the req->priv.priv from the ADJUSTED request. */
 	kzfree(priv);
 }
 
 static void ahash_op_unaligned_done(struct crypto_async_request *req, int err)
 {
 	struct ahash_request *areq = req->data;
-	struct ahash_request_priv *priv = areq->priv;
-	crypto_completion_t complete = priv->complete;
-	void *data = priv->data;
 
-	ahash_op_unaligned_finish(areq, err);
+	/*
+	 * Restore the original request, see ahash_op_unaligned() for what
+	 * goes where.
+	 *
+	 * The "struct ahash_request *req" here is in fact the "req.base"
+	 * from the ADJUSTED request from ahash_op_unaligned(), thus as it
+	 * is a pointer to self, it is also the ADJUSTED "req" .
+	 */
 
-	areq->base.complete = complete;
-	areq->base.data = data;
+	/* First copy areq->result into areq->priv.result */
+	ahash_op_unaligned_finish(areq, err);
 
-	complete(&areq->base, err);
+	/* Complete the ORIGINAL request. */
+	areq->base.complete(&areq->base, err);
 }
 
 static int ahash_op_unaligned(struct ahash_request *req,
@@ -234,9 +246,39 @@ static int ahash_op_unaligned(struct ahash_request *req,
 	if (!priv)
 		return -ENOMEM;
 
+	/*
+	 * WARNING: Voodoo programming below!
+	 *
+	 * The code below is obscure and hard to understand, thus explanation
+	 * is necessary. See include/crypto/hash.h and include/linux/crypto.h
+	 * to understand the layout of structures used here!
+	 *
+	 * The code here will replace portions of the ORIGINAL request with
+	 * pointers to new code and buffers so the hashing operation can store
+	 * the result in aligned buffer. We will call the modified request
+	 * an ADJUSTED request.
+	 *
+	 * The newly mangled request will look as such:
+	 *
+	 * req {
+	 *   .result        = ADJUSTED[new aligned buffer]
+	 *   .base.complete = ADJUSTED[pointer to completion function]
+	 *   .base.data     = ADJUSTED[*req (pointer to self)]
+	 *   .priv          = ADJUSTED[new priv] {
+	 *           .result   = ORIGINAL(result)
+	 *           .complete = ORIGINAL(base.complete)
+	 *           .data     = ORIGINAL(base.data)
+	 *   }
+	 */
+
 	priv->result = req->result;
 	priv->complete = req->base.complete;
 	priv->data = req->base.data;
+	/*
+	 * WARNING: We do not backup req->priv here! The req->priv
+	 *          is for internal use of the Crypto API and the
+	 *          user must _NOT_ _EVER_ depend on it's content!
+	 */
 
 	req->result = PTR_ALIGN((u8 *)priv->ubuf, alignmask + 1);
 	req->base.complete = ahash_op_unaligned_done;
-- 
1.8.5.2

[PATCH 2/3 V4] crypto: Pull out the functions to save/restore request

[Marek Vasut]

The functions to save original request within a newly adjusted request
and it's counterpart to restore the original request can be re-used by
more code in the crypto/ahash.c file. Pull these functions out from the
code so they're available.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
---
 crypto/ahash.c | 107 +++++++++++++++++++++++++++++++++------------------------
 1 file changed, 62 insertions(+), 45 deletions(-)

V2: Rebase on top of 1/3 V2 , no functional changes.
V3: Rebase on top of 1/3 V3 , no functional changes.
V4: Rebase on top of 1/3 V4 , no functional changes.

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 4fdb4d3..fcc6077 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -190,55 +190,12 @@ static inline unsigned int ahash_align_buffer_size(unsigned len,
 	return len + (mask & ~(crypto_tfm_ctx_alignment() - 1));
 }
 
-static void ahash_op_unaligned_finish(struct ahash_request *req, int err)
-{
-	struct ahash_request_priv *priv = req->priv;
-
-	if (err == -EINPROGRESS)
-		return;
-
-	if (!err)
-		memcpy(priv->result, req->result,
-		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
-
-	/* Restore the original crypto request. */
-	req->result = priv->result;
-	req->base.complete = priv->complete;
-	req->base.data = priv->data;
-	req->priv = NULL;
-
-	/* Free the req->priv.priv from the ADJUSTED request. */
-	kzfree(priv);
-}
-
-static void ahash_op_unaligned_done(struct crypto_async_request *req, int err)
-{
-	struct ahash_request *areq = req->data;
-
-	/*
-	 * Restore the original request, see ahash_op_unaligned() for what
-	 * goes where.
-	 *
-	 * The "struct ahash_request *req" here is in fact the "req.base"
-	 * from the ADJUSTED request from ahash_op_unaligned(), thus as it
-	 * is a pointer to self, it is also the ADJUSTED "req" .
-	 */
-
-	/* First copy areq->result into areq->priv.result */
-	ahash_op_unaligned_finish(areq, err);
-
-	/* Complete the ORIGINAL request. */
-	areq->base.complete(&areq->base, err);
-}
-
-static int ahash_op_unaligned(struct ahash_request *req,
-			      int (*op)(struct ahash_request *))
+static int ahash_save_req(struct ahash_request *req, crypto_completion_t cplt)
 {
 	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
 	unsigned long alignmask = crypto_ahash_alignmask(tfm);
 	unsigned int ds = crypto_ahash_digestsize(tfm);
 	struct ahash_request_priv *priv;
-	int err;
 
 	priv = kmalloc(sizeof(*priv) + ahash_align_buffer_size(ds, alignmask),
 		       (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
@@ -281,10 +238,70 @@ static int ahash_op_unaligned(struct ahash_request *req,
 	 */
 
 	req->result = PTR_ALIGN((u8 *)priv->ubuf, alignmask + 1);
-	req->base.complete = ahash_op_unaligned_done;
+	req->base.complete = cplt;
 	req->base.data = req;
 	req->priv = priv;
 
+	return 0;
+}
+
+static void ahash_restore_req(struct ahash_request *req)
+{
+	struct ahash_request_priv *priv = req->priv;
+
+	/* Restore the original crypto request. */
+	req->result = priv->result;
+	req->base.complete = priv->complete;
+	req->base.data = priv->data;
+	req->priv = NULL;
+
+	/* Free the req->priv.priv from the ADJUSTED request. */
+	kzfree(priv);
+}
+
+static void ahash_op_unaligned_finish(struct ahash_request *req, int err)
+{
+	struct ahash_request_priv *priv = req->priv;
+
+	if (err == -EINPROGRESS)
+		return;
+
+	if (!err)
+		memcpy(priv->result, req->result,
+		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
+
+	ahash_restore_req(req);
+}
+
+static void ahash_op_unaligned_done(struct crypto_async_request *req, int err)
+{
+	struct ahash_request *areq = req->data;
+
+	/*
+	 * Restore the original request, see ahash_op_unaligned() for what
+	 * goes where.
+	 *
+	 * The "struct ahash_request *req" here is in fact the "req.base"
+	 * from the ADJUSTED request from ahash_op_unaligned(), thus as it
+	 * is a pointer to self, it is also the ADJUSTED "req" .
+	 */
+
+	/* First copy req->result into req->priv.result */
+	ahash_op_unaligned_finish(areq, err);
+
+	/* Complete the ORIGINAL request. */
+	areq->base.complete(&areq->base, err);
+}
+
+static int ahash_op_unaligned(struct ahash_request *req,
+			      int (*op)(struct ahash_request *))
+{
+	int err;
+
+	err = ahash_save_req(req, ahash_op_unaligned_done);
+	if (err)
+		return err;
+
 	err = op(req);
 	ahash_op_unaligned_finish(req, err);
 
-- 
1.8.5.2

[PATCH 3/3 V4] crypto: Simplify the ahash_finup implementation

[Marek Vasut]

The ahash_def_finup() can make use of the request save/restore functions,
thus make it so. This simplifies the code a little and unifies the code
paths.

Note that the same remark about free()ing the req->priv applies here, the
req->priv can only be free()'d after the original request was restored.

Finally, squash a bug in the invocation of completion in the ASYNC path.
In both ahash_def_finup_done{1,2}, the function areq->base.complete(X, err);
was called with X=areq->base.data . This is incorrect , as X=&areq->base
is the correct value. By analysis of the data structures, we see the areq is
of type 'struct ahash_request' , areq->base is of type 'struct crypto_async_request'
and areq->base.completion is of type crypto_completion_t, which is defined in
include/linux/crypto.h as:

  typedef void (*crypto_completion_t)(struct crypto_async_request *req, int err);

This is one lead that the X should be &areq->base . Next up, we can inspect
other code which calls the completion callback to give us kind-of statistical
idea of how this callback is used. We can try:

  $ git grep base\.complete\( drivers/crypto/

Finally, by inspecting ahash_request_set_callback() implementation defined
in include/crypto/hash.h , we observe that the .data entry of 'struct
crypto_async_request' is intended for arbitrary data, not for completion
argument.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
---
 crypto/ahash.c | 36 +++++++++---------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

V2: Same as with 1/3 V2, move the restoration and freeing of the private
    data right past memcpy() of the buffer.
V3: - Swap the names of req<->areq variables to be consistent with the rest
      of the crypto API again.
    - Squash another bug in areq->base.complete(X, err); invocation, see the
      commit message, which was augmented with the last paragraph.
V4: Rebase, no functional change.

diff --git a/crypto/ahash.c b/crypto/ahash.c
index fcc6077..6e72233 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -349,19 +349,16 @@ static void ahash_def_finup_finish2(struct ahash_request *req, int err)
 		memcpy(priv->result, req->result,
 		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
 
-	kzfree(priv);
+	ahash_restore_req(req);
 }
 
 static void ahash_def_finup_done2(struct crypto_async_request *req, int err)
 {
 	struct ahash_request *areq = req->data;
-	struct ahash_request_priv *priv = areq->priv;
-	crypto_completion_t complete = priv->complete;
-	void *data = priv->data;
 
 	ahash_def_finup_finish2(areq, err);
 
-	complete(data, err);
+	areq->base.complete(&areq->base, err);
 }
 
 static int ahash_def_finup_finish1(struct ahash_request *req, int err)
@@ -381,38 +378,23 @@ out:
 static void ahash_def_finup_done1(struct crypto_async_request *req, int err)
 {
 	struct ahash_request *areq = req->data;
-	struct ahash_request_priv *priv = areq->priv;
-	crypto_completion_t complete = priv->complete;
-	void *data = priv->data;
 
 	err = ahash_def_finup_finish1(areq, err);
 
-	complete(data, err);
+	areq->base.complete(&areq->base, err);
 }
 
 static int ahash_def_finup(struct ahash_request *req)
 {
 	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
-	unsigned long alignmask = crypto_ahash_alignmask(tfm);
-	unsigned int ds = crypto_ahash_digestsize(tfm);
-	struct ahash_request_priv *priv;
-
-	priv = kmalloc(sizeof(*priv) + ahash_align_buffer_size(ds, alignmask),
-		       (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
-		       GFP_KERNEL : GFP_ATOMIC);
-	if (!priv)
-		return -ENOMEM;
-
-	priv->result = req->result;
-	priv->complete = req->base.complete;
-	priv->data = req->base.data;
+	int err;
 
-	req->result = PTR_ALIGN((u8 *)priv->ubuf, alignmask + 1);
-	req->base.complete = ahash_def_finup_done1;
-	req->base.data = req;
-	req->priv = priv;
+	err = ahash_save_req(req, ahash_def_finup_done1);
+	if (err)
+		return err;
 
-	return ahash_def_finup_finish1(req, tfm->update(req));
+	err = tfm->update(req);
+	return ahash_def_finup_finish1(req, err);
 }
 
 static int ahash_no_export(struct ahash_request *req, void *out)
-- 
1.8.5.2

[PATCH 1/3 V4] crypto: Fix the pointer voodoo in unaligned ahash

[Herbert Xu]

On Fri, Mar 14, 2014 at 02:37:04AM +0100, Marek Vasut wrote:
> Add documentation for the pointer voodoo that is happening in crypto/ahash.c
> in ahash_op_unaligned(). This code is quite confusing, so add a beefy chunk
> of documentation.
> 
> Moreover, make sure the mangled request is completely restored after finishing
> this unaligned operation. This means restoring all of .result, .base.data
> and .base.complete .
> 
> Also, remove the crypto_completion_t complete = ... line present in the
> ahash_op_unaligned_done() function. This type actually declares a function
> pointer, which is very confusing.
> 
> Finally, yet very important nonetheless, make sure the req->priv is free()'d
> only after the original request is restored in ahash_op_unaligned_done().
> The req->priv data must not be free()'d before that in ahash_op_unaligned_finish(),
> since we would be accessing previously free()'d data in ahash_op_unaligned_done()
> and cause corruption.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>

All applied.  Thanks Marek!
-- 
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

[PATCH 1/3 V4] crypto: Fix the pointer voodoo in unaligned ahash

[Marek Vasut]

On Friday, March 21, 2014 at 03:40:07 PM, Herbert Xu wrote:
> On Fri, Mar 14, 2014 at 02:37:04AM +0100, Marek Vasut wrote:
> > Add documentation for the pointer voodoo that is happening in
> > crypto/ahash.c in ahash_op_unaligned(). This code is quite confusing, so
> > add a beefy chunk of documentation.
> > 
> > Moreover, make sure the mangled request is completely restored after
> > finishing this unaligned operation. This means restoring all of .result,
> > .base.data and .base.complete .
> > 
> > Also, remove the crypto_completion_t complete = ... line present in the
> > ahash_op_unaligned_done() function. This type actually declares a
> > function pointer, which is very confusing.
> > 
> > Finally, yet very important nonetheless, make sure the req->priv is
> > free()'d only after the original request is restored in
> > ahash_op_unaligned_done(). The req->priv data must not be free()'d
> > before that in ahash_op_unaligned_finish(), since we would be accessing
> > previously free()'d data in ahash_op_unaligned_done() and cause
> > corruption.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> 
> All applied.  Thanks Marek!

Thank you for kicking me in the right direction all the time and thanks for your 
patience with me !

Best regards,
Marek Vasut