[PATCH 1/6] hw_random: explicit ordering of initcalls

[PATCH 1/6] hw_random: explicit ordering of initcalls

[Dominik Brodowski]

hw-random device drivers depend on the hw-random core being
initialized. Make this ordering explicit, also for the case
these drivers are built-in. As the core itself depends on
misc_register() which is set up at subsys_initcall time,
advance the initialization of the core (only) to the
fs_initcall() level.

Cc: Matt Mackall <mpm@selenic.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/hw_random/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index a3db27916256..e860e044b19e 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -638,7 +638,7 @@ static void __exit hwrng_modexit(void)
 	unregister_miscdev();
 }
 
-module_init(hwrng_modinit);
+fs_initcall(hwrng_modinit); /* depends on misc_register() */
 module_exit(hwrng_modexit);
 
 MODULE_DESCRIPTION("H/W Random Number Generator (RNG) driver");
-- 
2.34.1

[PATCH 2/6] hw_random: read() callback must be called for size of 32 or more bytes

[Dominik Brodowski]

According to <linux/hw_random.h>, the @max parameter of the ->read
callback "is a multiple of 4 and >= 32 bytes". That promise was not
kept by add_early_randomness(), which only asked for 16 bytes. As
rng_buffer_size() is at least 32, we can simply ask for 32 bytes.

Cc: Matt Mackall <mpm@selenic.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/hw_random/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index e860e044b19e..c2d260b5dd92 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -64,10 +64,9 @@ static size_t rng_buffer_size(void)
 static void add_early_randomness(struct hwrng *rng)
 {
 	int bytes_read;
-	size_t size = min_t(size_t, 16, rng_buffer_size());
 
 	mutex_lock(&reading_mutex);
-	bytes_read = rng_get_data(rng, rng_buffer, size, 0);
+	bytes_read = rng_get_data(rng, rng_buffer, 32, 0);
 	mutex_unlock(&reading_mutex);
 	if (bytes_read > 0)
 		add_device_randomness(rng_buffer, bytes_read);
-- 
2.34.1

[PATCH 3/6] hw_random: use rng_fillbuf in add_early_randomness()

[Dominik Brodowski]

Using rng_buffer in add_early_randomness() may race with rng_dev_read().
Use rng_fillbuf instead, as it is otherwise only used within the kernel
by hwrng_fillfn() and therefore never exposed to userspace.

Cc: Matt Mackall <mpm@selenic.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/hw_random/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c2d260b5dd92..89891ac87af0 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -66,10 +66,10 @@ static void add_early_randomness(struct hwrng *rng)
 	int bytes_read;
 
 	mutex_lock(&reading_mutex);
-	bytes_read = rng_get_data(rng, rng_buffer, 32, 0);
+	bytes_read = rng_get_data(rng, rng_fillbuf, 32, 0);
 	mutex_unlock(&reading_mutex);
 	if (bytes_read > 0)
-		add_device_randomness(rng_buffer, bytes_read);
+		add_device_randomness(rng_fillbuf, bytes_read);
 }
 
 static inline void cleanup_rng(struct kref *kref)
-- 
2.34.1

[PATCH 4/6] hw_random: only set cur_rng_set_by_user if it is working

[Dominik Brodowski]

In case the user-specified rng device is not working, it is not used;
therefore cur_rng_set_by_user must not be set to 1.

