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

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> Signed-off-by: Stephan Mueller <smueller@xxxxxxxxxx>
> ---
>  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