memory leak in crypto_create_tfm

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: 19409891 Merge tag 'pnp-5.8-rc1' of git://git.kernel.org/p..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13165aa6100000
kernel config: https://syzkaller.appspot.com/x/.config?x=6d41e63a2c7e0715
dashboard link: https://syzkaller.appspot.com/bug?extid=2e635807decef724a1fa
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17f00ef2100000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=170f2ef2100000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+2e6358...@syzkaller.appspotmail.com

executing program
executing program
BUG: memory leak
unreferenced object 0xffff8881175bc480 (size 64):
comm "syz-executor064", pid 6388, jiffies 4294941622 (age 13.280s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
e0 7e 56 84 ff ff ff ff 00 00 00 00 00 00 00 00 .~V.............
backtrace:
[<0000000029c7602f>] kmalloc include/linux/slab.h:560 [inline]
[<0000000029c7602f>] kzalloc include/linux/slab.h:669 [inline]
[<0000000029c7602f>] crypto_create_tfm+0x31/0x100 crypto/api.c:448
[<00000000bec8cbdb>] crypto_alloc_tfm+0x79/0x1a0 crypto/api.c:527
[<000000002f9791ba>] drbg_prepare_hrng crypto/drbg.c:1509 [inline]
[<000000002f9791ba>] drbg_instantiate crypto/drbg.c:1587 [inline]
[<000000002f9791ba>] drbg_kcapi_seed+0x432/0x6a9 crypto/drbg.c:1980
[<0000000041302bb8>] crypto_rng_reset+0x35/0x1a0 crypto/rng.c:53
[<000000004758c3c4>] alg_setkey crypto/af_alg.c:222 [inline]
[<000000004758c3c4>] alg_setsockopt+0x149/0x190 crypto/af_alg.c:255
[<000000008bc4b5cb>] __sys_setsockopt+0x112/0x230 net/socket.c:2132
[<00000000cfbf30da>] __do_sys_setsockopt net/socket.c:2148 [inline]
[<00000000cfbf30da>] __se_sys_setsockopt net/socket.c:2145 [inline]
[<00000000cfbf30da>] __x64_sys_setsockopt+0x22/0x30 net/socket.c:2145
[<00000000fc9c2183>] do_syscall_64+0x6e/0x220 arch/x86/entry/common.c:295
[<0000000040e080a1>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

BUG: memory leak
unreferenced object 0xffff8881175bc040 (size 64):
comm "syz-executor064", pid 6389, jiffies 4294942172 (age 7.780s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
e0 7e 56 84 ff ff ff ff 00 00 00 00 00 00 00 00 .~V.............
backtrace:
[<0000000029c7602f>] kmalloc include/linux/slab.h:560 [inline]
[<0000000029c7602f>] kzalloc include/linux/slab.h:669 [inline]
[<0000000029c7602f>] crypto_create_tfm+0x31/0x100 crypto/api.c:448
[<00000000bec8cbdb>] crypto_alloc_tfm+0x79/0x1a0 crypto/api.c:527
[<000000002f9791ba>] drbg_prepare_hrng crypto/drbg.c:1509 [inline]
[<000000002f9791ba>] drbg_instantiate crypto/drbg.c:1587 [inline]
[<000000002f9791ba>] drbg_kcapi_seed+0x432/0x6a9 crypto/drbg.c:1980
[<0000000041302bb8>] crypto_rng_reset+0x35/0x1a0 crypto/rng.c:53
[<000000004758c3c4>] alg_setkey crypto/af_alg.c:222 [inline]
[<000000004758c3c4>] alg_setsockopt+0x149/0x190 crypto/af_alg.c:255
[<000000008bc4b5cb>] __sys_setsockopt+0x112/0x230 net/socket.c:2132
[<00000000cfbf30da>] __do_sys_setsockopt net/socket.c:2148 [inline]
[<00000000cfbf30da>] __se_sys_setsockopt net/socket.c:2145 [inline]
[<00000000cfbf30da>] __x64_sys_setsockopt+0x22/0x30 net/socket.c:2145
[<00000000fc9c2183>] do_syscall_64+0x6e/0x220 arch/x86/entry/common.c:295
[<0000000040e080a1>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

BUG: memory leak
unreferenced object 0xffff88811b3ca080 (size 96):
comm "syz-executor064", pid 6389, jiffies 4294942172 (age 7.780s)
hex dump (first 32 bytes):
89 c7 08 cb 8a 12 10 6e 00 00 00 00 00 00 00 00 .......n........
71 51 5a c2 1b 00 00 00 35 7d 00 00 00 00 00 00 qQZ.....5}......
backtrace:
[<000000008ec3eca0>] jent_entropy_collector_alloc+0x1b/0xf8 crypto/jitterentropy.c:662
[<0000000026ed401a>] jent_kcapi_init+0x17/0x40 crypto/jitterentropy-kcapi.c:119
[<00000000be7d6b06>] crypto_create_tfm+0x89/0x100 crypto/api.c:459
[<00000000bec8cbdb>] crypto_alloc_tfm+0x79/0x1a0 crypto/api.c:527
[<000000002f9791ba>] drbg_prepare_hrng crypto/drbg.c:1509 [inline]
[<000000002f9791ba>] drbg_instantiate crypto/drbg.c:1587 [inline]
[<000000002f9791ba>] drbg_kcapi_seed+0x432/0x6a9 crypto/drbg.c:1980
[<0000000041302bb8>] crypto_rng_reset+0x35/0x1a0 crypto/rng.c:53
[<000000004758c3c4>] alg_setkey crypto/af_alg.c:222 [inline]
[<000000004758c3c4>] alg_setsockopt+0x149/0x190 crypto/af_alg.c:255
[<000000008bc4b5cb>] __sys_setsockopt+0x112/0x230 net/socket.c:2132
[<00000000cfbf30da>] __do_sys_setsockopt net/socket.c:2148 [inline]
[<00000000cfbf30da>] __se_sys_setsockopt net/socket.c:2145 [inline]
[<00000000cfbf30da>] __x64_sys_setsockopt+0x22/0x30 net/socket.c:2145
[<00000000fc9c2183>] do_syscall_64+0x6e/0x220 arch/x86/entry/common.c:295
[<0000000040e080a1>] entry_SYSCALL_64_after_hwframe+0x44/0xa9



---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Eric Biggers]

Probably a bug in crypto/drbg.c. Stephan, can you take a look?

> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/0000000000002a280b05a725cd93%40google.com.

[Stephan Müller]

The Jitter RNG is unconditionally allocated as a seed source follwoing
the patch 97f2650e5040. Thus, the instance must always be deallocated.

Reported-by: syzbot+2e6358...@syzkaller.appspotmail.com
Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
Signed-off-by: Stephan Mueller <smue...@chronox.de>
---
crypto/drbg.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 37526eb8c5d5..33d28016da2d 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1631,6 +1631,9 @@ static int drbg_uninstantiate(struct drbg_state *drbg)
if (drbg->random_ready.func) {
del_random_ready_callback(&drbg->random_ready);
cancel_work_sync(&drbg->seed_work);
+ }
+
+ if (drbg->jent) {
crypto_free_rng(drbg->jent);
drbg->jent = NULL;
}
--
2.26.2

[Dan Carpenter]

free_everything functions never work. For example, "drbg->jent" can be
an error pointer at this point.

crypto/drbg.c
1577 if (!drbg->core) {
1578 drbg->core = &drbg_cores[coreref];
1579 drbg->pr = pr;
1580 drbg->seeded = false;
1581 drbg->reseed_threshold = drbg_max_requests(drbg);
1582
1583 ret = drbg_alloc_state(drbg);
1584 if (ret)
1585 goto unlock;
1586
1587 ret = drbg_prepare_hrng(drbg);
1588 if (ret)
1589 goto free_everything;
^^^^^^^^^^^^^^^^^^^^
If we hit two failures inside drbg_prepare_hrng() then "drbg->jent" can
be an error pointer.

1590
1591 if (IS_ERR(drbg->jent)) {
1592 ret = PTR_ERR(drbg->jent);
1593 drbg->jent = NULL;
1594 if (fips_enabled || ret != -ENOENT)
1595 goto free_everything;
1596 pr_info("DRBG: Continuing without Jitter RNG\n");
1597 }
1598
1599 reseed = false;
1600 }
1601
1602 ret = drbg_seed(drbg, pers, reseed);
1603
1604 if (ret && !reseed)
1605 goto free_everything;
1606
1607 mutex_unlock(&drbg->drbg_mutex);
1608 return ret;
1609
1610 unlock:
1611 mutex_unlock(&drbg->drbg_mutex);
1612 return ret;
1613
1614 free_everything:
1615 mutex_unlock(&drbg->drbg_mutex);
1616 drbg_uninstantiate(drbg);
^^^^
Leading to an Oops.

1617 return ret;
1618 }

regards,
dan carpenter

[Stephan Mueller]

Am Mittwoch, 3. Juni 2020, 13:09:19 CEST schrieb Dan Carpenter:

Hi Dan,

Thanks a lot for the hint - a version 2 should come shortly.

>
> 1617 return ret;
> 1618 }
>
> regards,
> dan carpenter

