Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1617824 > unrolled thread

Re: XTS Crypto Not Found In /proc/crypto Even After Compiled for 4.10.1.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: XTS Crypto Not Found In /proc/crypto Even After Compiled for  4.10.1. Herbert Xu <herbert@gondor.apana.org.au> - 2017-04-06 12:00 +0200
    Re: XTS Crypto Not Found In /proc/crypto Even After Compiled for  4.10.1. Herbert Xu <herbert@gondor.apana.org.au> - 2017-04-08 04:10 +0200
      Re: XTS Crypto Not Found In /proc/crypto Even After Compiled for  4.10.1. Krzysztof Kozlowski <krzk@kernel.org> - 2017-04-08 14:30 +0200

#1617824 — Re: XTS Crypto Not Found In /proc/crypto Even After Compiled for 4.10.1.

On Mon, Mar 13, 2017 at 07:06:01PM +0200, Krzysztof Kozlowski wrote:
>
> I bisected this to commit f1c131b45410 ("crypto: xts - Convert to
> skcipher"). The s5p-sss driver stays the same... but the xts changes and
> as a result we have a NULL pointer dereference (actually of value
> 00000004):
> [   18.930195] Unable to handle kernel NULL pointer dereference at virtual address 00000004
> ...
> [   18.972325] [<c0313c98>] (post_crypt) from [<c031408c>] (decrypt_done+0x4c/0x54)
> [   18.972343] [<c031408c>] (decrypt_done) from [<c056309c>] (s5p_aes_interrupt+0x1bc/0x208)
> [   18.972360] [<c056309c>] (s5p_aes_interrupt) from [<c0164930>] (irq_thread_fn+0x1c/0x54)
> 
> Any hints?

I haven't found any smoking guns, but the locking between the
tasklet and the IRQ routine looks suspect.  First of all the
tasklet is modifying the dev structure without holding any locks.

More importantly, the IRQ routine does not seem to be robust in
the face of spurious interrupts.  Should a spurious interrupt
arrive, it is entirely possible for the tasklet's modifying of
dev->req to race with the IRQ routine which reads dev->req.

However, this does depend on there being a spurious interrupt so
I don't know how likely it is.

Anyway, if we can't get to the bottom of this, we should disable
the broken functionality.

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

[toc] | [next] | [standalone]

#1619212

On Thu, Apr 06, 2017 at 05:54:14PM +0800, Herbert Xu wrote:
> On Mon, Mar 13, 2017 at 07:06:01PM +0200, Krzysztof Kozlowski wrote:
> >
> > I bisected this to commit f1c131b45410 ("crypto: xts - Convert to
> > skcipher"). The s5p-sss driver stays the same... but the xts changes and
> > as a result we have a NULL pointer dereference (actually of value
> > 00000004):
> > [   18.930195] Unable to handle kernel NULL pointer dereference at virtual address 00000004
> > ...
> > [   18.972325] [<c0313c98>] (post_crypt) from [<c031408c>] (decrypt_done+0x4c/0x54)
> > [   18.972343] [<c031408c>] (decrypt_done) from [<c056309c>] (s5p_aes_interrupt+0x1bc/0x208)
> > [   18.972360] [<c056309c>] (s5p_aes_interrupt) from [<c0164930>] (irq_thread_fn+0x1c/0x54)
> > 
> > Any hints?
> 
> I haven't found any smoking guns, but the locking between the
> tasklet and the IRQ routine looks suspect.  First of all the
> tasklet is modifying the dev structure without holding any locks.

I think I see the problem.  Could you please try this patch and
let me know if it fixes the crash?

---8<---
Subject: crypto: xts - Fix use-after-free on EINPROGRESS

When we get an EINPROGRESS completion in xts, we will end up marking
the request as done and freeing it.  This then blows up when the
request is really completed as we've already freed the memory.

Fixes: f1c131b45410 ("crypto: xts - Convert to skcipher")
Cc: <stable@vger.kernel.org>
Reported-by: Nathan Royce <nroycea+kernel@gmail.com>
Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/xts.c b/crypto/xts.c
index e197e64..d86c11a 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -286,6 +286,13 @@ static void encrypt_done(struct crypto_async_request *areq, int err)
 	struct rctx *rctx;
 
 	rctx = skcipher_request_ctx(req);
+
+	if (err == -EINPROGRESS) {
+		if (rctx->left != req->cryptlen)
+			return;
+		goto out;
+	}
+
 	subreq = &rctx->subreq;
 	subreq->base.flags &= CRYPTO_TFM_REQ_MAY_BACKLOG;
 
@@ -293,6 +300,7 @@ static void encrypt_done(struct crypto_async_request *areq, int err)
 	if (rctx->left)
 		return;
 
+out:
 	skcipher_request_complete(req, err);
 }
 
@@ -330,6 +338,13 @@ static void decrypt_done(struct crypto_async_request *areq, int err)
 	struct rctx *rctx;
 
 	rctx = skcipher_request_ctx(req);
+
+	if (err == -EINPROGRESS) {
+		if (rctx->left != req->cryptlen)
+			return;
+		goto out;
+	}
+
 	subreq = &rctx->subreq;
 	subreq->base.flags &= CRYPTO_TFM_REQ_MAY_BACKLOG;
 
@@ -337,6 +352,7 @@ static void decrypt_done(struct crypto_async_request *areq, int err)
 	if (rctx->left)
 		return;
 
+out:
 	skcipher_request_complete(req, err);
 }
 
-- 
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

[toc] | [prev] | [next] | [standalone]

#1619274

On Sat, Apr 08, 2017 at 10:02:46AM +0800, Herbert Xu wrote:
> On Thu, Apr 06, 2017 at 05:54:14PM +0800, Herbert Xu wrote:
> > On Mon, Mar 13, 2017 at 07:06:01PM +0200, Krzysztof Kozlowski wrote:
> > >
> > > I bisected this to commit f1c131b45410 ("crypto: xts - Convert to
> > > skcipher"). The s5p-sss driver stays the same... but the xts changes and
> > > as a result we have a NULL pointer dereference (actually of value
> > > 00000004):
> > > [   18.930195] Unable to handle kernel NULL pointer dereference at virtual address 00000004
> > > ...
> > > [   18.972325] [<c0313c98>] (post_crypt) from [<c031408c>] (decrypt_done+0x4c/0x54)
> > > [   18.972343] [<c031408c>] (decrypt_done) from [<c056309c>] (s5p_aes_interrupt+0x1bc/0x208)
> > > [   18.972360] [<c056309c>] (s5p_aes_interrupt) from [<c0164930>] (irq_thread_fn+0x1c/0x54)
> > > 
> > > Any hints?
> > 
> > I haven't found any smoking guns, but the locking between the
> > tasklet and the IRQ routine looks suspect.  First of all the
> > tasklet is modifying the dev structure without holding any locks.
> 
> I think I see the problem.  Could you please try this patch and
> let me know if it fixes the crash?

Yes, fixed! Thanks.
Tested on Odroid XU3 with following script:
https://github.com/krzk/tools/blob/master/tests/s5p-sss-cryptsetup.sh

Tested-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web