| Message ID | 1393806108-6374-1-git-send-email-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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
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,
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
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,
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;
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.