[PATCH 0/3] crypto: Clean up ahash handling confusion

[PATCH 0/3] crypto: Clean up ahash handling confusion

[Marek Vasut]

This set of patches shall clean up the confusion in restoring the ahash request
context in crypto/ahash.c . The code was a bit refactored to make it easier to
understand as well.

Please, make sure the code is well tested before applying. Also, please review
very thoroughly.

Marek Vasut (3):
  crypto: Fix the pointer voodoo in unaligned ahash
  crypto: Pull out the functions to save/restore request
  crypto: Simplify the ahash_finup implementation

 crypto/ahash.c | 172 ++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 110 insertions(+), 62 deletions(-)

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>
-- 
1.8.5.2

[PATCH 1/3] 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, .priv, .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 | 65 ++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 11 deletions(-)

diff --git a/crypto/ahash.c b/crypto/ahash.c
index a92dc38..5ca8ede 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -29,6 +29,7 @@
 struct ahash_request_priv {
 	crypto_completion_t complete;
 	void *data;
+	void *priv;
 	u8 *result;
 	void *ubuf[] CRYPTO_MINALIGN_ATTR;
 };
@@ -200,23 +201,38 @@ static void ahash_op_unaligned_finish(struct ahash_request *req, int err)
 	if (!err)
 		memcpy(priv->result, req->result,
 		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
-
-	kzfree(priv);
 }
 
-static void ahash_op_unaligned_done(struct crypto_async_request *req, int err)
+static void ahash_op_unaligned_done(struct crypto_async_request *areq, 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;
+	struct ahash_request *req = areq->data;
+	struct ahash_request_priv *priv = req->priv;
+	struct crypto_async_request *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(req, err);
 
-	ahash_op_unaligned_finish(areq, err);
+	/* Restore the original crypto request. */
+	req->result = priv->result;
+	req->base.complete = priv->complete;
+	req->base.data = priv->data;
+	req->priv = priv->priv;
 
-	areq->base.complete = complete;
-	areq->base.data = data;
+	/* Free the req->priv.priv from the ADJUSTED request. */
+	kzfree(priv);
 
-	complete(&areq->base, err);
+	/* Complete the ORIGINAL request. */
+	data = req->base.data;
+	req->base.complete(data, err);
 }
 
 static int ahash_op_unaligned(struct ahash_request *req,
@@ -234,9 +250,36 @@ 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     = ORIGINAL(priv)
+	 *   }
+	 */
+
 	priv->result = req->result;
 	priv->complete = req->base.complete;
 	priv->data = req->base.data;
+	priv->priv = req->priv;
 
 	req->result = PTR_ALIGN((u8 *)priv->ubuf, alignmask + 1);
 	req->base.complete = ahash_op_unaligned_done;
-- 
1.8.5.2

[PATCH 2/3] 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 | 112 +++++++++++++++++++++++++++++++++------------------------
 1 file changed, 65 insertions(+), 47 deletions(-)

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 5ca8ede..635cd49 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -191,58 +191,14 @@ 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)));
-}
+static void ahash_op_unaligned_done(struct crypto_async_request *areq, int err);
 
