Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Stephan Mueller <smueller@chronox.de>
To:     Yann Droneaud <ydroneaud@opteya.com>, linux-crypto@vger.kernel.org
Cc:     Herbert Xu <herbert@gondor.apana.org.au>
Subject: Re: [PATCH v5] crypto: DRBG - add FIPS 140-2 CTRNG for noise source
Date:   Tue, 07 May 2019 15:34:50 +0200
Message-ID: <2429866.3tyyabiZt7@tauon.chronox.de>
In-Reply-To: <74c517ac2c654a7372af731a67e24743c843e157.camel@opteya.com>
References: <1852500.fyBc0DU23F@positron.chronox.de> <1654549.mqJkfNR9fV@positron.chronox.de> <74c517ac2c654a7372af731a67e24743c843e157.camel@opteya.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

Am Dienstag, 7. Mai 2019, 15:10:38 CEST schrieb Yann Droneaud:

Hi Yann,

> > +	if (IS_ENABLED(CONFIG_CRYPTO_FIPS)) {
> 
> 	if (!IS_ENABLED(CONFIG_CRYPTO_FIPS))
> 		return 0;

Changed
> 
> > +		unsigned short entropylen = drbg_sec_strength(drbg->core-
>flags);
> > +		int ret = 0;
> > +
> > +		/* skip test if we test the overall system */
> > +		if (list_empty(&drbg->test_data.list))
> > +			return 0;
> > +		/* only perform test in FIPS mode */
> > +		if (!fips_enabled)
> > +			return 0;
> > +
> > +		if (!drbg->fips_primed) {
> > +			/* Priming of FIPS test */
> > +			memcpy(drbg->prev, entropy, entropylen);
> > +			drbg->fips_primed = true;
> > +			/* priming: another round is needed */
> > +			return -EAGAIN;
> > +		}
> > +		ret = memcmp(drbg->prev, entropy, entropylen);
> > +		if (!ret)
> > +			panic("DRBG continuous self test failed\n");
> 
> Previous version from commit b3614763059b ("crypto: drbg - remove FIPS
> 140-2 continuous test") already has it, and so does the "self test" in
> drivers/char/random.c, but do we really want to panic() in the
> unlikely, but still possible, event of a duplicated output from the
> PRNG ? The longer the system is up, the likelier this can happen ... if
> one can wait for the end of the universe :)

Yes, we want that in FIPS mode.

The requirement is that the "crypto module" (i.e. the kernel crypto API) must 
become unavailable if that issue happens.

Commonly this can only be achieved in two ways: either we have a kind of 
global flag that effectively deactivates the kernel crypto API or we terminate 
the kernel.
> 
> > +		memcpy(drbg->prev, entropy, entropylen);
> > +		/* the test shall pass when the two values are not equal */
> > +		return (ret != 0) ? 0 : -EFAULT;
> 
> Here, it's not possible to have ret == 0, since that would panic(), so
> -EFAULT cannot be returned.

Agreed.
> 
> > +	} else {
> > +		return 0;
> > +	}
> > +}
> > +
> > 
> >  /*
> >  
> >   * Convert an integer into a byte representation of this integer.
> >   * The byte representation is big-endian
> > 
> > @@ -1006,16 +1057,23 @@ static void drbg_async_seed(struct work_struct
> > *work)> 
> >  					       seed_work);
> >  	
> >  	unsigned int entropylen = drbg_sec_strength(drbg->core->flags);
> >  	unsigned char entropy[32];
> > 
> > +	int ret;
> > 
> >  	BUG_ON(!entropylen);
> >  	BUG_ON(entropylen > sizeof(entropy));
> > 
> > -	get_random_bytes(entropy, entropylen);
> > 
> >  	drbg_string_fill(&data, entropy, entropylen);
> >  	list_add_tail(&data.list, &seedlist);
> >  	
> >  	mutex_lock(&drbg->drbg_mutex);
> > 
> > +	do {
> > +		get_random_bytes(entropy, entropylen);
> > +		ret = drbg_fips_continuous_test(drbg, entropy);
> > +		if (ret && ret != -EAGAIN)
> > +			goto unlock;
> > +	} while (ret);
> > +
> 
> A function doing get_random_bytes() and continous_test() would be
> useful to both sync and async seed function.

Changed

Thanks.

Ciao
Stephan