| Message ID | 20180608065438.110109-1-louiscollard@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 08, 2018 at 02:54:38PM +0800, Louis Collard wrote: > 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 <louiscollard@chromium.org> > --- > 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); Should this be 600 i.e. not to leak this information? /Jarkko
On Mon, Jun 18, 2018 at 09:07:12PM +0300, Jarkko Sakkinen wrote: > On Fri, Jun 08, 2018 at 02:54:38PM +0800, Louis Collard wrote: > > 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 <louiscollard@chromium.org> > > 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 > > +++ 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); > > 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? Jason
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
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 <jarkko.sakkinen@linux.intel.com> 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
On Wed, Jun 27, 2018 at 1:11 AM, Louis Collard <louiscollard@chromium.org> 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. 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. 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)? As it stands, it encourages programmers to not use methods like getrandom() or not check the entropy pool estimate before reading from dev/urandom. They know the rng was supposedly seeded by the TPM --- the entropy estimate just wasn't updated --- so the easiest "fix" is to just not check the entropy pool estimate. I think the default config on a machine with a TPM should be that the rng is seeded by the TPM and that the entropy pool estimate is updated to reflect that. Then programs that are written properly (i.e., use getrandom()) will work correctly with no further effort from the user. Best, David
On Fri, Jun 29, 2018 at 9:03 PM, David R. Bild <david.bild@xaptum.com> wrote: > On Wed, Jun 27, 2018 at 1:11 AM, Louis Collard > <louiscollard@chromium.org> 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
On Wed, Jul 4, 2018 at 2:54 AM, Louis Collard <louiscollard@chromium.org> wrote: > On Fri, Jun 29, 2018 at 9:03 PM, David R. Bild <david.bild@xaptum.com> wrote: >> 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. Fair enough. > 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. Interesting. add_hwgenereator_randomness() will call crng_fast_load(), regardless of entropy estimate/quality, if crng_init is 0. So initializing crng_init from the hwrng, regardless of quality, is already the intent. But hw_random only calls add_hwgenerator_randomness() if current_quality > 0, via the hwrng_fillfn() kthread. All that to say, I agree. add_early_randomness() should (indirectly) call crng_fast_load(), like add_hwgenerator_randomness() does. > 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. > It wouldn't be a problem for our use case either. Best, David
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(&chip->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(&chip->hwrng); }
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 <louiscollard@chromium.org> --- drivers/char/tpm/tpm-chip.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)