| Message ID | 2583872.mvXUDI8C0e@positron.chronox.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: DRBG - always try to free Jitter RNG instance | expand |
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
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
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; }
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(+)