| Message ID | 20170516000852.28400-1-joshz@google.com |
|---|---|
| State | New |
| Headers | show |
Are there any other changes I should make to this patch, or is it good to go once the patch it depends on is in? Thanks! Josh On Mon, May 15, 2017 at 5:08 PM, Josh Zimmerman <joshz@google.com> wrote: > If a TPM2 loses power without a TPM2_Shutdown command being issued (a > "disorderly reboot"), it may lose some state that has yet to be > persisted to NVRam, and will increment the DA counter (eventually, this > will cause the TPM to lock the user out.) > > NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses sysfs, > and sysfs relies on implicit locking on chip->ops, it is not safe to > allow this code to run in TPM1, or to add sysfs support to TPM2, until > that locking is made explicit. > > This patch is dependent on '[PATCH] Add "shutdown" to "struct class".' > http://marc.info/?l=linux-kernel&m=149463235025420&w=2 > > Signed-off-by: Josh Zimmerman <joshz@google.com> > > ---- > v2: > - Properly split changes between this and another commit > - Use proper locking primitive. > - Fix commenting style > v3: > - Re-fix commenting style > --- > drivers/char/tpm/tpm-chip.c | 20 ++++++++++++++++++++ > drivers/char/tpm/tpm-sysfs.c | 3 +++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 9dec9f551b83..272a42e77574 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -142,6 +142,25 @@ static void tpm_devs_release(struct device *dev) > put_device(&chip->dev); > } > > +static void tpm_shutdown(struct device *dev) > +{ > + struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev); > + /* TPM 2.0 requires that the TPM2_Shutdown() command be issued prior to > + * loss of power. If it is not, the DA counter will be incremented and, > + * eventually, the user will be locked out of their TPM. > + * XXX: This codepath relies on the fact that sysfs is not enabled for > + * TPM2: sysfs uses an implicit lock on chip->ops, so this use could > + * race if TPM2 has sysfs support enabled before TPM sysfs's implicit > + * locking is fixed. > + */ > + if (chip->flags & TPM_CHIP_FLAG_TPM2) { > + down_write(&chip->ops_sem); > + tpm2_shutdown(chip, TPM_SU_CLEAR); > + chip->ops = NULL; > + up_write(&chip->ops_sem); > + } > +} > + > /** > * tpm_chip_alloc() - allocate a new struct tpm_chip instance > * @pdev: device to which the chip is associated > @@ -181,6 +200,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, > device_initialize(&chip->devs); > > chip->dev.class = tpm_class; > + chip->dev.class.shutdown = tpm_shutdown; > chip->dev.release = tpm_dev_release; > chip->dev.parent = pdev; > chip->dev.groups = chip->groups; > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c > index 55405dbe43fa..5e5ff7eb6f7e 100644 > --- a/drivers/char/tpm/tpm-sysfs.c > +++ b/drivers/char/tpm/tpm-sysfs.c > @@ -294,6 +294,9 @@ static const struct attribute_group tpm_dev_group = { > > void tpm_sysfs_add_device(struct tpm_chip *chip) > { > + /* XXX: Before this restriction is removed, tpm_sysfs must be updated > + * to explicitly lock chip->ops. > + */ > if (chip->flags & TPM_CHIP_FLAG_TPM2) > return; > > -- > 2.13.0.303.g4ebf302169-goog > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
This is a bit hard to track down, but I think I've found a relevant bit of the PTP spec (section 3.8): https://www.trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2-0-v43-150126.pdf > The TPM2_Shutdown > (STATE) command allows a Static OS to indicate to the TPM that the platform may > enter a low power state where the TPM will be required to enter into the D3 power > state. The use of the term "may" is significant in that there is no requirement for the > platform to actually enter the low power state after sending the TPM2_Shutdown > (STATE) command. The software may, in fact, send subsequent commands after > sending the TPM2_Shutdown (STATE) commands. The TPM2_Shutdown (STATE) > command simply tells the TPM to save the required volatile contents because power to > the TPM may be removed at any time. The TPM is responsible for tracking its internal > state so that, if a command that alters the TPM’s saved state is sent to the TPM after a > TPM2_Shutdown (STATE) command, the TPM voids the saved internal state so a > subsequent TPM2_Startup(STATE) will fail. In this case, it is the responsibility of > platform software to send a subsequent TPM2_Shutdown (STATE) command to > preserve the new internal state of the TPM. From Part 1 (architecture), https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.38.pdf: > A TPM implementation may invalidate a preserved context on any command except TPM2_GetCapability(). I haven't found anything in the spec that explicitly addresses being able to immediately follow a TPM2_Shutdown(STATE) with a TPM2_Shutdown(CLEAR), but my understanding based on the first quote is that a TPM2_Shutdown(CLEAR) may clear the previous shutdown state, and as long as the Shutdown(CLEAR) is the final command, the shutdown will be orderly. Josh On Thu, May 18, 2017 at 2:13 PM, Stefan Berger <stefanb@us.ibm.com> wrote: > Does it work when doing suspend (to RAM) and tpm_pm_suspend sent a > tpm2_shutdown(chip, TPM2_SU_STATE) presumably before that? > > Stefan > > > ----- Original message ----- > From: Josh Zimmerman <joshz@google.com> > To: Peter Huewe <peterhuewe@gmx.de>, Marcel Selhorst <tpmdd@selhorst.net>, > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com>, tpmdd-devel@lists.sourceforge.net > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Subject: Re: [tpmdd-devel] [PATCH v3] tpm: Issue a TPM2_Shutdown for TPM2 > devices. > Date: Thu, May 18, 2017 11:22 AM > > Are there any other changes I should make to this patch, or is it good > to go once the patch it depends on is in? > > Thanks! > Josh > > > On Mon, May 15, 2017 at 5:08 PM, Josh Zimmerman <joshz@google.com> wrote: >> If a TPM2 loses power without a TPM2_Shutdown command being issued (a >> "disorderly reboot"), it may lose some state that has yet to be >> persisted to NVRam, and will increment the DA counter (eventually, this >> will cause the TPM to lock the user out.) >> >> NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses sysfs, >> and sysfs relies on implicit locking on chip->ops, it is not safe to >> allow this code to run in TPM1, or to add sysfs support to TPM2, until >> that locking is made explicit. >> >> This patch is dependent on '[PATCH] Add "shutdown" to "struct class".' >> http://marc.info/?l=linux-kernel&m=149463235025420&w=2 >> >> Signed-off-by: Josh Zimmerman <joshz@google.com> >> >> ---- >> v2: >> - Properly split changes between this and another commit >> - Use proper locking primitive. >> - Fix commenting style >> v3: >> - Re-fix commenting style >> --- >> drivers/char/tpm/tpm-chip.c | 20 ++++++++++++++++++++ >> drivers/char/tpm/tpm-sysfs.c | 3 +++ >> 2 files changed, 23 insertions(+) >> >> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c >> index 9dec9f551b83..272a42e77574 100644 >> --- a/drivers/char/tpm/tpm-chip.c >> +++ b/drivers/char/tpm/tpm-chip.c >> @@ -142,6 +142,25 @@ static void tpm_devs_release(struct device *dev) >> put_device(&chip->dev); >> } >> >> +static void tpm_shutdown(struct device *dev) >> +{ >> + struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev); >> + /* TPM 2.0 requires that the TPM2_Shutdown() command be issued >> prior to >> + * loss of power. If it is not, the DA counter will be incremented >> and, >> + * eventually, the user will be locked out of their TPM. >> + * XXX: This codepath relies on the fact that sysfs is not enabled >> for >> + * TPM2: sysfs uses an implicit lock on chip->ops, so this use >> could >> + * race if TPM2 has sysfs support enabled before TPM sysfs's >> implicit >> + * locking is fixed. >> + */ >> + if (chip->flags & TPM_CHIP_FLAG_TPM2) { >> + down_write(&chip->ops_sem); >> + tpm2_shutdown(chip, TPM_SU_CLEAR); >> + chip->ops = NULL; >> + up_write(&chip->ops_sem); >> + } >> +} >> + >> /** >> * tpm_chip_alloc() - allocate a new struct tpm_chip instance >> * @pdev: device to which the chip is associated >> @@ -181,6 +200,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, >> device_initialize(&chip->devs); >> >> chip->dev.class = tpm_class; >> + chip->dev.class.shutdown = tpm_shutdown; >> chip->dev.release = tpm_dev_release; >> chip->dev.parent = pdev; >> chip->dev.groups = chip->groups; >> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c >> index 55405dbe43fa..5e5ff7eb6f7e 100644 >> --- a/drivers/char/tpm/tpm-sysfs.c >> +++ b/drivers/char/tpm/tpm-sysfs.c >> @@ -294,6 +294,9 @@ static const struct attribute_group tpm_dev_group = { >> >> void tpm_sysfs_add_device(struct tpm_chip *chip) >> { >> + /* XXX: Before this restriction is removed, tpm_sysfs must be >> updated >> + * to explicitly lock chip->ops. >> + */ >> if (chip->flags & TPM_CHIP_FLAG_TPM2) >> return; >> >> -- >> 2.13.0.303.g4ebf302169-goog >> > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel > > > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, May 18, 2017 at 03:24:40PM -0700, Josh Zimmerman wrote: > This is a bit hard to track down, but I think I've found a relevant > bit of the PTP spec (section 3.8): > https://www.trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2-0-v43-150126.pdf > > > The TPM2_Shutdown > > (STATE) command allows a Static OS to indicate to the TPM that the platform may > > enter a low power state where the TPM will be required to enter into the D3 power > > state. The use of the term "may" is significant in that there is no requirement for the > > platform to actually enter the low power state after sending the TPM2_Shutdown > > (STATE) command. The software may, in fact, send subsequent commands after > > sending the TPM2_Shutdown (STATE) commands. The TPM2_Shutdown (STATE) > > command simply tells the TPM to save the required volatile contents because power to > > the TPM may be removed at any time. The TPM is responsible for tracking its internal > > state so that, if a command that alters the TPM’s saved state is sent to the TPM after a > > TPM2_Shutdown (STATE) command, the TPM voids the saved internal state so a > > subsequent TPM2_Startup(STATE) will fail. In this case, it is the responsibility of > > platform software to send a subsequent TPM2_Shutdown (STATE) command to > > preserve the new internal state of the TPM. > > From Part 1 (architecture), > https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.38.pdf: > > > A TPM implementation may invalidate a preserved context on any command except TPM2_GetCapability(). > > I haven't found anything in the spec that explicitly addresses being > able to immediately follow a TPM2_Shutdown(STATE) with a > TPM2_Shutdown(CLEAR), but my understanding based on the first quote is > that a TPM2_Shutdown(CLEAR) may clear the previous shutdown state, and > as long as the Shutdown(CLEAR) is the final command, the shutdown will > be orderly. > Josh > Per spec, there's nothing wrong with TPM2_Shutdown(CLEAR) after TPM2_Shutdown(STATE). However, if that happened, that'd break the suspend-resume process, which presumably sends TPM2_Startup(STATE) on resume, which would obviously fail after TPM2_Shutdown(CLEAR). But more importantly: Freeze/suspend doesn't trigger .shutdown for the device. As proposed in https://patchwork.kernel.org/patch/9727693/, .shutdown only happens when the kernel shuts down or restarts - kernel_restart, kernel_halt, or kernel_power_offr. Not for PM transitions to S3 or S0ix. > > > On Thu, May 18, 2017 at 2:13 PM, Stefan Berger <stefanb@us.ibm.com> wrote: > > Does it work when doing suspend (to RAM) and tpm_pm_suspend sent a > > tpm2_shutdown(chip, TPM2_SU_STATE) presumably before that? > > > > Stefan > > > > > > ----- Original message ----- > > From: Josh Zimmerman <joshz@google.com> > > To: Peter Huewe <peterhuewe@gmx.de>, Marcel Selhorst <tpmdd@selhorst.net>, > > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>, Jason Gunthorpe > > <jgunthorpe@obsidianresearch.com>, tpmdd-devel@lists.sourceforge.net > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Subject: Re: [tpmdd-devel] [PATCH v3] tpm: Issue a TPM2_Shutdown for TPM2 > > devices. > > Date: Thu, May 18, 2017 11:22 AM > > > > Are there any other changes I should make to this patch, or is it good > > to go once the patch it depends on is in? > > > > Thanks! > > Josh > > > > > > On Mon, May 15, 2017 at 5:08 PM, Josh Zimmerman <joshz@google.com> wrote: > >> If a TPM2 loses power without a TPM2_Shutdown command being issued (a > >> "disorderly reboot"), it may lose some state that has yet to be > >> persisted to NVRam, and will increment the DA counter (eventually, this > >> will cause the TPM to lock the user out.) > >> > >> NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses sysfs, > >> and sysfs relies on implicit locking on chip->ops, it is not safe to > >> allow this code to run in TPM1, or to add sysfs support to TPM2, until > >> that locking is made explicit. > >> > >> This patch is dependent on '[PATCH] Add "shutdown" to "struct class".' > >> http://marc.info/?l=linux-kernel&m=149463235025420&w=2 > >> > >> Signed-off-by: Josh Zimmerman <joshz@google.com> > >> > >> ---- > >> v2: > >> - Properly split changes between this and another commit > >> - Use proper locking primitive. > >> - Fix commenting style > >> v3: > >> - Re-fix commenting style > >> --- > >> drivers/char/tpm/tpm-chip.c | 20 ++++++++++++++++++++ > >> drivers/char/tpm/tpm-sysfs.c | 3 +++ > >> 2 files changed, 23 insertions(+) > >> > >> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > >> index 9dec9f551b83..272a42e77574 100644 > >> --- a/drivers/char/tpm/tpm-chip.c > >> +++ b/drivers/char/tpm/tpm-chip.c > >> @@ -142,6 +142,25 @@ static void tpm_devs_release(struct device *dev) > >> put_device(&chip->dev); > >> } > >> > >> +static void tpm_shutdown(struct device *dev) > >> +{ > >> + struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev); > >> + /* TPM 2.0 requires that the TPM2_Shutdown() command be issued > >> prior to > >> + * loss of power. If it is not, the DA counter will be incremented > >> and, > >> + * eventually, the user will be locked out of their TPM. > >> + * XXX: This codepath relies on the fact that sysfs is not enabled > >> for > >> + * TPM2: sysfs uses an implicit lock on chip->ops, so this use > >> could > >> + * race if TPM2 has sysfs support enabled before TPM sysfs's > >> implicit > >> + * locking is fixed. > >> + */ > >> + if (chip->flags & TPM_CHIP_FLAG_TPM2) { > >> + down_write(&chip->ops_sem); > >> + tpm2_shutdown(chip, TPM_SU_CLEAR); > >> + chip->ops = NULL; > >> + up_write(&chip->ops_sem); > >> + } > >> +} > >> + > >> /** > >> * tpm_chip_alloc() - allocate a new struct tpm_chip instance > >> * @pdev: device to which the chip is associated > >> @@ -181,6 +200,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, > >> device_initialize(&chip->devs); > >> > >> chip->dev.class = tpm_class; > >> + chip->dev.class.shutdown = tpm_shutdown; > >> chip->dev.release = tpm_dev_release; > >> chip->dev.parent = pdev; > >> chip->dev.groups = chip->groups; > >> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c > >> index 55405dbe43fa..5e5ff7eb6f7e 100644 > >> --- a/drivers/char/tpm/tpm-sysfs.c > >> +++ b/drivers/char/tpm/tpm-sysfs.c > >> @@ -294,6 +294,9 @@ static const struct attribute_group tpm_dev_group = { > >> > >> void tpm_sysfs_add_device(struct tpm_chip *chip) > >> { > >> + /* XXX: Before this restriction is removed, tpm_sysfs must be > >> updated > >> + * to explicitly lock chip->ops. > >> + */ > >> if (chip->flags & TPM_CHIP_FLAG_TPM2) > >> return; > >> > >> -- > >> 2.13.0.303.g4ebf302169-goog > >> > > > > ------------------------------------------------------------------------------ > > Check out the vibrant tech community on one of the world's most > > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > > _______________________________________________ > > tpmdd-devel mailing list > > tpmdd-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel > > > > > > > > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Yes. Can you ping me once it is in? I can merge this after that. /Jarkko On Thu, May 18, 2017 at 08:21:32AM -0700, Josh Zimmerman wrote: > Are there any other changes I should make to this patch, or is it good > to go once the patch it depends on is in? > > Thanks! > Josh > > > On Mon, May 15, 2017 at 5:08 PM, Josh Zimmerman <joshz@google.com> wrote: > > If a TPM2 loses power without a TPM2_Shutdown command being issued (a > > "disorderly reboot"), it may lose some state that has yet to be > > persisted to NVRam, and will increment the DA counter (eventually, this > > will cause the TPM to lock the user out.) > > > > NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses sysfs, > > and sysfs relies on implicit locking on chip->ops, it is not safe to > > allow this code to run in TPM1, or to add sysfs support to TPM2, until > > that locking is made explicit. > > > > This patch is dependent on '[PATCH] Add "shutdown" to "struct class".' > > http://marc.info/?l=linux-kernel&m=149463235025420&w=2 > > > > Signed-off-by: Josh Zimmerman <joshz@google.com> > > > > ---- > > v2: > > - Properly split changes between this and another commit > > - Use proper locking primitive. > > - Fix commenting style > > v3: > > - Re-fix commenting style > > --- > > drivers/char/tpm/tpm-chip.c | 20 ++++++++++++++++++++ > > drivers/char/tpm/tpm-sysfs.c | 3 +++ > > 2 files changed, 23 insertions(+) > > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > > index 9dec9f551b83..272a42e77574 100644 > > --- a/drivers/char/tpm/tpm-chip.c > > +++ b/drivers/char/tpm/tpm-chip.c > > @@ -142,6 +142,25 @@ static void tpm_devs_release(struct device *dev) > > put_device(&chip->dev); > > } > > > > +static void tpm_shutdown(struct device *dev) > > +{ > > + struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev); > > + /* TPM 2.0 requires that the TPM2_Shutdown() command be issued prior to > > + * loss of power. If it is not, the DA counter will be incremented and, > > + * eventually, the user will be locked out of their TPM. > > + * XXX: This codepath relies on the fact that sysfs is not enabled for > > + * TPM2: sysfs uses an implicit lock on chip->ops, so this use could > > + * race if TPM2 has sysfs support enabled before TPM sysfs's implicit > > + * locking is fixed. > > + */ > > + if (chip->flags & TPM_CHIP_FLAG_TPM2) { > > + down_write(&chip->ops_sem); > > + tpm2_shutdown(chip, TPM_SU_CLEAR); > > + chip->ops = NULL; > > + up_write(&chip->ops_sem); > > + } > > +} > > + > > /** > > * tpm_chip_alloc() - allocate a new struct tpm_chip instance > > * @pdev: device to which the chip is associated > > @@ -181,6 +200,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, > > device_initialize(&chip->devs); > > > > chip->dev.class = tpm_class; > > + chip->dev.class.shutdown = tpm_shutdown; > > chip->dev.release = tpm_dev_release; > > chip->dev.parent = pdev; > > chip->dev.groups = chip->groups; > > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c > > index 55405dbe43fa..5e5ff7eb6f7e 100644 > > --- a/drivers/char/tpm/tpm-sysfs.c > > +++ b/drivers/char/tpm/tpm-sysfs.c > > @@ -294,6 +294,9 @@ static const struct attribute_group tpm_dev_group = { > > > > void tpm_sysfs_add_device(struct tpm_chip *chip) > > { > > + /* XXX: Before this restriction is removed, tpm_sysfs must be updated > > + * to explicitly lock chip->ops. > > + */ > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > > return; > > > > -- > > 2.13.0.303.g4ebf302169-goog > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Yes, will do. Can you mark a Reviewed-by on this version of the patch as well? You marked v2 already, but this is probably the version that should be submitted. Josh On Sat, May 20, 2017 at 6:20 AM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > Yes. Can you ping me once it is in? I can merge this after that. > > /Jarkko > > On Thu, May 18, 2017 at 08:21:32AM -0700, Josh Zimmerman wrote: >> Are there any other changes I should make to this patch, or is it good >> to go once the patch it depends on is in? >> >> Thanks! >> Josh >> >> >> On Mon, May 15, 2017 at 5:08 PM, Josh Zimmerman <joshz@google.com> wrote: >> > If a TPM2 loses power without a TPM2_Shutdown command being issued (a >> > "disorderly reboot"), it may lose some state that has yet to be >> > persisted to NVRam, and will increment the DA counter (eventually, this >> > will cause the TPM to lock the user out.) >> > >> > NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses sysfs, >> > and sysfs relies on implicit locking on chip->ops, it is not safe to >> > allow this code to run in TPM1, or to add sysfs support to TPM2, until >> > that locking is made explicit. >> > >> > This patch is dependent on '[PATCH] Add "shutdown" to "struct class".' >> > http://marc.info/?l=linux-kernel&m=149463235025420&w=2 >> > >> > Signed-off-by: Josh Zimmerman <joshz@google.com> >> > >> > ---- >> > v2: >> > - Properly split changes between this and another commit >> > - Use proper locking primitive. >> > - Fix commenting style >> > v3: >> > - Re-fix commenting style >> > --- >> > drivers/char/tpm/tpm-chip.c | 20 ++++++++++++++++++++ >> > drivers/char/tpm/tpm-sysfs.c | 3 +++ >> > 2 files changed, 23 insertions(+) >> > >> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c >> > index 9dec9f551b83..272a42e77574 100644 >> > --- a/drivers/char/tpm/tpm-chip.c >> > +++ b/drivers/char/tpm/tpm-chip.c >> > @@ -142,6 +142,25 @@ static void tpm_devs_release(struct device *dev) >> > put_device(&chip->dev); >> > } >> > >> > +static void tpm_shutdown(struct device *dev) >> > +{ >> > + struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev); >> > + /* TPM 2.0 requires that the TPM2_Shutdown() command be issued prior to >> > + * loss of power. If it is not, the DA counter will be incremented and, >> > + * eventually, the user will be locked out of their TPM. >> > + * XXX: This codepath relies on the fact that sysfs is not enabled for >> > + * TPM2: sysfs uses an implicit lock on chip->ops, so this use could >> > + * race if TPM2 has sysfs support enabled before TPM sysfs's implicit >> > + * locking is fixed. >> > + */ >> > + if (chip->flags & TPM_CHIP_FLAG_TPM2) { >> > + down_write(&chip->ops_sem); >> > + tpm2_shutdown(chip, TPM_SU_CLEAR); >> > + chip->ops = NULL; >> > + up_write(&chip->ops_sem); >> > + } >> > +} >> > + >> > /** >> > * tpm_chip_alloc() - allocate a new struct tpm_chip instance >> > * @pdev: device to which the chip is associated >> > @@ -181,6 +200,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, >> > device_initialize(&chip->devs); >> > >> > chip->dev.class = tpm_class; >> > + chip->dev.class.shutdown = tpm_shutdown; >> > chip->dev.release = tpm_dev_release; >> > chip->dev.parent = pdev; >> > chip->dev.groups = chip->groups; >> > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c >> > index 55405dbe43fa..5e5ff7eb6f7e 100644 >> > --- a/drivers/char/tpm/tpm-sysfs.c >> > +++ b/drivers/char/tpm/tpm-sysfs.c >> > @@ -294,6 +294,9 @@ static const struct attribute_group tpm_dev_group = { >> > >> > void tpm_sysfs_add_device(struct tpm_chip *chip) >> > { >> > + /* XXX: Before this restriction is removed, tpm_sysfs must be updated >> > + * to explicitly lock chip->ops. >> > + */ >> > if (chip->flags & TPM_CHIP_FLAG_TPM2) >> > return; >> > >> > -- >> > 2.13.0.303.g4ebf302169-goog >> > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Tue, May 23, 2017 at 08:34:20AM -0700, Josh Zimmerman wrote: > Yes, will do. Can you mark a Reviewed-by on this version of the patch > as well? You marked v2 already, but this is probably the version that > should be submitted. > Josh Reviewed-by: Jarko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko > > > On Sat, May 20, 2017 at 6:20 AM, Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com> wrote: > > Yes. Can you ping me once it is in? I can merge this after that. > > > > /Jarkko > > > > On Thu, May 18, 2017 at 08:21:32AM -0700, Josh Zimmerman wrote: > >> Are there any other changes I should make to this patch, or is it good > >> to go once the patch it depends on is in? > >> > >> Thanks! > >> Josh > >> > >> > >> On Mon, May 15, 2017 at 5:08 PM, Josh Zimmerman <joshz@google.com> wrote: > >> > If a TPM2 loses power without a TPM2_Shutdown command being issued (a > >> > "disorderly reboot"), it may lose some state that has yet to be > >> > persisted to NVRam, and will increment the DA counter (eventually, this > >> > will cause the TPM to lock the user out.) > >> > > >> > NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses sysfs, > >> > and sysfs relies on implicit locking on chip->ops, it is not safe to > >> > allow this code to run in TPM1, or to add sysfs support to TPM2, until > >> > that locking is made explicit. > >> > > >> > This patch is dependent on '[PATCH] Add "shutdown" to "struct class".' > >> > http://marc.info/?l=linux-kernel&m=149463235025420&w=2 > >> > > >> > Signed-off-by: Josh Zimmerman <joshz@google.com> > >> > > >> > ---- > >> > v2: > >> > - Properly split changes between this and another commit > >> > - Use proper locking primitive. > >> > - Fix commenting style > >> > v3: > >> > - Re-fix commenting style > >> > --- > >> > drivers/char/tpm/tpm-chip.c | 20 ++++++++++++++++++++ > >> > drivers/char/tpm/tpm-sysfs.c | 3 +++ > >> > 2 files changed, 23 insertions(+) > >> > > >> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > >> > index 9dec9f551b83..272a42e77574 100644 > >> > --- a/drivers/char/tpm/tpm-chip.c > >> > +++ b/drivers/char/tpm/tpm-chip.c > >> > @@ -142,6 +142,25 @@ static void tpm_devs_release(struct device *dev) > >> > put_device(&chip->dev); > >> > } > >> > > >> > +static void tpm_shutdown(struct device *dev) > >> > +{ > >> > + struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev); > >> > + /* TPM 2.0 requires that the TPM2_Shutdown() command be issued prior to > >> > + * loss of power. If it is not, the DA counter will be incremented and, > >> > + * eventually, the user will be locked out of their TPM. > >> > + * XXX: This codepath relies on the fact that sysfs is not enabled for > >> > + * TPM2: sysfs uses an implicit lock on chip->ops, so this use could > >> > + * race if TPM2 has sysfs support enabled before TPM sysfs's implicit > >> > + * locking is fixed. > >> > + */ > >> > + if (chip->flags & TPM_CHIP_FLAG_TPM2) { > >> > + down_write(&chip->ops_sem); > >> > + tpm2_shutdown(chip, TPM_SU_CLEAR); > >> > + chip->ops = NULL; > >> > + up_write(&chip->ops_sem); > >> > + } > >> > +} > >> > + > >> > /** > >> > * tpm_chip_alloc() - allocate a new struct tpm_chip instance > >> > * @pdev: device to which the chip is associated > >> > @@ -181,6 +200,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, > >> > device_initialize(&chip->devs); > >> > > >> > chip->dev.class = tpm_class; > >> > + chip->dev.class.shutdown = tpm_shutdown; > >> > chip->dev.release = tpm_dev_release; > >> > chip->dev.parent = pdev; > >> > chip->dev.groups = chip->groups; > >> > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c > >> > index 55405dbe43fa..5e5ff7eb6f7e 100644 > >> > --- a/drivers/char/tpm/tpm-sysfs.c > >> > +++ b/drivers/char/tpm/tpm-sysfs.c > >> > @@ -294,6 +294,9 @@ static const struct attribute_group tpm_dev_group = { > >> > > >> > void tpm_sysfs_add_device(struct tpm_chip *chip) > >> > { > >> > + /* XXX: Before this restriction is removed, tpm_sysfs must be updated > >> > + * to explicitly lock chip->ops. > >> > + */ > >> > if (chip->flags & TPM_CHIP_FLAG_TPM2) > >> > return; > >> > > >> > -- > >> > 2.13.0.303.g4ebf302169-goog > >> > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 9dec9f551b83..272a42e77574 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -142,6 +142,25 @@ static void tpm_devs_release(struct device *dev) put_device(&chip->dev); } +static void tpm_shutdown(struct device *dev) +{ + struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev); + /* TPM 2.0 requires that the TPM2_Shutdown() command be issued prior to + * loss of power. If it is not, the DA counter will be incremented and, + * eventually, the user will be locked out of their TPM. + * XXX: This codepath relies on the fact that sysfs is not enabled for + * TPM2: sysfs uses an implicit lock on chip->ops, so this use could + * race if TPM2 has sysfs support enabled before TPM sysfs's implicit + * locking is fixed. + */ + if (chip->flags & TPM_CHIP_FLAG_TPM2) { + down_write(&chip->ops_sem); + tpm2_shutdown(chip, TPM_SU_CLEAR); + chip->ops = NULL; + up_write(&chip->ops_sem); + } +} + /** * tpm_chip_alloc() - allocate a new struct tpm_chip instance * @pdev: device to which the chip is associated @@ -181,6 +200,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, device_initialize(&chip->devs); chip->dev.class = tpm_class; + chip->dev.class.shutdown = tpm_shutdown; chip->dev.release = tpm_dev_release; chip->dev.parent = pdev; chip->dev.groups = chip->groups; diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c index 55405dbe43fa..5e5ff7eb6f7e 100644 --- a/drivers/char/tpm/tpm-sysfs.c +++ b/drivers/char/tpm/tpm-sysfs.c @@ -294,6 +294,9 @@ static const struct attribute_group tpm_dev_group = { void tpm_sysfs_add_device(struct tpm_chip *chip) { + /* XXX: Before this restriction is removed, tpm_sysfs must be updated + * to explicitly lock chip->ops. + */ if (chip->flags & TPM_CHIP_FLAG_TPM2) return;
If a TPM2 loses power without a TPM2_Shutdown command being issued (a "disorderly reboot"), it may lose some state that has yet to be persisted to NVRam, and will increment the DA counter (eventually, this will cause the TPM to lock the user out.) NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses sysfs, and sysfs relies on implicit locking on chip->ops, it is not safe to allow this code to run in TPM1, or to add sysfs support to TPM2, until that locking is made explicit. This patch is dependent on '[PATCH] Add "shutdown" to "struct class".' http://marc.info/?l=linux-kernel&m=149463235025420&w=2 Signed-off-by: Josh Zimmerman <joshz@google.com> ---- v2: - Properly split changes between this and another commit - Use proper locking primitive. - Fix commenting style v3: - Re-fix commenting style --- drivers/char/tpm/tpm-chip.c | 20 ++++++++++++++++++++ drivers/char/tpm/tpm-sysfs.c | 3 +++ 2 files changed, 23 insertions(+)