diff mbox

[v3] tpm: Issue a TPM2_Shutdown for TPM2 devices.

Message ID 20170516000852.28400-1-joshz@google.com (mailing list archive)
State New, archived
Headers show

Commit Message

Josh Zimmerman May 16, 2017, 12:08 a.m. UTC
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(+)

Comments

Josh Zimmerman May 18, 2017, 3:21 p.m. UTC | #1
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
Stefan Berger May 18, 2017, 9:13 p.m. UTC | #2
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Josh Zimmerman May 18, 2017, 10:24 p.m. UTC | #3
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
Andrey Pronin May 18, 2017, 10:29 p.m. UTC | #4
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
Jarkko Sakkinen May 20, 2017, 1:20 p.m. UTC | #5
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
Josh Zimmerman May 23, 2017, 3:34 p.m. UTC | #6
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
Jarkko Sakkinen May 24, 2017, 5:28 p.m. UTC | #7
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 mbox

Patch

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;