Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 3 Jun 2020 14:09:19 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Stephan =?iso-8859-1?Q?M=FCller?= <smueller@chronox.de>
Cc:     davem@davemloft.net, herbert@gondor.apana.org.au,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
        syzkaller-bugs@googlegroups.com,
        syzbot <syzbot+2e635807decef724a1fa@syzkaller.appspotmail.com>
Subject: Re: [PATCH] crypto: DRBG - always try to free Jitter RNG instance
Message-ID: <20200603110919.GK30374@kadam>
References: <0000000000002a280b05a725cd93@google.com>
 <2583872.mvXUDI8C0e@positron.chronox.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2583872.mvXUDI8C0e@positron.chronox.de>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

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