Cc: Matt Mackall <mpm@selenic.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/hw_random/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 89891ac87af0..9405fcdace38 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -335,8 +335,9 @@ static ssize_t rng_current_store(struct device *dev,
 	} else {
 		list_for_each_entry(rng, &rng_list, list) {
 			if (sysfs_streq(rng->name, buf)) {
-				cur_rng_set_by_user = 1;
 				err = set_current_rng(rng);
+				if (!err)
+					cur_rng_set_by_user = 1;
 				break;
 			}
 		}
-- 
2.34.1

[PATCH 5/6] hw_random: break out of hwrng_fillfn if current rng is not trusted

[Dominik Brodowski]

For two reasons, current_quality may become zero within the rngd
kernel thread: (1) The user lowers current_quality to 0 by writing
to the sysfs module parameter file (note that increasing the quality
from zero is without effect at the moment), or (2) there are two or
more hwrng devices registered, and those which provide quality>0 are
unregistered, but one with quality==0 remains.

If current_quality is 0, the randomness is not trusted and cannot help
to increase the entropy count. That will lead to continuous calls to
the hwrngd thread and continuous stirring of the input pool with
untrusted bits.

Cc: Matt Mackall <mpm@selenic.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/hw_random/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 9405fcdace38..bc9f95cbac92 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -429,6 +429,9 @@ static int hwrng_fillfn(void *unused)
 	while (!kthread_should_stop()) {
 		struct hwrng *rng;
 
+		if (!current_quality)
+			break;
+
 		rng = get_current_rng();
 		if (IS_ERR(rng) || !rng)
 			break;
-- 
2.34.1

[PATCH 6/6] hw_random: credit entropy for low quality sources of randomness

[Dominik Brodowski]

In case the entropy quality is low, there may be less than one bit to
credit in the call to add_hwgenerator_randomness(): The number of bytes
returned by rng_get_data() multiplied by the current quality (in entropy
bits per 1024 bits of input) must be larger than 128 to credit at least
one bit. However, imx-rngc.c sets the quality to 19, but may return less
than 32 bytes; hid_u2fzero.c sets the quality to 1; and users may override
the quality setting manually.

In case there is less than one bit to credit, keep track of it and add
that credit to the next iteration.

Cc: Matt Mackall <mpm@selenic.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/hw_random/core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index bc9f95cbac92..6d7f05641c7c 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -427,6 +427,7 @@ static int hwrng_fillfn(void *unused)
 	long rc;
 
 	while (!kthread_should_stop()) {
+		size_t entropy, entropy_credit = 0; /* in 1/1024 of a bit */
 		struct hwrng *rng;
 
 		if (!current_quality)
@@ -445,9 +446,17 @@ static int hwrng_fillfn(void *unused)
 			msleep_interruptible(10000);
 			continue;
 		}
+
+		/* If we cannot credit at least one bit of entropy,
+		 * keep track of the remainder for the next iteration
+		 */
+		entropy = rc * current_quality * 8 + entropy_credit;
+		if ((entropy >> 10) == 0)
+			entropy_credit = entropy;
+
 		/* Outside lock, sure, but y'know: randomness. */
 		add_hwgenerator_randomness((void *)rng_fillbuf, rc,
-					   rc * current_quality * 8 >> 10);
+					   entropy >> 10);
 	}
 	hwrng_fill = NULL;
 	return 0;
-- 
2.34.1

[PATCH v2 6/6] hw_random: credit entropy for low quality sources of randomness

[Dominik Brodowski]

In case the entropy quality is low, there may be less than one bit to
credit in the call to add_hwgenerator_randomness(): The number of bytes
returned by rng_get_data() multiplied by the current quality (in entropy
bits per 1024 bits of input) must be larger than 128 to credit at least
one bit. However, imx-rngc.c sets the quality to 19, but may return less
than 32 bytes; hid_u2fzero.c sets the quality to 1; and users may override
the quality setting manually.

In case there is less than one bit to credit, keep track of it and add
that credit to the next iteration.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/hw_random/core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

This patch needed an update, as noted by the kernel test robot: the
initialiation of entropy_credit = 0 must be outside the loop.

That's the only change between v1 and v2 of this patch. The other five
patches sent earlier do not (yet?) need an update.

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index bc9f95cbac92..f327f7493585 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -424,6 +424,7 @@ static int __init register_miscdev(void)
 
 static int hwrng_fillfn(void *unused)
 {
+	size_t entropy, entropy_credit = 0; /* in 1/1024 of a bit */
 	long rc;
 
 	while (!kthread_should_stop()) {
@@ -445,9 +446,17 @@ static int hwrng_fillfn(void *unused)
 			msleep_interruptible(10000);
 			continue;
 		}
+
+		/* If we cannot credit at least one bit of entropy,
+		 * keep track of the remainder for the next iteration
+		 */
+		entropy = rc * current_quality * 8 + entropy_credit;
+		if ((entropy >> 10) == 0)
+			entropy_credit = entropy;
+
 		/* Outside lock, sure, but y'know: randomness. */
 		add_hwgenerator_randomness((void *)rng_fillbuf, rc,
-					   rc * current_quality * 8 >> 10);
+					   entropy >> 10);
 	}
 	hwrng_fill = NULL;
 	return 0;
-- 
2.35.0

Re: [PATCH 1/6] hw_random: explicit ordering of initcalls

[Herbert Xu]

On Mon, Jan 24, 2022 at 09:29:46PM +0100, Dominik Brodowski wrote:
> hw-random device drivers depend on the hw-random core being
> initialized. Make this ordering explicit, also for the case
> these drivers are built-in. As the core itself depends on
> misc_register() which is set up at subsys_initcall time,
> advance the initialization of the core (only) to the
> fs_initcall() level.
> 
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
> ---
>  drivers/char/hw_random/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt