2017-12-19 15:42:39

by Jan Engelhardt

[permalink] [raw]
Subject: Re: [PATCH] crypto: n2 - cure use after free


On Tuesday 2017-12-19 16:31, David Miller wrote:
>
>Instead, what fails is the algorithm registry which you should look
>more deeply into the cause of.

You are right. The registration failure is because the crypto layer
expects halg->statesize to be non-zero, and drivers/crypto/n2_core.c
does not set it, causing breakage since possibly
v4.2-rc1-182-g8996eafdcbad (commit by rmk):

(1471) halg = &ahash->halg;
halg->digestsize = tmpl->digest_size;

Nevertheless, I think that the error pathing in n2_core.c should be made
robust as well. Should I resubmit with a new commit message?


2017-12-19 16:38:20

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] crypto: n2 - cure use after free

From: Jan Engelhardt <[email protected]>
Date: Tue, 19 Dec 2017 16:42:39 +0100 (CET)

> Nevertheless, I think that the error pathing in n2_core.c should be made
> robust as well.

I completely agree.

> Should I resubmit with a new commit message?

Yes.

2017-12-19 18:09:07

by Jan Engelhardt

[permalink] [raw]
Subject: [PATCH] crypto: n2 - cure use after free

queue_cache_init is first called for the Control Word Queue
(n2_crypto_probe). At that time, queue_cache[0] is NULL and a new
kmem_cache will be allocated. If the subsequent n2_register_algs call
fails, the kmem_cache will be released in queue_cache_destroy, but
queue_cache_init[0] is not set back to NULL.

So when the Module Arithmetic Unit gets probed next (n2_mau_probe),
queue_cache_init will not allocate a kmem_cache again, but leave it
as its bogus value, causing a BUG() to trigger when queue_cache[0] is
eventually passed to kmem_cache_zalloc:

n2_crypto: Found N2CP at /virtual-devices@100/n2cp@7
n2_crypto: Registered NCS HVAPI version 2.0
called queue_cache_init
n2_crypto: md5 alg registration failed
n2cp f028687c: /virtual-devices@100/n2cp@7: Unable to register algorithms.
called queue_cache_destroy
n2cp: probe of f028687c failed with error -22
n2_crypto: Found NCP at /virtual-devices@100/ncp@6
n2_crypto: Registered NCS HVAPI version 2.0
called queue_cache_init
kernel BUG at mm/slab.c:2993!
Call Trace:
[0000000000604488] kmem_cache_alloc+0x1a8/0x1e0
(inlined) kmem_cache_zalloc
(inlined) new_queue
(inlined) spu_queue_setup
(inlined) handle_exec_unit
[0000000010c61eb4] spu_mdesc_scan+0x1f4/0x460 [n2_crypto]
[0000000010c62b80] n2_mau_probe+0x100/0x220 [n2_crypto]
[000000000084b174] platform_drv_probe+0x34/0xc0

Signed-off-by: Jan Engelhardt <[email protected]>
---
drivers/crypto/n2_core.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/crypto/n2_core.c b/drivers/crypto/n2_core.c
index 48de52cf2ecc..662e709812cc 100644
--- a/drivers/crypto/n2_core.c
+++ b/drivers/crypto/n2_core.c
@@ -1625,6 +1625,7 @@ static int queue_cache_init(void)
CWQ_ENTRY_SIZE, 0, NULL);
if (!queue_cache[HV_NCS_QTYPE_CWQ - 1]) {
kmem_cache_destroy(queue_cache[HV_NCS_QTYPE_MAU - 1]);
+ queue_cache[HV_NCS_QTYPE_MAU - 1] = NULL;
return -ENOMEM;
}
return 0;
@@ -1634,6 +1635,8 @@ static void queue_cache_destroy(void)
{
kmem_cache_destroy(queue_cache[HV_NCS_QTYPE_MAU - 1]);
kmem_cache_destroy(queue_cache[HV_NCS_QTYPE_CWQ - 1]);
+ queue_cache[HV_NCS_QTYPE_MAU - 1] = NULL;
+ queue_cache[HV_NCS_QTYPE_CWQ - 1] = NULL;
}

static long spu_queue_register_workfn(void *arg)
--
2.12.3


2017-12-19 18:39:13

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] crypto: n2 - cure use after free

From: Jan Engelhardt <[email protected]>
Date: Tue, 19 Dec 2017 19:09:07 +0100

> queue_cache_init is first called for the Control Word Queue
> (n2_crypto_probe). At that time, queue_cache[0] is NULL and a new
> kmem_cache will be allocated. If the subsequent n2_register_algs call
> fails, the kmem_cache will be released in queue_cache_destroy, but
> queue_cache_init[0] is not set back to NULL.
>
> So when the Module Arithmetic Unit gets probed next (n2_mau_probe),
> queue_cache_init will not allocate a kmem_cache again, but leave it
> as its bogus value, causing a BUG() to trigger when queue_cache[0] is
> eventually passed to kmem_cache_zalloc:
...
> Signed-off-by: Jan Engelhardt <[email protected]>

Acked-by: David S. Miller <[email protected]>

2017-12-22 08:36:16

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH] crypto: n2 - cure use after free

Jan Engelhardt <[email protected]> wrote:
> queue_cache_init is first called for the Control Word Queue
> (n2_crypto_probe). At that time, queue_cache[0] is NULL and a new
> kmem_cache will be allocated. If the subsequent n2_register_algs call
> fails, the kmem_cache will be released in queue_cache_destroy, but
> queue_cache_init[0] is not set back to NULL.
>
> So when the Module Arithmetic Unit gets probed next (n2_mau_probe),
> queue_cache_init will not allocate a kmem_cache again, but leave it
> as its bogus value, causing a BUG() to trigger when queue_cache[0] is
> eventually passed to kmem_cache_zalloc:
>
> n2_crypto: Found N2CP at /virtual-devices@100/n2cp@7
> n2_crypto: Registered NCS HVAPI version 2.0
> called queue_cache_init
> n2_crypto: md5 alg registration failed
> n2cp f028687c: /virtual-devices@100/n2cp@7: Unable to register algorithms.
> called queue_cache_destroy
> n2cp: probe of f028687c failed with error -22
> n2_crypto: Found NCP at /virtual-devices@100/ncp@6
> n2_crypto: Registered NCS HVAPI version 2.0
> called queue_cache_init
> kernel BUG at mm/slab.c:2993!
> Call Trace:
> [0000000000604488] kmem_cache_alloc+0x1a8/0x1e0
> (inlined) kmem_cache_zalloc
> (inlined) new_queue
> (inlined) spu_queue_setup
> (inlined) handle_exec_unit
> [0000000010c61eb4] spu_mdesc_scan+0x1f4/0x460 [n2_crypto]
> [0000000010c62b80] n2_mau_probe+0x100/0x220 [n2_crypto]
> [000000000084b174] platform_drv_probe+0x34/0xc0
>
> Signed-off-by: Jan Engelhardt <[email protected]>

Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt