Re: [PATCH] crypto: DRBG - always try to free Jitter RNG instance

[Stephan Mueller <smueller@chronox.de>]

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