Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Mon, 07 Nov 2022 10:24:42 +0100
From:   Harald Freudenberger <freude@linux.ibm.com>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     herbert@gondor.apana.org.au, linux-crypto@vger.kernel.org,
        Ard Biesheuvel <ardb@kernel.org>,
        Holger Dengler <dengler@linux.ibm.com>
Subject: Re: [PATCH] hw_random: treat default_quality as a maximum and default
 to 1024
Reply-To: freude@linux.ibm.com
In-Reply-To: <20221104154230.52836-1-Jason@zx2c4.com>
References: <20221104154230.52836-1-Jason@zx2c4.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <a0863b503b22b42fb8129b6847188a2e@linux.ibm.com>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022-11-04 16:42, Jason A. Donenfeld wrote:
> Most hw_random devices return entropy which is assumed to be of full
> quality, but driver authors don't bother setting the quality knob. Some
> hw_random devices return less than full quality entropy, and then 
> driver
> authors set the quality knob. Therefore, the entropy crediting should 
> be
> opt-out rather than opt-in per-driver, to reflect the actual reality on
> the ground.
> 
> For example, the two Raspberry Pi RNG drivers produce full entropy
> randomness, and both EDK2 and U-Boot's drivers for these treat them as
> such. The result is that EFI then uses these numbers and passes the to
> Linux, and Linux credits them as boot, thereby initializing the RNG.
> Yet, in Linux, the quality knob was never set to anything, and so on 
> the
> chance that Linux is booted without EFI, nothing is ever credited.
> That's annoying.
> 
> The same pattern appears to repeat itself throughout various drivers. 
> In
> fact, very very few drivers have bothered setting quality=1024.
> 
> So let's invert this logic. A hw_random struct's quality knob now
> controls the maximum quality a driver can produce, or 0 to specify 
> 1024.
> Then, the module-wide switch called "default_quality" is changed to
> represent the maximum quality of any driver. By default it's 1024, and
> the quality of any particular driver is then given by:
> 
>     min(default_quality, rng->quality ?: 1024);
> 
> This way, the user can still turn this off for weird reasons, yet we 
> get
> proper crediting for relevant RNGs.
> 
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  arch/um/drivers/random.c           | 1 -
>  drivers/char/hw_random/core.c      | 9 +++------
>  drivers/char/hw_random/mpfs-rng.c  | 1 -
>  drivers/char/hw_random/s390-trng.c | 1 -
>  drivers/crypto/atmel-sha204a.c     | 1 -
>  drivers/crypto/caam/caamrng.c      | 1 -
>  drivers/firmware/turris-mox-rwtm.c | 1 -
>  drivers/usb/misc/chaoskey.c        | 1 -
>  include/linux/hw_random.h          | 2 +-
>  9 files changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/um/drivers/random.c b/arch/um/drivers/random.c
> index 32b3341fe970..da985e0dc69a 100644
> --- a/arch/um/drivers/random.c
> +++ b/arch/um/drivers/random.c
> @@ -82,7 +82,6 @@ static int __init rng_init (void)
>  	sigio_broken(random_fd);
>  	hwrng.name = RNG_MODULE_NAME;
>  	hwrng.read = rng_dev_read;
> -	hwrng.quality = 1024;
> 
>  	err = hwrng_register(&hwrng);
>  	if (err) {
> diff --git a/drivers/char/hw_random/core.c 
> b/drivers/char/hw_random/core.c
> index cc002b0c2f0c..afde685f5e0a 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -41,14 +41,14 @@ static DEFINE_MUTEX(reading_mutex);
>  static int data_avail;
>  static u8 *rng_buffer, *rng_fillbuf;
>  static unsigned short current_quality;
> -static unsigned short default_quality; /* = 0; default to "off" */
> +static unsigned short default_quality = 1024; /* default to maximum */
> 
>  module_param(current_quality, ushort, 0644);
>  MODULE_PARM_DESC(current_quality,
>  		 "current hwrng entropy estimation per 1024 bits of input --
> obsolete, use rng_quality instead");
>  module_param(default_quality, ushort, 0644);
>  MODULE_PARM_DESC(default_quality,
> -		 "default entropy content of hwrng per 1024 bits of input");
> +		 "default maximum entropy content of hwrng per 1024 bits of input");
> 
>  static void drop_current_rng(void);
>  static int hwrng_init(struct hwrng *rng);
> @@ -170,10 +170,7 @@ static int hwrng_init(struct hwrng *rng)
>  	reinit_completion(&rng->cleanup_done);
> 
>  skip_init:
> -	if (!rng->quality)
> -		rng->quality = default_quality;
> -	if (rng->quality > 1024)
> -		rng->quality = 1024;
> +	rng->quality = min_t(u16, min_t(u16, default_quality, 1024),
> rng->quality ?: 1024);
>  	current_quality = rng->quality; /* obsolete */
> 
>  	return 0;
> diff --git a/drivers/char/hw_random/mpfs-rng.c
> b/drivers/char/hw_random/mpfs-rng.c
> index 5813da617a48..c6972734ae62 100644
> --- a/drivers/char/hw_random/mpfs-rng.c
> +++ b/drivers/char/hw_random/mpfs-rng.c
> @@ -78,7 +78,6 @@ static int mpfs_rng_probe(struct platform_device 
> *pdev)
> 
>  	rng_priv->rng.read = mpfs_rng_read;
>  	rng_priv->rng.name = pdev->name;
> -	rng_priv->rng.quality = 1024;
> 
>  	platform_set_drvdata(pdev, rng_priv);
> 
> diff --git a/drivers/char/hw_random/s390-trng.c
> b/drivers/char/hw_random/s390-trng.c
> index 795853dfc46b..cffa326ddc8d 100644
> --- a/drivers/char/hw_random/s390-trng.c
> +++ b/drivers/char/hw_random/s390-trng.c
> @@ -191,7 +191,6 @@ static struct hwrng trng_hwrng_dev = {
>  	.name		= "s390-trng",
>  	.data_read	= trng_hwrng_data_read,
>  	.read		= trng_hwrng_read,
> -	.quality	= 1024,
>  };
> 
> 
> diff --git a/drivers/crypto/atmel-sha204a.c 
> b/drivers/crypto/atmel-sha204a.c
> index a84b657598c6..c0103e7fc2e7 100644
> --- a/drivers/crypto/atmel-sha204a.c
> +++ b/drivers/crypto/atmel-sha204a.c
> @@ -107,7 +107,6 @@ static int atmel_sha204a_probe(struct i2c_client 
> *client,
> 
>  	i2c_priv->hwrng.name = dev_name(&client->dev);
>  	i2c_priv->hwrng.read = atmel_sha204a_rng_read;
> -	i2c_priv->hwrng.quality = 1024;
> 
>  	ret = devm_hwrng_register(&client->dev, &i2c_priv->hwrng);
>  	if (ret)
> diff --git a/drivers/crypto/caam/caamrng.c 
> b/drivers/crypto/caam/caamrng.c
> index 77d048dfe5d0..1f0e82050976 100644
> --- a/drivers/crypto/caam/caamrng.c
> +++ b/drivers/crypto/caam/caamrng.c
> @@ -246,7 +246,6 @@ int caam_rng_init(struct device *ctrldev)
>  	ctx->rng.cleanup = caam_cleanup;
>  	ctx->rng.read    = caam_read;
>  	ctx->rng.priv    = (unsigned long)ctx;
> -	ctx->rng.quality = 1024;
> 
>  	dev_info(ctrldev, "registering rng-caam\n");
> 
> diff --git a/drivers/firmware/turris-mox-rwtm.c
> b/drivers/firmware/turris-mox-rwtm.c
> index c2d34dc8ba46..6ea5789a89e2 100644
> --- a/drivers/firmware/turris-mox-rwtm.c
> +++ b/drivers/firmware/turris-mox-rwtm.c
> @@ -528,7 +528,6 @@ static int turris_mox_rwtm_probe(struct
> platform_device *pdev)
>  	rwtm->hwrng.name = DRIVER_NAME "_hwrng";
>  	rwtm->hwrng.read = mox_hwrng_read;
>  	rwtm->hwrng.priv = (unsigned long) rwtm;
> -	rwtm->hwrng.quality = 1024;
> 
>  	ret = devm_hwrng_register(dev, &rwtm->hwrng);
>  	if (ret < 0) {
> diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c
> index 87067c3d6109..6fb5140e29b9 100644
> --- a/drivers/usb/misc/chaoskey.c
> +++ b/drivers/usb/misc/chaoskey.c
> @@ -200,7 +200,6 @@ static int chaoskey_probe(struct usb_interface 
> *interface,
> 
>  	dev->hwrng.name = dev->name ? dev->name : chaoskey_driver.name;
>  	dev->hwrng.read = chaoskey_rng_read;
> -	dev->hwrng.quality = 1024;
> 
>  	dev->hwrng_registered = (hwrng_register(&dev->hwrng) == 0);
>  	if (!dev->hwrng_registered)
> diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
> index 77c2885c4c13..8a3115516a1b 100644
> --- a/include/linux/hw_random.h
> +++ b/include/linux/hw_random.h
> @@ -34,7 +34,7 @@
>   * @priv:		Private data, for use by the RNG driver.
>   * @quality:		Estimation of true entropy in RNG's bitstream
>   *			(in bits of entropy per 1024 bits of input;
> - *			valid values: 1 to 1024, or 0 for unknown).
> + *			valid values: 1 to 1024, or 0 for maximum).
>   */
>  struct hwrng {
>  	const char *name;

Well, I am not sure if this is the right way to go. So by default a
hw rng which does not implement the registration correctly is
rewarded with the implicit assumption that it produces 100% of
entropy.
I see your point - a grep through the kernel code gives the impression
that a whole bunch of registrations is done with an empty quality
field. What about assuming a default quality of 50% if the field
is not filled ?