-static void ahash_op_unaligned_done(struct crypto_async_request *areq, int err)
-{
-	struct ahash_request *req = areq->data;
-	struct ahash_request_priv *priv = req->priv;
-	struct crypto_async_request *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(req, err);
-
-	/* Restore the original crypto request. */
-	req->result = priv->result;
-	req->base.complete = priv->complete;
-	req->base.data = priv->data;
-	req->priv = priv->priv;
-
-	/* Free the req->priv.priv from the ADJUSTED request. */
-	kzfree(priv);
-
-	/* Complete the ORIGINAL request. */
-	data = req->base.data;
-	req->base.complete(data, 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) ?
@@ -282,10 +238,72 @@ static int ahash_op_unaligned(struct ahash_request *req,
 	priv->priv = req->priv;
 
 	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 = priv->priv;
+
+	/* 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)));
+}
+
+static void ahash_op_unaligned_done(struct crypto_async_request *areq, int err)
+{
+	struct ahash_request *req = areq->data;
+	struct crypto_async_request *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(req, err);
+
+	ahash_restore_req(req);
+
+	/* Complete the ORIGINAL request. */
+	data = req->base.data;
+	req->base.complete(data, 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] 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.

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 | 53 ++++++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 635cd49..561ebaf 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -350,20 +350,19 @@ static void ahash_def_finup_finish2(struct ahash_request *req, int err)
 	if (!err)
 		memcpy(priv->result, req->result,
 		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
-
-	kzfree(priv);
 }
 
-static void ahash_def_finup_done2(struct crypto_async_request *req, int err)
+static void ahash_def_finup_done2(struct crypto_async_request *areq, 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;
+	struct ahash_request *req = areq->data;
+	struct crypto_async_request *data;
 
-	ahash_def_finup_finish2(areq, err);
+	ahash_def_finup_finish2(req, err);
 
-	complete(data, err);
+	ahash_restore_req(req);
+
+	data = req->base.data;
+	req->base.complete(data, err);
 }
 
 static int ahash_def_finup_finish1(struct ahash_request *req, int err)
@@ -380,39 +379,27 @@ out:
 	return err;
 }
 
-static void ahash_def_finup_done1(struct crypto_async_request *req, int err)
+static void ahash_def_finup_done1(struct crypto_async_request *areq, 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;
+	struct ahash_request *req = areq->data;
+	struct crypto_async_request *data;
 
-	err = ahash_def_finup_finish1(areq, err);
+	err = ahash_def_finup_finish1(req, err);
+
+	ahash_restore_req(req);
 
-	complete(data, err);
+	data = req->base.data;
+	req->base.complete(data, 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));
 }
-- 
1.8.5.2

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

[Tom Lendacky]

On Tuesday, January 14, 2014 06:33:47 PM 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, .priv, .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 | 65 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/crypto/ahash.c b/crypto/ahash.c
> index a92dc38..5ca8ede 100644
> --- a/crypto/ahash.c
> +++ b/crypto/ahash.c
> @@ -29,6 +29,7 @@
>  struct ahash_request_priv {
>  	crypto_completion_t complete;
>  	void *data;
> +	void *priv;
>  	u8 *result;
>  	void *ubuf[] CRYPTO_MINALIGN_ATTR;
>  };
> @@ -200,23 +201,38 @@ static void ahash_op_unaligned_finish(struct ahash_request *req, int err)
>  	if (!err)
>  		memcpy(priv->result, req->result,
>  		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
> -
> -	kzfree(priv);

You can't move/remove this kzfree since a synchronous operation will not take
the ahash_op_unaligned_done path.  A synchronous operation will never return
-EINPROGRESS and the effect will be to never free the priv structure.

>  }
>  
> -static void ahash_op_unaligned_done(struct crypto_async_request *req, int err)
> +static void ahash_op_unaligned_done(struct crypto_async_request *areq, 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;
> +	struct ahash_request *req = areq->data;
> +	struct ahash_request_priv *priv = req->priv;
> +	struct crypto_async_request *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(req, err);

Given the above comment on the kzfree, you'll need to save all the priv
values as was done previously.

Thanks,
Tom

>  
> -	ahash_op_unaligned_finish(areq, err);
> +	/* Restore the original crypto request. */
> +	req->result = priv->result;
> +	req->base.complete = priv->complete;
> +	req->base.data = priv->data;
> +	req->priv = priv->priv;
>  
> -	areq->base.complete = complete;
> -	areq->base.data = data;
> +	/* Free the req->priv.priv from the ADJUSTED request. */
> +	kzfree(priv);
>  
> -	complete(&areq->base, err);
> +	/* Complete the ORIGINAL request. */
> +	data = req->base.data;
> +	req->base.complete(data, err);
>  }
>  
>  static int ahash_op_unaligned(struct ahash_request *req,
> @@ -234,9 +250,36 @@ 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     = ORIGINAL(priv)
> +	 *   }
> +	 */
> +
>  	priv->result = req->result;
>  	priv->complete = req->base.complete;
>  	priv->data = req->base.data;
> +	priv->priv = req->priv;
>  
>  	req->result = PTR_ALIGN((u8 *)priv->ubuf, alignmask + 1);
>  	req->base.complete = ahash_op_unaligned_done;
> -- 
> 1.8.5.2
> 
> 

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

[Herbert Xu]

