[PATCH] tpm: Add module parameter for hwrng quality.

[PATCH] tpm: Add module parameter for hwrng quality.

[Louis Collard]

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);
+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);
 }
 
-- 
2.13.5

Re: [PATCH] tpm: Add module parameter for hwrng quality.

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: Add module parameter for hwrng quality.

[Jason Gunthorpe]

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

Re: [PATCH] tpm: Add module parameter for hwrng quality.

[Jarkko Sakkinen]

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.

[Louis Collard]

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

Re: [PATCH] tpm: Add module parameter for hwrng quality.

[David R. Bild]

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

Re: [PATCH] tpm: Add module parameter for hwrng quality.

[Louis Collard]

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

Re: [PATCH] tpm: Add module parameter for hwrng quality.

[David R. Bild]

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