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: [email protected]
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 [email protected].
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
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: [email protected]
>
> 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 [email protected].
>
> 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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/0000000000002a280b05a725cd93%40google.com.
The Jitter RNG is unconditionally allocated as a seed source follwoing
the patch 97f2650e5040. Thus, the instance must always be deallocated.
Reported-by: [email protected]
Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
Signed-off-by: Stephan Mueller <[email protected]>
---
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
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: [email protected]
> Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> Signed-off-by: Stephan Mueller <[email protected]>
> ---
> 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
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: [email protected]
> > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> > Signed-off-by: Stephan Mueller <[email protected]>
> > ---
> >
> > 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
The Jitter RNG is unconditionally allocated as a seed source follwoing
the patch 97f2650e5040. Thus, the instance must always be deallocated.
Reported-by: [email protected]
Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
Signed-off-by: Stephan Mueller <[email protected]>
---
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
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: [email protected]
> Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> Signed-off-by: Stephan Mueller <[email protected]>
> ---
> 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
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: [email protected]
> > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> > Signed-off-by: Stephan Mueller <[email protected]>
> > ---
> >
> > 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
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: [email protected]
> > > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> > > Signed-off-by: Stephan Mueller <[email protected]>
> > > ---
> > >
> > > 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
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: [email protected]
> > > > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B
> > > > ...")
> > > > Signed-off-by: Stephan Mueller <[email protected]>
> > > > ---
> > > >
> > > > 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
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: [email protected]
> > > > > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B
> > > > > ...")
> > > > > Signed-off-by: Stephan Mueller <[email protected]>
> > > > > ---
> > > > >
> > > > > 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
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
The Jitter RNG is unconditionally allocated as a seed source follwoing
the patch 97f2650e5040. Thus, the instance must always be deallocated.
Reported-by: [email protected]
Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
Signed-off-by: Stephan Mueller <[email protected]>
---
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