On Tue, Jan 14, 2014 at 01:35:50PM -0600, Tom Lendacky wrote:
> 
> On Tuesday, January 14, 2014 06:33:47 PM 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, .priv, .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 | 65 ++++++++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 54 insertions(+), 11 deletions(-)
> > 
> > diff --git a/crypto/ahash.c b/crypto/ahash.c
> > index a92dc38..5ca8ede 100644
> > --- a/crypto/ahash.c
> > +++ b/crypto/ahash.c
> > @@ -29,6 +29,7 @@
> >  struct ahash_request_priv {
> >  	crypto_completion_t complete;
> >  	void *data;
> > +	void *priv;
> >  	u8 *result;
> >  	void *ubuf[] CRYPTO_MINALIGN_ATTR;
> >  };
> > @@ -200,23 +201,38 @@ static void ahash_op_unaligned_finish(struct ahash_request *req, int err)
> >  	if (!err)
> >  		memcpy(priv->result, req->result,
> >  		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
> > -
> > -	kzfree(priv);
> 
> You can't move/remove this kzfree since a synchronous operation will not take
> the ahash_op_unaligned_done path.  A synchronous operation will never return
> -EINPROGRESS and the effect will be to never free the priv structure.

Marek, did you have a chance to address this?

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

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

[Marek Vasut]

On Sunday, February 09, 2014 at 02:18:56 AM, Herbert Xu wrote:
> On Tue, Jan 14, 2014 at 01:35:50PM -0600, Tom Lendacky wrote:
> > On Tuesday, January 14, 2014 06:33:47 PM 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, .priv, .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 | 65
> > >  ++++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file
> > >  changed, 54 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/crypto/ahash.c b/crypto/ahash.c
> > > index a92dc38..5ca8ede 100644
> > > --- a/crypto/ahash.c
> > > +++ b/crypto/ahash.c
> > > @@ -29,6 +29,7 @@
> > > 
> > >  struct ahash_request_priv {
> > >  
> > >  	crypto_completion_t complete;
> > >  	void *data;
> > > 
> > > +	void *priv;
> > > 
> > >  	u8 *result;
> > >  	void *ubuf[] CRYPTO_MINALIGN_ATTR;
> > >  
> > >  };
> > > 
> > > @@ -200,23 +201,38 @@ static void ahash_op_unaligned_finish(struct
> > > ahash_request *req, int err)
> > > 
> > >  	if (!err)
> > >  	
> > >  		memcpy(priv->result, req->result,
> > >  		
> > >  		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
> > > 
> > > -
> > > -	kzfree(priv);
> > 
> > You can't move/remove this kzfree since a synchronous operation will not
> > take the ahash_op_unaligned_done path.  A synchronous operation will
> > never return -EINPROGRESS and the effect will be to never free the priv
> > structure.
> 
> Marek, did you have a chance to address this?

Hi,

it's in the pipeline, I'm just overloaded. We missed a MW for this, right ? :-(

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

[Marek Vasut]

On Sunday, February 09, 2014 at 02:18:56 AM, Herbert Xu wrote:
> On Tue, Jan 14, 2014 at 01:35:50PM -0600, Tom Lendacky wrote:
> > On Tuesday, January 14, 2014 06:33:47 PM 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, .priv, .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 | 65
> > >  ++++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file
> > >  changed, 54 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/crypto/ahash.c b/crypto/ahash.c
> > > index a92dc38..5ca8ede 100644
> > > --- a/crypto/ahash.c
> > > +++ b/crypto/ahash.c
> > > @@ -29,6 +29,7 @@
> > > 
> > >  struct ahash_request_priv {
> > >  
> > >  	crypto_completion_t complete;
> > >  	void *data;
> > > 
> > > +	void *priv;
> > > 
> > >  	u8 *result;
> > >  	void *ubuf[] CRYPTO_MINALIGN_ATTR;
> > >  
> > >  };
> > > 
> > > @@ -200,23 +201,38 @@ static void ahash_op_unaligned_finish(struct
> > > ahash_request *req, int err)
> > > 
> > >  	if (!err)
> > >  	
> > >  		memcpy(priv->result, req->result,
> > >  		
> > >  		       crypto_ahash_digestsize(crypto_ahash_reqtfm(req)));
> > > 
> > > -
> > > -	kzfree(priv);
> > 
> > You can't move/remove this kzfree since a synchronous operation will not
> > take the ahash_op_unaligned_done path.  A synchronous operation will
> > never return -EINPROGRESS and the effect will be to never free the priv
> > structure.
> 
> Marek, did you have a chance to address this?

V2 is on it's way. I know there's really no apology for this horrible delay :-(

[PATCH 1/3 V2] 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, .priv, .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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 49 insertions(+), 9 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.

diff --git a/crypto/ahash.c b/crypto/ahash.c
index a92dc38..affebb5 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -29,6 +29,7 @@
 struct ahash_request_priv {
 	crypto_completion_t complete;
 	void *data;
+	void *priv;
 	u8 *result;
 	void *ubuf[] CRYPTO_MINALIGN_ATTR;
 };
@@ -201,22 +202,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 = priv->priv;
+
+	/* Free the req->priv.priv from the ADJUSTED request. */
 	kzfree(priv);
 }
 
-static void ahash_op_unaligned_done(struct crypto_async_request *req, int err)
+static void ahash_op_unaligned_done(struct crypto_async_request *areq, 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;
+	struct ahash_request *req = areq->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 req->result into req->priv.result */
+	ahash_op_unaligned_finish(req, err);
 
-	complete(&areq->base, err);
+	/* Complete the ORIGINAL request. */
+	req->base.complete(&req->base, err);
 }
 
 static int ahash_op_unaligned(struct ahash_request *req,
@@ -234,9 +247,36 @@ 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     = ORIGINAL(priv)
+	 *   }
+	 */
+
 	priv->result = req->result;
 	priv->complete = req->base.complete;
 	priv->data = req->base.data;
+	priv->priv = req->priv;
 
 	req->result = PTR_ALIGN((u8 *)priv->ubuf, alignmask + 1);
 	req->base.complete = ahash_op_unaligned_done;
-- 
1.8.5.2

[PATCH 2/3 V2] 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.

diff --git a/crypto/ahash.c b/crypto/ahash.c
index affebb5..4faa50e 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -191,55 +191,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 = priv->priv;
-
-	/* Free the req->priv.priv from the ADJUSTED request. */
-	kzfree(priv);
-}
-
-static void ahash_op_unaligned_done(struct crypto_async_request *areq, int err)
-{
-	struct ahash_request *req = areq->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(req, err);
-
-	/* Complete the ORIGINAL request. */
-	req->base.complete(&req->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) ?
@@ -279,10 +236,70 @@ static int ahash_op_unaligned(struct ahash_request *req,
 	priv->priv = req->priv;
 
 	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 = priv->priv;
+
+	/* 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 *areq, int err)
+{
+	struct ahash_request *req = areq->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(req, err);
+
+	/* Complete the ORIGINAL request. */
+	req->base.complete(&req->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 V2] 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.

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 | 52 +++++++++++++++++++---------------------------------
 1 file changed, 19 insertions(+), 33 deletions(-)

V2: Same as with 1/3 V2, move the restoration and freeing of the private
    data right past memcpy() of the buffer.

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 4faa50e..6f6d652 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -347,19 +347,18 @@ 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)
+static void ahash_def_finup_done2(struct crypto_async_request *areq, 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;
+	struct ahash_request *req = areq->data;
+	struct crypto_async_request *data;
 
-	ahash_def_finup_finish2(areq, err);
+	ahash_def_finup_finish2(req, err);
 
-	complete(data, err);
+	data = req->base.data;
+	req->base.complete(data, err);
 }
 
 static int ahash_def_finup_finish1(struct ahash_request *req, int err)
@@ -376,41 +375,28 @@ out:
 	return err;
 }
 
-static void ahash_def_finup_done1(struct crypto_async_request *req, int err)
+static void ahash_def_finup_done1(struct crypto_async_request *areq, 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;
+	struct ahash_request *req = areq->data;
+	struct crypto_async_request *data;
 
-	err = ahash_def_finup_finish1(areq, err);
+	err = ahash_def_finup_finish1(req, err);
 
-	complete(data, err);
+	data = req->base.data;
+	req->base.complete(data, 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 V2] crypto: Fix the pointer voodoo in unaligned ahash

[Herbert Xu]

On Mon, Mar 03, 2014 at 01:21:46AM +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, .priv, .base.data
> and .base.complete .

There is no point in saving priv because it is only meant to be
used by the crypto API.

Otherwise the patch looks OK to me.

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

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

[Herbert Xu]

On Mon, Mar 03, 2014 at 01:21:48AM +0100, Marek Vasut wrote:
>
> -static void ahash_def_finup_done2(struct crypto_async_request *req, int err)
> +static void ahash_def_finup_done2(struct crypto_async_request *areq, int err)

Please keep the existing name to be consistent with the rest
of the crypto API code.  You should also use the same name in
your second patch for consistency.

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

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

[Marek Vasut]

On Wednesday, March 12, 2014 at 01:08:14 PM, Herbert Xu wrote:
> On Mon, Mar 03, 2014 at 01:21:46AM +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,
> > .priv, .base.data and .base.complete .
> 
> There is no point in saving priv because it is only meant to be
> used by the crypto API.

OK, understood. But shall we not preserve the request intact in case a crypto-
api function called crypto_ahash_final() with request which has .priv already 
set? Then we would have really funny corruption of the request going on and I'm 
not sure that'd be nice.

> Otherwise the patch looks OK to me.

Thanks!

Best regards,
Marek Vasut

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

[Marek Vasut]

On Wednesday, March 12, 2014 at 01:11:01 PM, Herbert Xu wrote:
> On Mon, Mar 03, 2014 at 01:21:48AM +0100, Marek Vasut wrote:
> > -static void ahash_def_finup_done2(struct crypto_async_request *req, int
> > err) +static void ahash_def_finup_done2(struct crypto_async_request
> > *areq, int err)
> 
> Please keep the existing name to be consistent with the rest
> of the crypto API code.  You should also use the same name in
> your second patch for consistency.

Done.

Moreover, the original code this patch changes still has a bug I think. The 
invocation of areq->base.complete() is wrong in the original code here. I will 
post V3 of the patches where I explain the problem in detail.

Best regards,
Marek Vasut

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

[Herbert Xu]

On Thu, Mar 13, 2014 at 02:20:29AM +0100, Marek Vasut wrote:
>
> OK, understood. But shall we not preserve the request intact in case a crypto-
> api function called crypto_ahash_final() with request which has .priv already 
> set? Then we would have really funny corruption of the request going on and I'm 
> not sure that'd be nice.

The priv field is only ever used by ahash.c so how can this happen?
The crypto API refers to code in the API itself, excluding drivers
and users.

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

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

[Marek Vasut]

On Thursday, March 13, 2014 at 02:56:25 AM, Herbert Xu wrote:
> On Thu, Mar 13, 2014 at 02:20:29AM +0100, Marek Vasut wrote:
> > OK, understood. But shall we not preserve the request intact in case a
> > crypto- api function called crypto_ahash_final() with request which has
> > .priv already set? Then we would have really funny corruption of the
> > request going on and I'm not sure that'd be nice.
> 
> The priv field is only ever used by ahash.c so how can this happen?
> The crypto API refers to code in the API itself, excluding drivers
> and users.

OK, I agree with you that people plumbing in the API itself will know what 
they're doing.

btw. can you please check the V3 of 3/3 for the fixup of the base.completion() 
call ? I will then do tests and roll V4 of the series.

Best regards,
Marek Vasut

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

[Herbert Xu]

On Thu, Mar 13, 2014 at 05:01:00AM +0100, Marek Vasut wrote:
> On Thursday, March 13, 2014 at 02:56:25 AM, Herbert Xu wrote:
> > On Thu, Mar 13, 2014 at 02:20:29AM +0100, Marek Vasut wrote:
> > > OK, understood. But shall we not preserve the request intact in case a
> > > crypto- api function called crypto_ahash_final() with request which has
> > > .priv already set? Then we would have really funny corruption of the
> > > request going on and I'm not sure that'd be nice.
> > 
> > The priv field is only ever used by ahash.c so how can this happen?
> > The crypto API refers to code in the API itself, excluding drivers
> > and users.
> 
> OK, I agree with you that people plumbing in the API itself will know what 
> they're doing.
> 
> btw. can you please check the V3 of 3/3 for the fixup of the base.completion() 
> call ? I will then do tests and roll V4 of the series.

It looks good to me.

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