* memory leak in crypto_create_tfm
@ 2020-06-03 3:41 syzbot
2020-06-03 3:55 ` Eric Biggers
2020-06-03 8:08 ` [PATCH] crypto: DRBG - always try to free Jitter RNG instance Stephan Müller
0 siblings, 2 replies; 14+ messages in thread
From: syzbot @ 2020-06-03 3:41 UTC (permalink / raw)
To: davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs
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+2e635807decef724a1fa@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 syzkaller@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: memory leak in crypto_create_tfm 2020-06-03 3:41 memory leak in crypto_create_tfm syzbot @ 2020-06-03 3:55 ` Eric Biggers 2020-06-03 8:08 ` [PATCH] crypto: DRBG - always try to free Jitter RNG instance Stephan Müller 1 sibling, 0 replies; 14+ messages in thread From: Eric Biggers @ 2020-06-03 3:55 UTC (permalink / raw) To: Stephan Müller Cc: davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot Probably a bug in crypto/drbg.c. Stephan, can you take a look? On Tue, Jun 02, 2020 at 08:41:21PM -0700, syzbot wrote: > 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+2e635807decef724a1fa@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 syzkaller@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 > > -- > 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-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/0000000000002a280b05a725cd93%40google.com. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] crypto: DRBG - always try to free Jitter RNG instance 2020-06-03 3:41 memory leak in crypto_create_tfm syzbot 2020-06-03 3:55 ` Eric Biggers @ 2020-06-03 8:08 ` Stephan Müller 2020-06-03 11:09 ` Dan Carpenter 1 sibling, 1 reply; 14+ messages in thread From: Stephan Müller @ 2020-06-03 8:08 UTC (permalink / raw) To: davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot The Jitter RNG is unconditionally allocated as a seed source follwoing the patch 97f2650e5040. Thus, the instance must always be deallocated. Reported-by: syzbot+2e635807decef724a1fa@syzkaller.appspotmail.com Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...") Signed-off-by: Stephan Mueller <smueller@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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] crypto: DRBG - always try to free Jitter RNG instance 2020-06-03 8:08 ` [PATCH] crypto: DRBG - always try to free Jitter RNG instance Stephan Müller @ 2020-06-03 11:09 ` Dan Carpenter 2020-06-03 11:40 ` Stephan Mueller 2020-06-04 6:41 ` [PATCH v2] " Stephan Müller 0 siblings, 2 replies; 14+ messages in thread From: Dan Carpenter @ 2020-06-03 11:09 UTC (permalink / raw) To: Stephan Müller Cc: davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot On Wed, Jun 03, 2020 at 10:08:56AM +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+2e635807decef724a1fa@syzkaller.appspotmail.com > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...") > Signed-off-by: Stephan Mueller <smueller@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; > } 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] crypto: DRBG - always try to free Jitter RNG instance 2020-06-03 11:09 ` Dan Carpenter @ 2020-06-03 11:40 ` Stephan Mueller 2020-06-04 6:41 ` [PATCH v2] " Stephan Müller 1 sibling, 0 replies; 14+ messages in thread From: Stephan Mueller @ 2020-06-03 11:40 UTC (permalink / raw) To: Dan Carpenter Cc: davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot Am Mittwoch, 3. Juni 2020, 13:09:19 CEST schrieb Dan Carpenter: Hi Dan, > On Wed, Jun 03, 2020 at 10:08:56AM +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+2e635807decef724a1fa@syzkaller.appspotmail.com > > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...") > > Signed-off-by: Stephan Mueller <smueller@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; > > > > } > > 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. Thanks a lot for the hint - a version 2 should come shortly. > > 1617 return ret; > 1618 } > > regards, > dan carpenter Ciao Stephan ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] crypto: DRBG - always try to free Jitter RNG instance 2020-06-03 11:09 ` Dan Carpenter 2020-06-03 11:40 ` Stephan Mueller @ 2020-06-04 6:41 ` Stephan Müller 2020-06-05 0:43 ` Eric Biggers 2020-06-07 13:20 ` [PATCH v3] " Stephan Müller 1 sibling, 2 replies; 14+ messages in thread From: Stephan Müller @ 2020-06-04 6:41 UTC (permalink / raw) To: Dan Carpenter Cc: davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot The Jitter RNG is unconditionally allocated as a seed source follwoing the patch 97f2650e5040. Thus, the instance must always be deallocated. Reported-by: syzbot+2e635807decef724a1fa@syzkaller.appspotmail.com Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...") Signed-off-by: Stephan Mueller <smueller@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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] crypto: DRBG - always try to free Jitter RNG instance 2020-06-04 6:41 ` [PATCH v2] " Stephan Müller @ 2020-06-05 0:43 ` Eric Biggers 2020-06-05 5:58 ` Stephan Mueller 2020-06-07 13:20 ` [PATCH v3] " Stephan Müller 1 sibling, 1 reply; 14+ messages in thread From: Eric Biggers @ 2020-06-05 0:43 UTC (permalink / raw) To: Stephan Müller Cc: Dan Carpenter, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot On Thu, Jun 04, 2020 at 08:41:00AM +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+2e635807decef724a1fa@syzkaller.appspotmail.com > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...") > Signed-off-by: Stephan Mueller <smueller@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; > } It it okay that ->jent can be left as an ERR_PTR() value? Perhaps it should always be set to NULL? - Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] crypto: DRBG - always try to free Jitter RNG instance 2020-06-05 0:43 ` Eric Biggers @ 2020-06-05 5:58 ` Stephan Mueller 2020-06-05 6:16 ` Eric Biggers 0 siblings, 1 reply; 14+ messages in thread From: Stephan Mueller @ 2020-06-05 5:58 UTC (permalink / raw) To: Eric Biggers Cc: Dan Carpenter, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot Am Freitag, 5. Juni 2020, 02:43:36 CEST schrieb Eric Biggers: Hi Eric, > On Thu, Jun 04, 2020 at 08:41:00AM +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+2e635807decef724a1fa@syzkaller.appspotmail.com > > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...") > > Signed-off-by: Stephan Mueller <smueller@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; > > > > } > > It it okay that ->jent can be left as an ERR_PTR() value? > > Perhaps it should always be set to NULL? 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] crypto: DRBG - always try to free Jitter RNG instance 2020-06-05 5:58 ` Stephan Mueller @ 2020-06-05 6:16 ` Eric Biggers 2020-06-05 6:52 ` Stephan Mueller 0 siblings, 1 reply; 14+ messages in thread From: Eric Biggers @ 2020-06-05 6:16 UTC (permalink / raw) To: Stephan Mueller Cc: Dan Carpenter, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot On Fri, Jun 05, 2020 at 07:58:15AM +0200, Stephan Mueller wrote: > Am Freitag, 5. Juni 2020, 02:43:36 CEST schrieb Eric Biggers: > > Hi Eric, > > > On Thu, Jun 04, 2020 at 08:41:00AM +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+2e635807decef724a1fa@syzkaller.appspotmail.com > > > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...") > > > Signed-off-by: Stephan Mueller <smueller@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; > > > > > > } > > > > It it okay that ->jent can be left as an ERR_PTR() value? > > > > Perhaps it should always be set to NULL? > > 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. > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] crypto: DRBG - always try to free Jitter RNG instance 2020-06-05 6:16 ` Eric Biggers @ 2020-06-05 6:52 ` Stephan Mueller 2020-06-05 16:21 ` Eric Biggers 0 siblings, 1 reply; 14+ messages in thread From: Stephan Mueller @ 2020-06-05 6:52 UTC (permalink / raw) To: Eric Biggers Cc: Dan Carpenter, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot Am Freitag, 5. Juni 2020, 08:16:46 CEST schrieb Eric Biggers: Hi Eric, > On Fri, Jun 05, 2020 at 07:58:15AM +0200, Stephan Mueller wrote: > > Am Freitag, 5. Juni 2020, 02:43:36 CEST schrieb Eric Biggers: > > > > Hi Eric, > > > > > On Thu, Jun 04, 2020 at 08:41:00AM +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+2e635807decef724a1fa@syzkaller.appspotmail.com > > > > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B > > > > ...") > > > > Signed-off-by: Stephan Mueller <smueller@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; > > > > > > > > } > > > > > > It it okay that ->jent can be left as an ERR_PTR() value? > > > > > > Perhaps it should always be set to NULL? > > > > 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. > > 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. 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] crypto: DRBG - always try to free Jitter RNG instance 2020-06-05 6:52 ` Stephan Mueller @ 2020-06-05 16:21 ` Eric Biggers 2020-06-07 13:07 ` Stephan Müller 0 siblings, 1 reply; 14+ messages in thread From: Eric Biggers @ 2020-06-05 16:21 UTC (permalink / raw) To: Stephan Mueller Cc: Dan Carpenter, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot On Fri, Jun 05, 2020 at 08:52:57AM +0200, Stephan Mueller wrote: > Am Freitag, 5. Juni 2020, 08:16:46 CEST schrieb Eric Biggers: > > Hi Eric, > > > On Fri, Jun 05, 2020 at 07:58:15AM +0200, Stephan Mueller wrote: > > > Am Freitag, 5. Juni 2020, 02:43:36 CEST schrieb Eric Biggers: > > > > > > Hi Eric, > > > > > > > On Thu, Jun 04, 2020 at 08:41:00AM +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+2e635807decef724a1fa@syzkaller.appspotmail.com > > > > > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B > > > > > ...") > > > > > Signed-off-by: Stephan Mueller <smueller@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; > > > > > > > > > > } > > > > > > > > It it okay that ->jent can be left as an ERR_PTR() value? > > > > > > > > Perhaps it should always be set to NULL? > > > > > > 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. > > > > 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. > > 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. > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] crypto: DRBG - always try to free Jitter RNG instance 2020-06-05 16:21 ` Eric Biggers @ 2020-06-07 13:07 ` Stephan Müller 0 siblings, 0 replies; 14+ messages in thread From: Stephan Müller @ 2020-06-07 13:07 UTC (permalink / raw) To: Eric Biggers Cc: Dan Carpenter, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] crypto: DRBG - always try to free Jitter RNG instance 2020-06-04 6:41 ` [PATCH v2] " Stephan Müller 2020-06-05 0:43 ` Eric Biggers @ 2020-06-07 13:20 ` Stephan Müller 2020-06-12 6:49 ` Herbert Xu 1 sibling, 1 reply; 14+ messages in thread From: Stephan Müller @ 2020-06-07 13:20 UTC (permalink / raw) To: Dan Carpenter Cc: davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, syzbot, Eric Biggers, Dan Carpenter The Jitter RNG is unconditionally allocated as a seed source follwoing the patch 97f2650e5040. Thus, the instance must always be deallocated. Reported-by: syzbot+2e635807decef724a1fa@syzkaller.appspotmail.com Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...") Signed-off-by: Stephan Mueller <smueller@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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3] crypto: DRBG - always try to free Jitter RNG instance 2020-06-07 13:20 ` [PATCH v3] " Stephan Müller @ 2020-06-12 6:49 ` Herbert Xu 0 siblings, 0 replies; 14+ messages in thread From: Herbert Xu @ 2020-06-12 6:49 UTC (permalink / raw) To: Stephan Müller Cc: Dan Carpenter, davem, linux-crypto, linux-kernel, syzkaller-bugs, syzbot, Eric Biggers 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+2e635807decef724a1fa@syzkaller.appspotmail.com > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...") > Signed-off-by: Stephan Mueller <smueller@chronox.de> > --- > crypto/drbg.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) Patch applied. 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-06-12 6:49 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-03 3:41 memory leak in crypto_create_tfm syzbot 2020-06-03 3:55 ` Eric Biggers 2020-06-03 8:08 ` [PATCH] crypto: DRBG - always try to free Jitter RNG instance Stephan Müller 2020-06-03 11:09 ` Dan Carpenter 2020-06-03 11:40 ` Stephan Mueller 2020-06-04 6:41 ` [PATCH v2] " Stephan Müller 2020-06-05 0:43 ` Eric Biggers 2020-06-05 5:58 ` Stephan Mueller 2020-06-05 6:16 ` Eric Biggers 2020-06-05 6:52 ` Stephan Mueller 2020-06-05 16:21 ` Eric Biggers 2020-06-07 13:07 ` Stephan Müller 2020-06-07 13:20 ` [PATCH v3] " Stephan Müller 2020-06-12 6:49 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).