Re: [PATCH v2] Allow hwrng to initialize crng.
On Thu, Dec 13, 2018 at 5:48 PM Ard Biesheuvel
wrote:
>
> On Thu, 13 Dec 2018 at 10:18, Louis Collard wrote:
> >
> > Some systems, for example embedded systems, do not generate
> > enough entropy on boot through interrupts, and boot may be blocked for
> > several minutes waiting for a call to getrandom to complete.
> >
> > Currently, random data is read from a hwrng when it is registered,
> > and is loaded into primary_crng. This data is treated in the same
> > way as data that is device-specific but otherwise unchanging, and
> > so primary_crng cannot become initialized with the data from the
> > hwrng.
> >
> > This change causes the data initially read from the hwrng to be
> > treated the same as subsequent data that is read from the hwrng if
> > it's quality score is non-zero.
> >
> > The implications of this are:
> >
> > The data read from hwrng can cause primary_crng to become
> > initialized, therefore avoiding problems of getrandom blocking
> > on boot.
> >
> > Calls to getrandom (with GRND_RANDOM) may be using entropy
> > exclusively (or in practise, almost exclusively) from the hwrng.
> >
> > Regarding the latter point; this behavior is the same as if a
> > user specified a quality score of 1 (bit of entropy per 1024 bits)
> > so hopefully this is not too scary a change to make.
> >
> > This change is the result of the discussion here:
> > https://patchwork.kernel.org/patch/10453893/
> >
> > Signed-off-by: Louis Collard
> > Acked-by: Jarkko Sakkinen
> > ---
> > drivers/char/hw_random/core.c | 11 +--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index 95be7228f327..7736e1a96321 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -24,6 +24,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #define RNG_MODULE_NAME"hw_random"
> >
> > @@ -64,13 +65,19 @@ 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());
> > + /* Read enough to initialize crng. */
> > + size_t size = min_t(size_t,
> > + 2*CHACHA20_KEY_SIZE,
>
> This should be as symbolic constant that retains its meaning even if
> we move away from ChaCha20 or modify the current implementation
>
> > + rng_buffer_size());
> >
> > mutex_lock(_mutex);
> > bytes_read = rng_get_data(rng, rng_buffer, size, 1);
> > mutex_unlock(_mutex);
> > if (bytes_read > 0)
> > - add_device_randomness(rng_buffer, bytes_read);
> > + /* Allow crng to become initialized, but do not add
> > +* entropy to the pool.
> > +*/
> > + add_hwgenerator_randomness(rng_buffer, bytes_read, 0);
> > }
> >
> > static inline void cleanup_rng(struct kref *kref)
> > --
> > 2.13.5
> >
Right, this should be [equal to] CRNG_INIT_CNT_THRESH from random.c -
I wasn't sure where/how to pull this out to though..
Re: [RESEND PATCH] Allow hwrng to initialize crng.
On Thu, Dec 13, 2018 at 4:51 PM Herbert Xu wrote:
>
> On Thu, Dec 13, 2018 at 04:40:17PM +0800, Louis Collard wrote:
> > Some systems, for example embedded systems, do not generate
> > enough entropy on boot through interrupts, and boot may be blocked for
> > several minutes waiting for a call to getrandom to complete.
> >
> > Currently, random data is read from a hwrng when it is registered,
> > and is loaded into primary_crng. This data is treated in the same
> > way as data that is device-specific but otherwise unchanging, and
> > so primary_crng cannot become initialized with the data from the
> > hwrng.
> >
> > This change causes the data initially read from the hwrng to be
> > treated the same as subsequent data that is read from the hwrng if
> > it's quality score is non-zero.
> >
> > The implications of this are:
> >
> > The data read from hwrng can cause primary_crng to become
> > initialized, therefore avoiding problems of getrandom blocking
> > on boot.
> >
> > Calls to getrandom (with GRND_RANDOM) may be using entropy
> > exclusively (or in practise, almost exclusively) from the hwrng.
> >
> > Regarding the latter point; this behavior is the same as if a
> > user specified a quality score of 1 (bit of entropy per 1024 bits)
> > so hopefully this is not too scary a change to make.
> >
> > This change is the result of the discussion here:
> > https://patchwork.kernel.org/patch/10453893/
> >
> > Signed-off-by: Louis Collard
> > Acked-by: Jarkko Sakkinen
> > ---
> > drivers/char/hw_random/core.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index 95be7228f327..99c3e4127949 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -24,6 +24,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #define RNG_MODULE_NAME "hw_random"
> >
> > @@ -64,13 +65,17 @@ 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());
> > + /* Read enough to initialize crng. */
> > + size_t size = 2*CHACHA20_KEY_SIZE;
> >
> > mutex_lock(_mutex);
> > bytes_read = rng_get_data(rng, rng_buffer, size, 1);
> > mutex_unlock(_mutex);
> > if (bytes_read > 0)
> > - add_device_randomness(rng_buffer, bytes_read);
> > + /* Allow crng to become initialized, but do not add
> > + * entropy to the pool.
> > + */
> > + add_hwgenerator_randomness(rng_buffer, bytes_read, 0);
> > }
> >
> > static inline void cleanup_rng(struct kref *kref)
> > --
> > 2.13.5
> >
>
> Adding Ted Ts'o to the cc list.
> --
> Email: Herbert Xu
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Thanks - and apologies, just re-read this and realized I clearly
wasn't paying attention the first time, have sent a v2 that calculates
size correctly.
[PATCH v2] Allow hwrng to initialize crng.
Some systems, for example embedded systems, do not generate
enough entropy on boot through interrupts, and boot may be blocked for
several minutes waiting for a call to getrandom to complete.
Currently, random data is read from a hwrng when it is registered,
and is loaded into primary_crng. This data is treated in the same
way as data that is device-specific but otherwise unchanging, and
so primary_crng cannot become initialized with the data from the
hwrng.
This change causes the data initially read from the hwrng to be
treated the same as subsequent data that is read from the hwrng if
it's quality score is non-zero.
The implications of this are:
The data read from hwrng can cause primary_crng to become
initialized, therefore avoiding problems of getrandom blocking
on boot.
Calls to getrandom (with GRND_RANDOM) may be using entropy
exclusively (or in practise, almost exclusively) from the hwrng.
Regarding the latter point; this behavior is the same as if a
user specified a quality score of 1 (bit of entropy per 1024 bits)
so hopefully this is not too scary a change to make.
This change is the result of the discussion here:
https://patchwork.kernel.org/patch/10453893/
Signed-off-by: Louis Collard
Acked-by: Jarkko Sakkinen
---
drivers/char/hw_random/core.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 95be7228f327..7736e1a96321 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#define RNG_MODULE_NAME"hw_random"
@@ -64,13 +65,19 @@ 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());
+ /* Read enough to initialize crng. */
+ size_t size = min_t(size_t,
+ 2*CHACHA20_KEY_SIZE,
+ rng_buffer_size());
mutex_lock(_mutex);
bytes_read = rng_get_data(rng, rng_buffer, size, 1);
mutex_unlock(_mutex);
if (bytes_read > 0)
- add_device_randomness(rng_buffer, bytes_read);
+ /* Allow crng to become initialized, but do not add
+* entropy to the pool.
+*/
+ add_hwgenerator_randomness(rng_buffer, bytes_read, 0);
}
static inline void cleanup_rng(struct kref *kref)
--
2.13.5
[RESEND PATCH] Allow hwrng to initialize crng.
Some systems, for example embedded systems, do not generate
enough entropy on boot through interrupts, and boot may be blocked for
several minutes waiting for a call to getrandom to complete.
Currently, random data is read from a hwrng when it is registered,
and is loaded into primary_crng. This data is treated in the same
way as data that is device-specific but otherwise unchanging, and
so primary_crng cannot become initialized with the data from the
hwrng.
This change causes the data initially read from the hwrng to be
treated the same as subsequent data that is read from the hwrng if
it's quality score is non-zero.
The implications of this are:
The data read from hwrng can cause primary_crng to become
initialized, therefore avoiding problems of getrandom blocking
on boot.
Calls to getrandom (with GRND_RANDOM) may be using entropy
exclusively (or in practise, almost exclusively) from the hwrng.
Regarding the latter point; this behavior is the same as if a
user specified a quality score of 1 (bit of entropy per 1024 bits)
so hopefully this is not too scary a change to make.
This change is the result of the discussion here:
https://patchwork.kernel.org/patch/10453893/
Signed-off-by: Louis Collard
Acked-by: Jarkko Sakkinen
---
drivers/char/hw_random/core.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 95be7228f327..99c3e4127949 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#define RNG_MODULE_NAME"hw_random"
@@ -64,13 +65,17 @@ 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());
+ /* Read enough to initialize crng. */
+ size_t size = 2*CHACHA20_KEY_SIZE;
mutex_lock(_mutex);
bytes_read = rng_get_data(rng, rng_buffer, size, 1);
mutex_unlock(_mutex);
if (bytes_read > 0)
- add_device_randomness(rng_buffer, bytes_read);
+ /* Allow crng to become initialized, but do not add
+* entropy to the pool.
+*/
+ add_hwgenerator_randomness(rng_buffer, bytes_read, 0);
}
static inline void cleanup_rng(struct kref *kref)
--
2.13.5
Re: [PATCH] Allow hwrng to initialize crng.
On Sun, Nov 18, 2018 at 4:15 AM Michael Niewöhner wrote:
>
> Hi Louis,
>
> On Wed, 2018-09-26 at 11:24 +0800, Louis Collard wrote:
> > Some systems, for example embedded systems, do not generate
> > enough entropy on boot through interrupts, and boot may be blocked for
> > several minutes waiting for a call to getrandom to complete.
> >
> > Currently, random data is read from a hwrng when it is registered,
> > and is loaded into primary_crng. This data is treated in the same
> > way as data that is device-specific but otherwise unchanging, and
> > so primary_crng cannot become initialized with the data from the
> > hwrng.
> >
> > This change causes the data initially read from the hwrng to be
> > treated the same as subsequent data that is read from the hwrng if
> > it's quality score is non-zero.
> >
> > The implications of this are:
> >
> > The data read from hwrng can cause primary_crng to become
> > initialized, therefore avoiding problems of getrandom blocking
> > on boot.
> >
> > Calls to getrandom (with GRND_RANDOM) may be using entropy
> > exclusively (or in practise, almost exclusively) from the hwrng.
> >
> > Regarding the latter point; this behavior is the same as if a
> > user specified a quality score of 1 (bit of entropy per 1024 bits)
> > so hopefully this is not too scary a change to make.
> >
> > This change is the result of the discussion here:
> > https://patchwork.kernel.org/patch/10453893/
> >
> > Signed-off-by: Louis Collard
> > Acked-by: Jarkko Sakkinen
> > ---
> > drivers/char/hw_random/core.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index aaf9e5afaad4..47f358aa0c3d 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -24,6 +24,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #define RNG_MODULE_NAME "hw_random"
> >
> > @@ -64,13 +65,17 @@ 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());
> > + /* Read enough to initialize crng. */
> > + size_t size = 2*CHACHA20_KEY_SIZE;
> >
> > mutex_lock(_mutex);
> > bytes_read = rng_get_data(rng, rng_buffer, size, 1);
> > mutex_unlock(_mutex);
> > if (bytes_read > 0)
> > - add_device_randomness(rng_buffer, bytes_read);
> > + /* Allow crng to become initialized, but do not add
> > + * entropy to the pool.
> > + */
> > + add_hwgenerator_randomness(rng_buffer, bytes_read, 0);
> > }
> >
> > static inline void cleanup_rng(struct kref *kref)
>
> I found your patch by chance, searching for a solution for crng init delay on
> my
> headless machine. Unfortunately it hardly makes any difference for me. With
> the
> patch the system hangs for about 80s instead of 120s until the "crng init
> done"
> message.In contrast, doing a `cat /dev/hwrng >/dev/random` or running rngd
> initializes the crng instantly.
>
> Isn't that delay the problem this patch tries to fix? Any idea what is wrong
> here?
>
> Thanks!
>
> Best regards
> Michael
>
>
Yes that is the problem this is trying to address. My guess would be
rng_get_data() is not returning as much data as requested, so the
delay is reduced but not eliminated. Looking at implementation of
rng_get_data() it appears this could be caused by device support for
read() vs data_read(). I don't have a good feel for whether looping to
retrieve more data here would be acceptable, it is certainly a bigger
change than currently proposed.
Thanks,
Louis
Re: [PATCH] Allow hwrng to initialize crng.
On Wed, Sep 26, 2018 at 6:35 PM Jarkko Sakkinen wrote: > > On Wed, Sep 26, 2018 at 11:24:55AM +0800, Louis Collard wrote: > > Some systems, for example embedded systems, do not generate > > enough entropy on boot through interrupts, and boot may be blocked for > > several minutes waiting for a call to getrandom to complete. > > > > Currently, random data is read from a hwrng when it is registered, > > and is loaded into primary_crng. This data is treated in the same > > way as data that is device-specific but otherwise unchanging, and > > so primary_crng cannot become initialized with the data from the > > hwrng. > > > > This change causes the data initially read from the hwrng to be > > treated the same as subsequent data that is read from the hwrng if > > it's quality score is non-zero. > > > > The implications of this are: > > > > The data read from hwrng can cause primary_crng to become > > initialized, therefore avoiding problems of getrandom blocking > > on boot. > > > > Calls to getrandom (with GRND_RANDOM) may be using entropy > > exclusively (or in practise, almost exclusively) from the hwrng. > > > > Regarding the latter point; this behavior is the same as if a > > user specified a quality score of 1 (bit of entropy per 1024 bits) > > so hopefully this is not too scary a change to make. > > > > This change is the result of the discussion here: > > https://patchwork.kernel.org/patch/10453893/ > > > > Signed-off-by: Louis Collard > > Acked-by: Jarkko Sakkinen > > /Jarkko Thanks! Adding a few people I think I should have included previously - I'm new to this process so apologies for any missteps; please let me know if there is anything else I can / should be doing!
Re: [PATCH] Allow hwrng to initialize crng.
On Wed, Sep 26, 2018 at 6:35 PM Jarkko Sakkinen wrote: > > On Wed, Sep 26, 2018 at 11:24:55AM +0800, Louis Collard wrote: > > Some systems, for example embedded systems, do not generate > > enough entropy on boot through interrupts, and boot may be blocked for > > several minutes waiting for a call to getrandom to complete. > > > > Currently, random data is read from a hwrng when it is registered, > > and is loaded into primary_crng. This data is treated in the same > > way as data that is device-specific but otherwise unchanging, and > > so primary_crng cannot become initialized with the data from the > > hwrng. > > > > This change causes the data initially read from the hwrng to be > > treated the same as subsequent data that is read from the hwrng if > > it's quality score is non-zero. > > > > The implications of this are: > > > > The data read from hwrng can cause primary_crng to become > > initialized, therefore avoiding problems of getrandom blocking > > on boot. > > > > Calls to getrandom (with GRND_RANDOM) may be using entropy > > exclusively (or in practise, almost exclusively) from the hwrng. > > > > Regarding the latter point; this behavior is the same as if a > > user specified a quality score of 1 (bit of entropy per 1024 bits) > > so hopefully this is not too scary a change to make. > > > > This change is the result of the discussion here: > > https://patchwork.kernel.org/patch/10453893/ > > > > Signed-off-by: Louis Collard > > Acked-by: Jarkko Sakkinen > > /Jarkko Thanks! Adding a few people I think I should have included previously - I'm new to this process so apologies for any missteps; please let me know if there is anything else I can / should be doing!
[PATCH] Allow hwrng to initialize crng.
Some systems, for example embedded systems, do not generate
enough entropy on boot through interrupts, and boot may be blocked for
several minutes waiting for a call to getrandom to complete.
Currently, random data is read from a hwrng when it is registered,
and is loaded into primary_crng. This data is treated in the same
way as data that is device-specific but otherwise unchanging, and
so primary_crng cannot become initialized with the data from the
hwrng.
This change causes the data initially read from the hwrng to be
treated the same as subsequent data that is read from the hwrng if
it's quality score is non-zero.
The implications of this are:
The data read from hwrng can cause primary_crng to become
initialized, therefore avoiding problems of getrandom blocking
on boot.
Calls to getrandom (with GRND_RANDOM) may be using entropy
exclusively (or in practise, almost exclusively) from the hwrng.
Regarding the latter point; this behavior is the same as if a
user specified a quality score of 1 (bit of entropy per 1024 bits)
so hopefully this is not too scary a change to make.
This change is the result of the discussion here:
https://patchwork.kernel.org/patch/10453893/
Signed-off-by: Louis Collard
---
drivers/char/hw_random/core.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aaf9e5afaad4..47f358aa0c3d 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#define RNG_MODULE_NAME"hw_random"
@@ -64,13 +65,17 @@ 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());
+ /* Read enough to initialize crng. */
+ size_t size = 2*CHACHA20_KEY_SIZE;
mutex_lock(_mutex);
bytes_read = rng_get_data(rng, rng_buffer, size, 1);
mutex_unlock(_mutex);
if (bytes_read > 0)
- add_device_randomness(rng_buffer, bytes_read);
+ /* Allow crng to become initialized, but do not add
+* entropy to the pool.
+*/
+ add_hwgenerator_randomness(rng_buffer, bytes_read, 0);
}
static inline void cleanup_rng(struct kref *kref)
--
2.13.5
[PATCH] Allow hwrng to initialize crng.
Some systems, for example embedded systems, do not generate
enough entropy on boot through interrupts, and boot may be blocked for
several minutes waiting for a call to getrandom to complete.
Currently, random data is read from a hwrng when it is registered,
and is loaded into primary_crng. This data is treated in the same
way as data that is device-specific but otherwise unchanging, and
so primary_crng cannot become initialized with the data from the
hwrng.
This change causes the data initially read from the hwrng to be
treated the same as subsequent data that is read from the hwrng if
it's quality score is non-zero.
The implications of this are:
The data read from hwrng can cause primary_crng to become
initialized, therefore avoiding problems of getrandom blocking
on boot.
Calls to getrandom (with GRND_RANDOM) may be using entropy
exclusively (or in practise, almost exclusively) from the hwrng.
Regarding the latter point; this behavior is the same as if a
user specified a quality score of 1 (bit of entropy per 1024 bits)
so hopefully this is not too scary a change to make.
This change is the result of the discussion here:
https://patchwork.kernel.org/patch/10453893/
Signed-off-by: Louis Collard
---
drivers/char/hw_random/core.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aaf9e5afaad4..47f358aa0c3d 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#define RNG_MODULE_NAME"hw_random"
@@ -64,13 +65,17 @@ 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());
+ /* Read enough to initialize crng. */
+ size_t size = 2*CHACHA20_KEY_SIZE;
mutex_lock(_mutex);
bytes_read = rng_get_data(rng, rng_buffer, size, 1);
mutex_unlock(_mutex);
if (bytes_read > 0)
- add_device_randomness(rng_buffer, bytes_read);
+ /* Allow crng to become initialized, but do not add
+* entropy to the pool.
+*/
+ add_hwgenerator_randomness(rng_buffer, bytes_read, 0);
}
static inline void cleanup_rng(struct kref *kref)
--
2.13.5
Re: [PATCH] tpm: Add module parameter for hwrng quality.
On Fri, Jun 29, 2018 at 9:03 PM, David R. Bild wrote: > On Wed, Jun 27, 2018 at 1:11 AM, Louis Collard > wrote: >> >> On some systems we have seen large delays in boot time, due to >> blocking on a call to getrandom() before the entropy pool has been >> initialized. On these systems the usual sources of entropy are not >> sufficient to initialize the pool in any kind of reasonable time - >> delays of minutes have been observed; the most common workaround is to >> mash the keyboard for a bit ;) >> >> Setting a non-zero quality score causes the hwrng to be used as a >> source of entropy for the pool, the pool is therefore initialized >> early during boot, and no delay is observed. >> > > We have the same issue on our embedded devices and thus carry patches > in our tree that set the quality. This would be a welcome change. Glad to hear this! > > As a point of clarification (and correct me if I'm wrong), the TPM is > always ready used to seed the rng. It just doesn't update the entropy > pool estimate. Good point. > > So, perhaps the default value for the TPM hwrng quality should be > non-zero (in addition to the module param that lets users override > it)? That makes sense to me, however I can imagine that some users would prefer to not have the TPM enabled as an ongoing source of entropy by default. Following on from your previous point - perhaps we can just make a small change to how the initial seeding is done: maybe we can replace the call to crng_slow_load (via add_early_randomness and add_device_randomness) with a call (indirectly) to crng_fast_load. (We might also need to increase the amount of data read at this point.) This would update crng_init_cnt and crng_init, and calls to getrandom [without GRND_RANDOM] would not block. This obviously doesn't solve the issue if there are blocking calls on boot that are querying random rather than urandom; I don't believe that would be a problem for our use case though. Thoughts? Thanks, Louis
Re: [PATCH] tpm: Add module parameter for hwrng quality.
On Fri, Jun 29, 2018 at 9:03 PM, David R. Bild wrote: > On Wed, Jun 27, 2018 at 1:11 AM, Louis Collard > wrote: >> >> On some systems we have seen large delays in boot time, due to >> blocking on a call to getrandom() before the entropy pool has been >> initialized. On these systems the usual sources of entropy are not >> sufficient to initialize the pool in any kind of reasonable time - >> delays of minutes have been observed; the most common workaround is to >> mash the keyboard for a bit ;) >> >> Setting a non-zero quality score causes the hwrng to be used as a >> source of entropy for the pool, the pool is therefore initialized >> early during boot, and no delay is observed. >> > > We have the same issue on our embedded devices and thus carry patches > in our tree that set the quality. This would be a welcome change. Glad to hear this! > > As a point of clarification (and correct me if I'm wrong), the TPM is > always ready used to seed the rng. It just doesn't update the entropy > pool estimate. Good point. > > So, perhaps the default value for the TPM hwrng quality should be > non-zero (in addition to the module param that lets users override > it)? That makes sense to me, however I can imagine that some users would prefer to not have the TPM enabled as an ongoing source of entropy by default. Following on from your previous point - perhaps we can just make a small change to how the initial seeding is done: maybe we can replace the call to crng_slow_load (via add_early_randomness and add_device_randomness) with a call (indirectly) to crng_fast_load. (We might also need to increase the amount of data read at this point.) This would update crng_init_cnt and crng_init, and calls to getrandom [without GRND_RANDOM] would not block. This obviously doesn't solve the issue if there are blocking calls on boot that are querying random rather than urandom; I don't believe that would be a problem for our use case though. Thoughts? Thanks, Louis
[PATCH v2] tpm: Allow tpm_tis drivers to set hwrng quality.
Adds plumbing required for drivers based on tpm_tis to set hwrng quality.
Signed-off-by: Louis Collard
---
Changes since v1:
- based on (hopefully!) correct git tree
drivers/char/tpm/tpm_tis_core.c | 2 ++
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8b46aaa9e049..d2345d9fd7b5 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -875,6 +875,8 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
chip->acpi_dev_handle = acpi_dev_handle;
#endif
+ chip->hwrng.quality = priv->rng_quality;
+
/* Maximum timeouts */
chip->timeout_a = msecs_to_jiffies(TIS_TIMEOUT_A_MAX);
chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index f6e1dbe212a7..f48125f1e6e0 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -99,6 +99,7 @@ struct tpm_tis_data {
wait_queue_head_t int_queue;
wait_queue_head_t read_queue;
const struct tpm_tis_phy_ops *phy_ops;
+ unsigned short rng_quality;
};
struct tpm_tis_phy_ops {
--
2.13.5
[PATCH v2] tpm: Allow tpm_tis drivers to set hwrng quality.
Adds plumbing required for drivers based on tpm_tis to set hwrng quality.
Signed-off-by: Louis Collard
---
Changes since v1:
- based on (hopefully!) correct git tree
drivers/char/tpm/tpm_tis_core.c | 2 ++
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8b46aaa9e049..d2345d9fd7b5 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -875,6 +875,8 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
chip->acpi_dev_handle = acpi_dev_handle;
#endif
+ chip->hwrng.quality = priv->rng_quality;
+
/* Maximum timeouts */
chip->timeout_a = msecs_to_jiffies(TIS_TIMEOUT_A_MAX);
chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index f6e1dbe212a7..f48125f1e6e0 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -99,6 +99,7 @@ struct tpm_tis_data {
wait_queue_head_t int_queue;
wait_queue_head_t read_queue;
const struct tpm_tis_phy_ops *phy_ops;
+ unsigned short rng_quality;
};
struct tpm_tis_phy_ops {
--
2.13.5
[PATCH v2] tpm: Allow tpm_tis drivers to set hwrng quality.
Adds plumbing required for drivers based on tpm_tis to set hwrng quality.
Signed-off-by: Louis Collard
---
Changes since v1:
- based on (hopefully!) correct git tree
drivers/char/tpm/tpm_tis_core.c | 2 ++
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8b46aaa9e049..d2345d9fd7b5 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -875,6 +875,8 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
chip->acpi_dev_handle = acpi_dev_handle;
#endif
+ chip->hwrng.quality = priv->rng_quality;
+
/* Maximum timeouts */
chip->timeout_a = msecs_to_jiffies(TIS_TIMEOUT_A_MAX);
chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index f6e1dbe212a7..f48125f1e6e0 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -99,6 +99,7 @@ struct tpm_tis_data {
wait_queue_head_t int_queue;
wait_queue_head_t read_queue;
const struct tpm_tis_phy_ops *phy_ops;
+ unsigned short rng_quality;
};
struct tpm_tis_phy_ops {
--
2.13.5
[PATCH v2] tpm: Allow tpm_tis drivers to set hwrng quality.
Adds plumbing required for drivers based on tpm_tis to set hwrng quality.
Signed-off-by: Louis Collard
---
Changes since v1:
- based on (hopefully!) correct git tree
drivers/char/tpm/tpm_tis_core.c | 2 ++
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8b46aaa9e049..d2345d9fd7b5 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -875,6 +875,8 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
chip->acpi_dev_handle = acpi_dev_handle;
#endif
+ chip->hwrng.quality = priv->rng_quality;
+
/* Maximum timeouts */
chip->timeout_a = msecs_to_jiffies(TIS_TIMEOUT_A_MAX);
chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index f6e1dbe212a7..f48125f1e6e0 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -99,6 +99,7 @@ struct tpm_tis_data {
wait_queue_head_t int_queue;
wait_queue_head_t read_queue;
const struct tpm_tis_phy_ops *phy_ops;
+ unsigned short rng_quality;
};
struct tpm_tis_phy_ops {
--
2.13.5
Re: [PATCH] tpm: Add module parameter for hwrng quality.
Thanks for all the replies, let me add some background around the motivation for this change. On some systems we have seen large delays in boot time, due to blocking on a call to getrandom() before the entropy pool has been initialized. On these systems the usual sources of entropy are not sufficient to initialize the pool in any kind of reasonable time - delays of minutes have been observed; the most common workaround is to mash the keyboard for a bit ;) Setting a non-zero quality score causes the hwrng to be used as a source of entropy for the pool, the pool is therefore initialized early during boot, and no delay is observed. I don't believe that the quality score is used anywhere else, so I don't think setting it should impact anything other than how the entropy pool is populated. It's my understanding that to be useful in the above situation, the parameter needs to be set on the kernel command line, so I'm not sure if a sysfs file would work. On Fri, Jun 22, 2018 at 12:21 AM, Jarkko Sakkinen wrote: > On Mon, Jun 18, 2018 at 01:33:06PM -0600, Jason Gunthorpe wrote: >> > > +module_param(override_rng_quality, short, 0644); >> > >> > Should this be 600 i.e. not to leak this information? >> >> There is a real push these days against adding module parameters, and >> apparently, IMA can't function with TPM as a module. >> >> Are you sure this shouldn't be done in some other way? > > Maybe a sysfs file would be a better choice for this? > > /Jarkko
Re: [PATCH] tpm: Add module parameter for hwrng quality.
Thanks for all the replies, let me add some background around the motivation for this change. On some systems we have seen large delays in boot time, due to blocking on a call to getrandom() before the entropy pool has been initialized. On these systems the usual sources of entropy are not sufficient to initialize the pool in any kind of reasonable time - delays of minutes have been observed; the most common workaround is to mash the keyboard for a bit ;) Setting a non-zero quality score causes the hwrng to be used as a source of entropy for the pool, the pool is therefore initialized early during boot, and no delay is observed. I don't believe that the quality score is used anywhere else, so I don't think setting it should impact anything other than how the entropy pool is populated. It's my understanding that to be useful in the above situation, the parameter needs to be set on the kernel command line, so I'm not sure if a sysfs file would work. On Fri, Jun 22, 2018 at 12:21 AM, Jarkko Sakkinen wrote: > On Mon, Jun 18, 2018 at 01:33:06PM -0600, Jason Gunthorpe wrote: >> > > +module_param(override_rng_quality, short, 0644); >> > >> > Should this be 600 i.e. not to leak this information? >> >> There is a real push these days against adding module parameters, and >> apparently, IMA can't function with TPM as a module. >> >> Are you sure this shouldn't be done in some other way? > > Maybe a sysfs file would be a better choice for this? > > /Jarkko
Re: [PATCH] tpm: Allow tpm_tis drivers to set hwrng quality.
Hi sorry I'm not sure I understand; I based the change on linux-next master, let me know if I should be using something else. The change is not dependent on https://patchwork.kernel.org/patch/10453893/ On Wed, Jun 27, 2018 at 1:28 PM, Louis Collard wrote: > Hi sorry I'm not sure I understand; I based the change on linux-next master, > let me know if I should be using something else. The change is not dependent > on https://patchwork.kernel.org/patch/10453893/ > > On Tue, Jun 19, 2018 at 2:09 AM, Jarkko Sakkinen > wrote: >> >> On Fri, Jun 08, 2018 at 03:11:46PM +0800, Louis Collard wrote: >> > Adds plumbing required for drivers based on tpm_tis to set hwrng >> > quality. >> > >> > Signed-off-by: Louis Collard >> >> NAK because this not connected to the current GIT tree (should probably >> make a patch set?). >> >> /Jarkko > >
Re: [PATCH] tpm: Allow tpm_tis drivers to set hwrng quality.
Hi sorry I'm not sure I understand; I based the change on linux-next master, let me know if I should be using something else. The change is not dependent on https://patchwork.kernel.org/patch/10453893/ On Wed, Jun 27, 2018 at 1:28 PM, Louis Collard wrote: > Hi sorry I'm not sure I understand; I based the change on linux-next master, > let me know if I should be using something else. The change is not dependent > on https://patchwork.kernel.org/patch/10453893/ > > On Tue, Jun 19, 2018 at 2:09 AM, Jarkko Sakkinen > wrote: >> >> On Fri, Jun 08, 2018 at 03:11:46PM +0800, Louis Collard wrote: >> > Adds plumbing required for drivers based on tpm_tis to set hwrng >> > quality. >> > >> > Signed-off-by: Louis Collard >> >> NAK because this not connected to the current GIT tree (should probably >> make a patch set?). >> >> /Jarkko > >
[PATCH] tpm: Allow tpm_tis drivers to set hwrng quality.
Adds plumbing required for drivers based on tpm_tis to set hwrng quality.
Signed-off-by: Louis Collard
---
drivers/char/tpm/tpm_tis_core.c | 2 ++
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8b46aaa9e049..d2345d9fd7b5 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -875,6 +875,8 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
chip->acpi_dev_handle = acpi_dev_handle;
#endif
+ chip->hwrng.quality = priv->rng_quality;
+
/* Maximum timeouts */
chip->timeout_a = msecs_to_jiffies(TIS_TIMEOUT_A_MAX);
chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index f6e1dbe212a7..f48125f1e6e0 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -99,6 +99,7 @@ struct tpm_tis_data {
wait_queue_head_t int_queue;
wait_queue_head_t read_queue;
const struct tpm_tis_phy_ops *phy_ops;
+ unsigned short rng_quality;
};
struct tpm_tis_phy_ops {
--
2.13.5
[PATCH] tpm: Allow tpm_tis drivers to set hwrng quality.
Adds plumbing required for drivers based on tpm_tis to set hwrng quality.
Signed-off-by: Louis Collard
---
drivers/char/tpm/tpm_tis_core.c | 2 ++
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8b46aaa9e049..d2345d9fd7b5 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -875,6 +875,8 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
chip->acpi_dev_handle = acpi_dev_handle;
#endif
+ chip->hwrng.quality = priv->rng_quality;
+
/* Maximum timeouts */
chip->timeout_a = msecs_to_jiffies(TIS_TIMEOUT_A_MAX);
chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index f6e1dbe212a7..f48125f1e6e0 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -99,6 +99,7 @@ struct tpm_tis_data {
wait_queue_head_t int_queue;
wait_queue_head_t read_queue;
const struct tpm_tis_phy_ops *phy_ops;
+ unsigned short rng_quality;
};
struct tpm_tis_phy_ops {
--
2.13.5
[PATCH] tpm: Add module parameter for hwrng quality.
It is now possible for drivers to easily specify a hwrng quality, however
most do not currently do this, and in cases where they do, it may be
desirable to override the driver-specified value with a user-specified
one. This patch adds a parameter to set or override the hwrng quality.
Signed-off-by: Louis Collard
---
drivers/char/tpm/tpm-chip.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0a62c19937b6..4def49cfc634 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -33,6 +33,11 @@
DEFINE_IDR(dev_nums_idr);
static DEFINE_MUTEX(idr_lock);
+static short override_rng_quality = -1;
+module_param(override_rng_quality, short, 0644);
+MODULE_PARM_DESC(override_rng_quality,
+"tpm-rng quality (overrides values provided by drivers)");
+
struct class *tpm_class;
struct class *tpmrm_class;
dev_t tpm_devt;
@@ -409,6 +414,13 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
"tpm-rng-%d", chip->dev_num);
chip->hwrng.name = chip->hwrng_name;
chip->hwrng.read = tpm_hwrng_read;
+ if (override_rng_quality > -1) {
+ dev_info(>dev,
+"Overriding hwrng quality for %s: driver default=%d,
override=%d",
+chip->hwrng.name, chip->hwrng.quality,
+override_rng_quality);
+ chip->hwrng.quality = override_rng_quality;
+ }
return hwrng_register(>hwrng);
}
--
2.13.5
[PATCH] tpm: Add module parameter for hwrng quality.
It is now possible for drivers to easily specify a hwrng quality, however
most do not currently do this, and in cases where they do, it may be
desirable to override the driver-specified value with a user-specified
one. This patch adds a parameter to set or override the hwrng quality.
Signed-off-by: Louis Collard
---
drivers/char/tpm/tpm-chip.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0a62c19937b6..4def49cfc634 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -33,6 +33,11 @@
DEFINE_IDR(dev_nums_idr);
static DEFINE_MUTEX(idr_lock);
+static short override_rng_quality = -1;
+module_param(override_rng_quality, short, 0644);
+MODULE_PARM_DESC(override_rng_quality,
+"tpm-rng quality (overrides values provided by drivers)");
+
struct class *tpm_class;
struct class *tpmrm_class;
dev_t tpm_devt;
@@ -409,6 +414,13 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
"tpm-rng-%d", chip->dev_num);
chip->hwrng.name = chip->hwrng_name;
chip->hwrng.read = tpm_hwrng_read;
+ if (override_rng_quality > -1) {
+ dev_info(>dev,
+"Overriding hwrng quality for %s: driver default=%d,
override=%d",
+chip->hwrng.name, chip->hwrng.quality,
+override_rng_quality);
+ chip->hwrng.quality = override_rng_quality;
+ }
return hwrng_register(>hwrng);
}
--
2.13.5