Ciao
Stephan

[Stephan Müller]

The Jitter RNG is unconditionally allocated as a seed source follwoing
the patch 97f2650e5040. Thus, the instance must always be deallocated.

Reported-by: syzbot+2e6358...@syzkaller.appspotmail.com
Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
Signed-off-by: Stephan Mueller <smue...@chronox.de>
---
crypto/drbg.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/crypto/drbg.c b/crypto/drbg.c

index 37526eb8c5d5..8a0f16950144 100644

--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1631,6 +1631,9 @@ static int drbg_uninstantiate(struct drbg_state *drbg)
if (drbg->random_ready.func) {
del_random_ready_callback(&drbg->random_ready);
cancel_work_sync(&drbg->seed_work);
+ }
+

+ if (!IS_ERR_OR_NULL(drbg->jent)) {

crypto_free_rng(drbg->jent);
drbg->jent = NULL;
}

--
2.26.2

[Eric Biggers]

It it okay that ->jent can be left as an ERR_PTR() value?

Perhaps it should always be set to NULL?

- Eric

[Stephan Mueller]

Am Freitag, 5. Juni 2020, 02:43:36 CEST schrieb Eric Biggers:

Hi Eric,

The error value is used in the drbg_instantiate function. There it is checked
whether -ENOENT (i.e. the cipher is not available) or any other error is
present. I am not sure we should move that check.

