| Message ID | CANc+2y5YDcJYcwyCUtdCcvW_07HhjTzSrShyWFkfCjXYTsi0QQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On Fri, May 12, 2017 at 3:06 AM, PrasannaKumar Muralidharan wrote: > On 12 May 2017 at 12:22, PrasannaKumar Muralidharan wrote: > > On 12 May 2017 at 12:11, Mike Frysinger wrote: > >> On Fri, May 12, 2017 at 2:15 AM, PrasannaKumar Muralidharan wrote: > >>> On 12 May 2017 at 09:47, Mike Frysinger wrote: > >>> > From: Mike Frysinger <vapier@chromium.org> > >>> > > >>> > If you build in hwrng & tpm-rng, but boot on a system that doesn't > >>> > have a tpm (like via KVM), hwrng will spam the logs every 10 seconds > >>> > with the line: > >>> > hwrng: no data available > >>> > > >>> > This isn't terribly useful, so squelch the error in the ENODEV case. > >>> > For all other errors, we still warn, and include the actual error. > > > > If the boot system does not have a tpm I think registering tpm-rng is > > not useful. On tpm-rng load instead of registering with hwrng a check > > can be made whether the system supports tpm. Is this possible? > > Completely untested patch below. Will something like this work? > > --- a/drivers/char/hw_random/tpm-rng.c > +++ b/drivers/char/hw_random/tpm-rng.c > @@ -35,7 +35,13 @@ static int tpm_rng_read(struct hwrng *rng, void > *data, size_t max, bool wait) > > static int __init rng_init(void) > { > - return hwrng_register(&tpm_rng); > + struct tpm_chip *tpm_rng_chip = tpm_chip_find_get(TPM_ANY_NUM); > + if (tpm_chip) { > + tpm_put_ops(tpm_rng_chip); > + return hwrng_register(&tpm_rng); > + } > + > + return -ENODEV; > } > module_init(rng_init); keep in mind that TPMs are often on slow buses like I2C, so i suspect rng_init runs before those have been initialized. so this patch would break them. it would also break if the tpm drivers are modules that don't get loaded until later, but tpm-rng is built in. or tpm-rng is loaded first. -mike
On 12 May 2017 at 13:17, Mike Frysinger <vapier@chromium.org> wrote: >> Completely untested patch below. Will something like this work? >> >> --- a/drivers/char/hw_random/tpm-rng.c >> +++ b/drivers/char/hw_random/tpm-rng.c >> @@ -35,7 +35,13 @@ static int tpm_rng_read(struct hwrng *rng, void >> *data, size_t max, bool wait) >> >> static int __init rng_init(void) >> { >> - return hwrng_register(&tpm_rng); >> + struct tpm_chip *tpm_rng_chip = tpm_chip_find_get(TPM_ANY_NUM); >> + if (tpm_chip) { >> + tpm_put_ops(tpm_rng_chip); >> + return hwrng_register(&tpm_rng); >> + } >> + >> + return -ENODEV; >> } >> module_init(rng_init); > > keep in mind that TPMs are often on slow buses like I2C, so i suspect > rng_init runs before those have been initialized. so this patch would > break them. > > it would also break if the tpm drivers are modules that don't get > loaded until later, but tpm-rng is built in. or tpm-rng is loaded > first. Hmm. I am not aware of the tpm hardware or driver behavior. Based on your explanation I see that this patch is not useful. It looks like it is possible to detect the presence of tpm device and call hwrng_register once the corresponding driver is loaded. I leave it to Herbert to decide whether to accept this patch in current form or not. Regardless of whether this patch gets accepted or not I can work on a better fix if you can provide instructions on how to setup and use tpm. But that will be only after a couple of months. Regards, PrasannaKumar
On Fri, May 12, 2017 at 01:49:52PM +0530, PrasannaKumar Muralidharan wrote: > > I leave it to Herbert to decide whether to accept this patch in > current form or not. I think the correct fix would be for the TPM subsystem to signal that it is ready and then register the tpm-rng device. Thanks,
On Sun, Jun 18, 2017 at 9:12 PM, Herbert Xu wrote: > On Fri, May 12, 2017 at 01:49:52PM +0530, PrasannaKumar Muralidharan wrote: > > I leave it to Herbert to decide whether to accept this patch in > > current form or not. > > I think the correct fix would be for the TPM subsystem to signal that > it is ready and then register the tpm-rng device. the TPM subsystem is ready. it's like saying "the USB subsystem should signal when it's ready". the TPM subsystem provides a bus (of sorts) and clients (like tpm-rng) can use whatever backend happens to be available. in order to make tpm-rng react in the way you're implying, the TPM subsystem would need to add a notification chain for transitions from none<->some devices, then tpm-rng could subscribe to that, and during those transition points, it would call hwrng_register/hwrng_unregister to make itself visible accordingly to the hwrng subsystem. maybe someone on the TPM side would be interested in writing all that logic, but it sounds excessive for this minor usage. the current tpm-rng driver is *extremely* simple -- it's 3 funcs, each of which are 1 line. -mike
On Sun, Jun 18, 2017 at 10:00:17PM -0700, Mike Frysinger wrote: > > in order to make tpm-rng react in the way you're implying, the TPM > subsystem would need to add a notification chain for transitions from > none<->some devices, then tpm-rng could subscribe to that, and during > those transition points, it would call hwrng_register/hwrng_unregister > to make itself visible accordingly to the hwrng subsystem. maybe > someone on the TPM side would be interested in writing all that logic, > but it sounds excessive for this minor usage. the current tpm-rng > driver is *extremely* simple -- it's 3 funcs, each of which are 1 > line. It's simple and it's broken, as far as the way it hooks into the hwrng is concerned. Cheers,
diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c index d6d4482..f78f8ca 100644 --- a/drivers/char/hw_random/tpm-rng.c +++ b/drivers/char/hw_random/tpm-rng.c @@ -35,7 +35,13 @@ static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait) static int __init rng_init(void) { - return hwrng_register(&tpm_rng); + struct tpm_chip *tpm_rng_chip = tpm_chip_find_get(TPM_ANY_NUM); + if (tpm_chip) { + tpm_put_ops(tpm_rng_chip); + return hwrng_register(&tpm_rng); + } + + return -ENODEV; } module_init(rng_init);