Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1558805 > unrolled thread

[PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

Back to article view | Back to linux.kernel

Contents

  [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown Andrey Pronin <apronin@chromium.org> - 2017-01-14 01:20 +0100
    Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on  shutdown Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-14 01:40 +0100
      Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on  shutdown Andrey Pronin <apronin@chromium.org> - 2017-01-14 01:50 +0100
        Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on  shutdown Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-16 10:40 +0100
        Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on  shutdown Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-16 17:20 +0100

#1558805 — [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

Resetting TPM while processing a command may lead to issues
on the next boot. Ensure that we don't have any ongoing
commands, and that no further commands can be sent to the chip
by unregistering the device in the shutdown handler.
tpm_chip_unregister() waits for the completion of an ongoing
command, if any, and then clears out chip->ops and unregisters
sysfs entities.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 drivers/char/tpm/tpm_i2c_infineon.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index 62ee44e57ddc..0c829fe26561 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -689,14 +689,18 @@ static int tpm_tis_i2c_probe(struct i2c_client *client,
 	return rc;
 }
 
-static int tpm_tis_i2c_remove(struct i2c_client *client)
+static void tpm_tis_i2c_shutdown(struct i2c_client *client)
 {
 	struct tpm_chip *chip = tpm_dev.chip;
 
 	tpm_chip_unregister(chip);
 	release_locality(chip, tpm_dev.locality, 1);
 	tpm_dev.client = NULL;
+}
 
+static int tpm_tis_i2c_remove(struct i2c_client *client)
+{
+	tpm_tis_i2c_shutdown(client);
 	return 0;
 }
 
@@ -704,6 +708,7 @@ static struct i2c_driver tpm_tis_i2c_driver = {
 	.id_table = tpm_tis_i2c_table,
 	.probe = tpm_tis_i2c_probe,
 	.remove = tpm_tis_i2c_remove,
+	.shutdown = tpm_tis_i2c_shutdown,
 	.driver = {
 		   .name = "tpm_i2c_infineon",
 		   .pm = &tpm_tis_i2c_ops,
-- 
2.11.0.483.g087da7b7c-goog

[toc] | [next] | [standalone]

#1558808 — Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> Resetting TPM while processing a command may lead to issues
> on the next boot. Ensure that we don't have any ongoing
> commands, and that no further commands can be sent to the chip
> by unregistering the device in the shutdown handler.
> tpm_chip_unregister() waits for the completion of an ongoing
> command, if any, and then clears out chip->ops and unregisters
> sysfs entities.

Unregistering in a shutdown handler seems very strange, it also waits
for userspace things, so I wonder if it could be problematic?

Maybe just use

   down_write(&chip->ops_sem);
   chip->ops = NULL;
   up_write(&chip->ops_sem);

In the shutdown handler?

Jason

[toc] | [prev] | [next] | [standalone]

#1558809 — Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> > Resetting TPM while processing a command may lead to issues
> > on the next boot. Ensure that we don't have any ongoing
> > commands, and that no further commands can be sent to the chip
> > by unregistering the device in the shutdown handler.
> > tpm_chip_unregister() waits for the completion of an ongoing
> > command, if any, and then clears out chip->ops and unregisters
> > sysfs entities.
> 
> Unregistering in a shutdown handler seems very strange, it also waits
> for userspace things, so I wonder if it could be problematic?
> 
> Maybe just use
> 
>    down_write(&chip->ops_sem);
>    chip->ops = NULL;
>    up_write(&chip->ops_sem);
> 
> In the shutdown handler?

down_write(&chip->ops_sem) would still wait for completing the initiated
writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops().
Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be
unregistered first.
And the last thing, this driver supports TPM 1.2, but if it was a 2.0
chip, it'd also need to send TPM2_Shutdown(CLEAR) from its shutdown
handler (or get an unorderly shutdown and DA counter increment).

All these things are handled by tpm_chip_unregister(). I thought about
creating a tpm_chip_shutdown routine that could be called from shutdown
handlers of the drivers that need it (and I'd do it for every driver,
especially in 2.0 case). But decided that it's better to reuse the
existing tpm_chip_unregister() that already does what's needed.

> 
> Jason

[toc] | [prev] | [next] | [standalone]

#1559553 — Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote:
> On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote:
> > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> > > Resetting TPM while processing a command may lead to issues
> > > on the next boot. Ensure that we don't have any ongoing
> > > commands, and that no further commands can be sent to the chip
> > > by unregistering the device in the shutdown handler.
> > > tpm_chip_unregister() waits for the completion of an ongoing
> > > command, if any, and then clears out chip->ops and unregisters
> > > sysfs entities.
> > 
> > Unregistering in a shutdown handler seems very strange, it also waits
> > for userspace things, so I wonder if it could be problematic?
> > 
> > Maybe just use
> > 
> >    down_write(&chip->ops_sem);
> >    chip->ops = NULL;
> >    up_write(&chip->ops_sem);
> > 
> > In the shutdown handler?
> 
> down_write(&chip->ops_sem) would still wait for completing the initiated
> writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops().
> Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be
> unregistered first.

Why don't you fix the tpm-sysfs issue but rather misusing
tpm_chip_unregister?

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1559869 — Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote:
> On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote:
> > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> > > Resetting TPM while processing a command may lead to issues
> > > on the next boot. Ensure that we don't have any ongoing
> > > commands, and that no further commands can be sent to the chip
> > > by unregistering the device in the shutdown handler.
> > > tpm_chip_unregister() waits for the completion of an ongoing
> > > command, if any, and then clears out chip->ops and unregisters
> > > sysfs entities.
> > 
> > Unregistering in a shutdown handler seems very strange, it also waits
> > for userspace things, so I wonder if it could be problematic?
> > 
> > Maybe just use
> > 
> >    down_write(&chip->ops_sem);
> >    chip->ops = NULL;
> >    up_write(&chip->ops_sem);
> > 
> > In the shutdown handler?
> 
> down_write(&chip->ops_sem) would still wait for completing the initiated
> writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops().

Yes, but that is a timeout limited wait. unregister waits for sysfs
files to be closed which is potentially unbounded.

> Yes, but it doesn't wait for sysfs
> Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be
> unregistered first.

Yes, sorry, I should have mentioned that.. Maybe that is too much to
fix..

> And the last thing, this driver supports TPM 1.2, but if it was a 2.0
> chip, it'd also need to send TPM2_Shutdown(CLEAR) from its shutdown
> handler (or get an unorderly shutdown and DA counter increment).

I'm confused - doesn't your system reset the TPM when it reboots?
Isn't that required so the firmware starts with known PCRs? Doesn't
reset trump unorderly shutdown?

In any event that seems like an all-chips problem not a chip specific
bug fix?

> All these things are handled by tpm_chip_unregister(). I thought about
> creating a tpm_chip_shutdown routine that could be called from shutdown
> handlers of the drivers that need it (and I'd do it for every driver,
> especially in 2.0 case). But decided that it's better to reuse the
> existing tpm_chip_unregister() that already does what's needed.

If for some reason we need this for every driver then this is probably
a better approach - but that seems very, very strange to me.

Jason

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web