Thanks for the review.
>
> - Eric


Ciao
Stephan

[Eric Biggers]

drbg_seed() and drbg_async_seed() check for drbg->jent being NULL.

Will that now break due it drbg->jent possibly being an ERR_PTR()?

Hence why I'm asking whether drbg_uninstantiate() should set it to NULL.

- Eric

[Stephan Mueller]

The allocation happens in drbg_prepare_hrng that is only invoked by
drbg_instantiate.

drbg_instantiate checks for the ERR_PTR and sets it to NULL in case the error
is deemed ok.

Thus, any subsequent functions would see either a valid pointer or NULL. The
only exception is drbg_uninstantiate when invoked from the error case

ret = drbg_prepare_hrng(drbg);
if (ret)
goto free_everything;

Thus, I think that the two functions you mention will never see any values
other than NULL or a valid pointer.

Thanks


Ciao
Stephan

[Eric Biggers]

To be concrete, I'm suggesting:

if (!IS_ERR_OR_NULL(drbg->jent))
crypto_free_rng(drbg->jent);
drbg->jent = NULL;

This would be similar to how drbg_dealloc_state() sets lots of other fields of
the drbg_state to NULL.

It's your call though. I haven't properly read this code; the above is just
what makes sense to me at first glance...

- Eric

[Stephan Müller]

Am Freitag, 5. Juni 2020, 18:21:49 CEST schrieb Eric Biggers:

Hi Eric,

> To be concrete, I'm suggesting:
>
> if (!IS_ERR_OR_NULL(drbg->jent))
> crypto_free_rng(drbg->jent);
> drbg->jent = NULL;

I currently do not see that this could lead to an issue. But you are right, we
should use defensive programming everywhere.

I will send a v3 shortly.

Thanks.


Ciao
Stephan

[Stephan Müller]

The Jitter RNG is unconditionally allocated as a seed source follwoing
the patch 97f2650e5040. Thus, the instance must always be deallocated.

Reported-by: syzbot+2e6358...@syzkaller.appspotmail.com
Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
Signed-off-by: Stephan Mueller <smue...@chronox.de>
---

crypto/drbg.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 37526eb8c5d5..8d80d93cab97 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1631,10 +1631,12 @@ static int drbg_uninstantiate(struct drbg_state *drbg)

if (drbg->random_ready.func) {
del_random_ready_callback(&drbg->random_ready);
cancel_work_sync(&drbg->seed_work);

- crypto_free_rng(drbg->jent);
- drbg->jent = NULL;
}

+ if (!IS_ERR_OR_NULL(drbg->jent))
+ crypto_free_rng(drbg->jent);
+ drbg->jent = NULL;
+
if (drbg->d_ops)
drbg->d_ops->crypto_fini(drbg);
drbg_dealloc_state(drbg);
--
2.26.2

[Herbert Xu]

On Sun, Jun 07, 2020 at 03:20:26PM +0200, Stephan Müller wrote:
> The Jitter RNG is unconditionally allocated as a seed source follwoing
> the patch 97f2650e5040. Thus, the instance must always be deallocated.
>
> Reported-by: syzbot+2e6358...@syzkaller.appspotmail.com
> Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> Signed-off-by: Stephan Mueller <smue...@chronox.de>
> ---
> crypto/drbg.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)

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