[PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Josh Zimmerman via tpmdd-devel]

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. After the DA
counter gets sufficiently large, the TPM will 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.

Signed-off-by: Josh Zimmerman <joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

----
v2:
  - Properly split changes between this and another commit
  - Use proper locking primitive.
  - Fix commenting style
v3:
  - Re-fix commenting style
v4:
  - Update description and tags (Reviewed-by, Cc).
v5:
  - Update documentation.
v6:
  - Call device and/or bus shutdown from tpm_shutdown.
---
 drivers/char/tpm/tpm-chip.c  | 31 +++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm-sysfs.c |  3 +++
 2 files changed, 34 insertions(+)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 9dec9f551b83..62691d266f22 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -142,6 +142,36 @@ static void tpm_devs_release(struct device *dev)
 	put_device(&chip->dev);
 }
 
+/**
+ * tpm_shutdown() - prepare the TPM device for loss of power.
+ * @dev: device to which the chip is associated.
+ *
+ * Issues a TPM2_Shutdown command prior to loss of power, as required by the
+ * TPM 2.0 spec.
+ *
+ * XXX: This codepath relies on the fact that sysfs is not enabled for
+ * TPM2: sysfs uses an implicit lock on chip->ops, so this could race if TPM2
+ * has sysfs support enabled before TPM sysfs's implicit locking is fixed.
+ */
+static void tpm_shutdown(struct device *dev)
+{
+	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
+
+	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);
+	}
+	// Allow bus- and device-specific code to run. Note: since chip->ops
+	// is NULL, more-specific shutdown code will not be able to issue TPM
+	// commands.
+	if (dev->bus->shutdown)
+		dev->bus->shutdown(dev);
+	else if (dev->driver && dev->driver->shutdown)
+		dev->driver->shutdown(dev);
+}
+
 /**
  * tpm_chip_alloc() - allocate a new struct tpm_chip instance
  * @pdev: device to which the chip is associated
@@ -181,6 +211,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..59bd0b7b5959 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: If you wish to remove this restriction, you must first update
+	 * tpm_sysfs to explicitly lock chip->ops.
+	 */
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return;
 
-- 
2.13.1.518.g3df882009-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH v6 2/2] Add "shutdown" to "struct class".

[Josh Zimmerman via tpmdd-devel]

The TPM class has some common shutdown code that must be executed for
all drivers. This adds some needed functionality for that.

Signed-off-by: Josh Zimmerman <joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

-----
v2: Add Signed-off-by.
v3: Remove logically separate change.
v4: Add "acked-by" and "cc".
v5: Execute only one of the class/bus/driver's shutdown functions.
---
 drivers/base/core.c    | 6 +++++-
 include/linux/device.h | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6bb60fb6a30b..9f426954681e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2667,7 +2667,11 @@ void device_shutdown(void)
 		pm_runtime_get_noresume(dev);
 		pm_runtime_barrier(dev);
 
-		if (dev->bus && dev->bus->shutdown) {
+		if (dev->class && dev->class->shutdown) {
+			if (initcall_debug)
+				dev_info(dev, "shutdown\n");
+			dev->class->shutdown(dev);
+		} else if (dev->bus && dev->bus->shutdown) {
 			if (initcall_debug)
 				dev_info(dev, "shutdown\n");
 			dev->bus->shutdown(dev);
diff --git a/include/linux/device.h b/include/linux/device.h
index 9ef518af5515..f240baac2001 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -378,6 +378,7 @@ int subsys_virtual_register(struct bus_type *subsys,
  * @suspend:	Used to put the device to sleep mode, usually to a low power
  *		state.
  * @resume:	Used to bring the device from the sleep mode.
+ * @shutdown:	Called at shut-down time to quiesce the device.
  * @ns_type:	Callbacks so sysfs can detemine namespaces.
  * @namespace:	Namespace of the device belongs to this class.
  * @pm:		The default device power management operations of this class.
@@ -407,6 +408,7 @@ struct class {
 
 	int (*suspend)(struct device *dev, pm_message_t state);
 	int (*resume)(struct device *dev);
+	int (*shutdown)(struct device *dev);
 
 	const struct kobj_ns_type_operations *ns_type;
 	const void *(*namespace)(struct device *dev);
-- 
2.13.1.518.g3df882009-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jarkko Sakkinen]

On Fri, 2017-06-16 at 10:25 -0700, Josh Zimmerman 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. After the DA
> counter gets sufficiently large, the TPM will 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.
> 
> Signed-off-by: Josh Zimmerman <joshz@google.com>
> Cc: stable@vger.kernel.org
> 
> ----
> v2:
>   - Properly split changes between this and another commit
>   - Use proper locking primitive.
>   - Fix commenting style
> v3:
>   - Re-fix commenting style
> v4:
>   - Update description and tags (Reviewed-by, Cc).
> v5:
>   - Update documentation.
> v6:
>   - Call device and/or bus shutdown from tpm_shutdown.

The change log should be after the dashes *below* right before diff
stat. Since this is 1/2 I should be abe to assume that I could apply on
this and not apply 2/2.

Why this patch set does not have a cover letter?

Anyway, I applied these to my master and if they don't break anything
I'm including them to my 4.13 pull request. I left the commit messages
to the form that they turn out if I just git am -3 the mbox files.

Next time, do a git am sanity check if you have changelogs in your
patches.

/Jarkko

------------------------------------------------------------------------------
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

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jarkko Sakkinen]

Hold on.

On Fri, 2017-06-16 at 10:25 -0700, Josh Zimmerman 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. After the DA
> counter gets sufficiently large, the TPM will 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.
> 
> Signed-off-by: Josh Zimmerman <joshz@google.com>
> Cc: stable@vger.kernel.org
> 
> ----
> v2:
>   - Properly split changes between this and another commit
>   - Use proper locking primitive.
>   - Fix commenting style
> v3:
>   - Re-fix commenting style
> v4:
>   - Update description and tags (Reviewed-by, Cc).
> v5:
>   - Update documentation.
> v6:
>   - Call device and/or bus shutdown from tpm_shutdown.
> ---
>  drivers/char/tpm/tpm-chip.c  | 31 +++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm-sysfs.c |  3 +++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-
chip.c
> index 9dec9f551b83..62691d266f22 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -142,6 +142,36 @@ static void tpm_devs_release(struct device *dev)
>  	put_device(&chip->dev);
>  }
>  
> +/**
> + * tpm_shutdown() - prepare the TPM device for loss of power.
> + * @dev: device to which the chip is associated.
> + *
> + * Issues a TPM2_Shutdown command prior to loss of power, as
required by the
> + * TPM 2.0 spec.
> + *
> + * XXX: This codepath relies on the fact that sysfs is not enabled
for
> + * TPM2: sysfs uses an implicit lock on chip->ops, so this could
race if TPM2
> + * has sysfs support enabled before TPM sysfs's implicit locking is
fixed.
> + */
> +static void tpm_shutdown(struct device *dev)
> +{
> +	struct tpm_chip *chip = container_of(dev, struct tpm_chip,
dev);
> +
> +	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);
> +	}
> +	// Allow bus- and device-specific code to run. Note: since
chip->ops
> +	// is NULL, more-specific shutdown code will not be able to
issue TPM
> +	// commands.
> +	if (dev->bus->shutdown)
> +		dev->bus->shutdown(dev);
> +	else if (dev->driver && dev->driver->shutdown)
> +		dev->driver->shutdown(dev);

WTF is this part.

And why we have now duplicate code??

I'm dropping these patches for now from my master branch. I don't
understand what is going on...

/Jarkko

------------------------------------------------------------------------------
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

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jarkko Sakkinen]

On Mon, 2017-06-19 at 01:21 +0200, Jarkko Sakkinen wrote:
> Hold on.
> 
> On Fri, 2017-06-16 at 10:25 -0700, Josh Zimmerman 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. After the DA
> > counter gets sufficiently large, the TPM will 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.
> >  
> > Signed-off-by: Josh Zimmerman <joshz@google.com>
> > Cc: stable@vger.kernel.org
> >  
> > ----
> > v2:
> >    - Properly split changes between this and another commit
> >    - Use proper locking primitive.
> >    - Fix commenting style
> > v3:
> >    - Re-fix commenting style
> > v4:
> >    - Update description and tags (Reviewed-by, Cc).
> > v5:
> >    - Update documentation.
> > v6:
> >    - Call device and/or bus shutdown from tpm_shutdown.
> > ---
> >   drivers/char/tpm/tpm-chip.c  | 31 +++++++++++++++++++++++++++++++
> >   drivers/char/tpm/tpm-sysfs.c |  3 +++
> >   2 files changed, 34 insertions(+)
> >  
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-
> 
> chip.c
> > index 9dec9f551b83..62691d266f22 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -142,6 +142,36 @@ static void tpm_devs_release(struct device
> > *dev)
> >   	put_device(&chip->dev);
> >   }
> >   
> > +/**
> > + * tpm_shutdown() - prepare the TPM device for loss of power.
> > + * @dev: device to which the chip is associated.
> > + *
> > + * Issues a TPM2_Shutdown command prior to loss of power, as
> 
> required by the
> > + * TPM 2.0 spec.
> > + *
> > + * XXX: This codepath relies on the fact that sysfs is not enabled
> 
> for
> > + * TPM2: sysfs uses an implicit lock on chip->ops, so this could
> 
> race if TPM2
> > + * has sysfs support enabled before TPM sysfs's implicit locking
> > is
> 
> fixed.
> > + */
> > +static void tpm_shutdown(struct device *dev)
> > +{
> > +	struct tpm_chip *chip = container_of(dev, struct tpm_chip,
> 
> dev);
> > +
> > +	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);
> > +	}
> > +	// Allow bus- and device-specific code to run. Note: since
> 
> chip->ops
> > +	// is NULL, more-specific shutdown code will not be able
> > to
> 
> issue TPM
> > +	// commands.
> > +	if (dev->bus->shutdown)
> > +		dev->bus->shutdown(dev);
> > +	else if (dev->driver && dev->driver->shutdown)
> > +		dev->driver->shutdown(dev);
> 
> WTF is this part.
> 
> And why we have now duplicate code??
> 
> I'm dropping these patches for now from my master branch. I don't
> understand what is going on...
> 
> /Jarkko


This series is too broken to be merged :-( I expect cover letter for
the next version...

Feels weird that you have to call framework functions like that in the
driver. You must have brilliant reason to do so and that should be very
well documented in the code. This is terrible...

/Jarkko

------------------------------------------------------------------------------
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

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jason Gunthorpe]

On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
 
> Feels weird that you have to call framework functions like that in the
> driver. You must have brilliant reason to do so and that should be very
> well documented in the code. This is terrible...

This was all discussed on the list. It the way these callbacks work,
the higher levels in the callback stack call the lower levels, this
allows each level the place the next level's callback properly, eg do
things before/after as necessary.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jarkko Sakkinen]

On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
>  
> > Feels weird that you have to call framework functions like that in the
> > driver. You must have brilliant reason to do so and that should be very
> > well documented in the code. This is terrible...
> 
> This was all discussed on the list. It the way these callbacks work,
> the higher levels in the callback stack call the lower levels, this
> allows each level the place the next level's callback properly, eg do
> things before/after as necessary.
> 
> Jason

I tried to look up for discussion from the patchwork. These had appeared
in v6. I guess I have to backtrack the discussions from my maidir
because I honestly don't understand why class shutdown would have to
call bus callback explicitly. There's nothing in the commit message
about this nor  is there any comment in the code.

This must be fairly recent development that I've missed?

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jarkko Sakkinen]

On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> >  
> > > Feels weird that you have to call framework functions like that in the
> > > driver. You must have brilliant reason to do so and that should be very
> > > well documented in the code. This is terrible...
> > 
> > This was all discussed on the list. It the way these callbacks work,
> > the higher levels in the callback stack call the lower levels, this
> > allows each level the place the next level's callback properly, eg do
> > things before/after as necessary.
> > 
> > Jason
> 
> I tried to look up for discussion from the patchwork. These had appeared
> in v6. I guess I have to backtrack the discussions from my maidir
> because I honestly don't understand why class shutdown would have to
> call bus callback explicitly. There's nothing in the commit message
> about this nor  is there any comment in the code.
> 
> This must be fairly recent development that I've missed?
> 
> /Jarkko

Found it:

"Looking at this closer, now you definately have to change the TPM
patch to call through to the other shutdown methods. We can say
current TPM drivers have no driver->shutdown, but we cannot be sure
about the bus->shutdown, so may as well call both from tpm's
class->shutdown.

I would say this should be done after the tpm2_shutdown completes as
lower level shutdowns could remove register access.

Jason"

And makes sense.

This patch is a NAK for two reasons:

1. No comment explaining this.
2. Patches are broken and they are in wrong order and cover letter is
   missing

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jarkko Sakkinen]

On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
> On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> > >  
> > > > Feels weird that you have to call framework functions like that in the
> > > > driver. You must have brilliant reason to do so and that should be very
> > > > well documented in the code. This is terrible...
> > > 
> > > This was all discussed on the list. It the way these callbacks work,
> > > the higher levels in the callback stack call the lower levels, this
> > > allows each level the place the next level's callback properly, eg do
> > > things before/after as necessary.
> > > 
> > > Jason
> > 
> > I tried to look up for discussion from the patchwork. These had appeared
> > in v6. I guess I have to backtrack the discussions from my maidir
> > because I honestly don't understand why class shutdown would have to
> > call bus callback explicitly. There's nothing in the commit message
> > about this nor  is there any comment in the code.
> > 
> > This must be fairly recent development that I've missed?
> > 
> > /Jarkko
> 
> Found it:
> 
> "Looking at this closer, now you definately have to change the TPM
> patch to call through to the other shutdown methods. We can say
> current TPM drivers have no driver->shutdown, but we cannot be sure
> about the bus->shutdown, so may as well call both from tpm's
> class->shutdown.
> 
> I would say this should be done after the tpm2_shutdown completes as
> lower level shutdowns could remove register access.
> 
> Jason"
> 
> And makes sense.
> 
> This patch is a NAK for two reasons:
> 
> 1. No comment explaining this.
> 2. Patches are broken and they are in wrong order and cover letter is
>    missing
> 
> /Jarkko

I *tried* to apply them myself after sending this in order to be helpful
but they have compilation errors:

drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
   tpm2_shutdown(chip, TPM_SU_CLEAR);
                       ^~~~~~~~~~~~
drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
  chip->dev.class.shutdown = tpm_shutdown;

/Jarkko

------------------------------------------------------------------------------
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

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Josh Zimmerman via tpmdd-devel]

Sorry about the mess. I'm a bit swamped today, but I'll work on
cleaning up the patch formatting & commit message and fix the
compilation problem later today or tomorrow. (It did build on my
checkout of the tpmdd branch... guess I didn't pull in some important
change.)
Josh


On Mon, Jun 19, 2017 at 3:38 PM, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
> On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
>> On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
>> > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
>> > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
>> > >
>> > > > Feels weird that you have to call framework functions like that in the
>> > > > driver. You must have brilliant reason to do so and that should be very
>> > > > well documented in the code. This is terrible...
>> > >
>> > > This was all discussed on the list. It the way these callbacks work,
>> > > the higher levels in the callback stack call the lower levels, this
>> > > allows each level the place the next level's callback properly, eg do
>> > > things before/after as necessary.
>> > >
>> > > Jason
>> >
>> > I tried to look up for discussion from the patchwork. These had appeared
>> > in v6. I guess I have to backtrack the discussions from my maidir
>> > because I honestly don't understand why class shutdown would have to
>> > call bus callback explicitly. There's nothing in the commit message
>> > about this nor  is there any comment in the code.
>> >
>> > This must be fairly recent development that I've missed?
>> >
>> > /Jarkko
>>
>> Found it:
>>
>> "Looking at this closer, now you definately have to change the TPM
>> patch to call through to the other shutdown methods. We can say
>> current TPM drivers have no driver->shutdown, but we cannot be sure
>> about the bus->shutdown, so may as well call both from tpm's
>> class->shutdown.
>>
>> I would say this should be done after the tpm2_shutdown completes as
>> lower level shutdowns could remove register access.
>>
>> Jason"
>>
>> And makes sense.
>>
>> This patch is a NAK for two reasons:
>>
>> 1. No comment explaining this.
>> 2. Patches are broken and they are in wrong order and cover letter is
>>    missing
>>
>> /Jarkko
>
> I *tried* to apply them myself after sending this in order to be helpful
> but they have compilation errors:
>
> drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
> drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
>    tpm2_shutdown(chip, TPM_SU_CLEAR);
>                        ^~~~~~~~~~~~
> drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
> drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
> drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
>   chip->dev.class.shutdown = tpm_shutdown;
>
> /Jarkko

------------------------------------------------------------------------------
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

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jarkko Sakkinen]

No worries. Just fix them and I will include to my next pull request!

/Jarkko

On Mon, Jun 19, 2017 at 03:44:55PM -0700, Josh Zimmerman wrote:
> Sorry about the mess. I'm a bit swamped today, but I'll work on
> cleaning up the patch formatting & commit message and fix the
> compilation problem later today or tomorrow. (It did build on my
> checkout of the tpmdd branch... guess I didn't pull in some important
> change.)
> Josh
> 
> 
> On Mon, Jun 19, 2017 at 3:38 PM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
> >> On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> >> > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> >> > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> >> > >
> >> > > > Feels weird that you have to call framework functions like that in the
> >> > > > driver. You must have brilliant reason to do so and that should be very
> >> > > > well documented in the code. This is terrible...
> >> > >
> >> > > This was all discussed on the list. It the way these callbacks work,
> >> > > the higher levels in the callback stack call the lower levels, this
> >> > > allows each level the place the next level's callback properly, eg do
> >> > > things before/after as necessary.
> >> > >
> >> > > Jason
> >> >
> >> > I tried to look up for discussion from the patchwork. These had appeared
> >> > in v6. I guess I have to backtrack the discussions from my maidir
> >> > because I honestly don't understand why class shutdown would have to
> >> > call bus callback explicitly. There's nothing in the commit message
> >> > about this nor  is there any comment in the code.
> >> >
> >> > This must be fairly recent development that I've missed?
> >> >
> >> > /Jarkko
> >>
> >> Found it:
> >>
> >> "Looking at this closer, now you definately have to change the TPM
> >> patch to call through to the other shutdown methods. We can say
> >> current TPM drivers have no driver->shutdown, but we cannot be sure
> >> about the bus->shutdown, so may as well call both from tpm's
> >> class->shutdown.
> >>
> >> I would say this should be done after the tpm2_shutdown completes as
> >> lower level shutdowns could remove register access.
> >>
> >> Jason"
> >>
> >> And makes sense.
> >>
> >> This patch is a NAK for two reasons:
> >>
> >> 1. No comment explaining this.
> >> 2. Patches are broken and they are in wrong order and cover letter is
> >>    missing
> >>
> >> /Jarkko
> >
> > I *tried* to apply them myself after sending this in order to be helpful
> > but they have compilation errors:
> >
> > drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
> > drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
> >    tpm2_shutdown(chip, TPM_SU_CLEAR);
> >                        ^~~~~~~~~~~~
> > drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
> > drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
> > drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
> >   chip->dev.class.shutdown = tpm_shutdown;
> >
> > /Jarkko

------------------------------------------------------------------------------
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

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jarkko Sakkinen]

While you still have to work on this, could you name the function as
tpm_class_shutdown() to better claarify its purpose? Otherwise, things
start to look a bit confusing. Thanks.

/Jarkko

On Tue, Jun 20, 2017 at 05:05:41PM +0200, Jarkko Sakkinen wrote:
> No worries. Just fix them and I will include to my next pull request!
> 
> /Jarkko
> 
> On Mon, Jun 19, 2017 at 03:44:55PM -0700, Josh Zimmerman wrote:
> > Sorry about the mess. I'm a bit swamped today, but I'll work on
> > cleaning up the patch formatting & commit message and fix the
> > compilation problem later today or tomorrow. (It did build on my
> > checkout of the tpmdd branch... guess I didn't pull in some important
> > change.)
> > Josh
> > 
> > 
> > On Mon, Jun 19, 2017 at 3:38 PM, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > > On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
> > >> On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> > >> > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> > >> > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> > >> > >
> > >> > > > Feels weird that you have to call framework functions like that in the
> > >> > > > driver. You must have brilliant reason to do so and that should be very
> > >> > > > well documented in the code. This is terrible...
> > >> > >
> > >> > > This was all discussed on the list. It the way these callbacks work,
> > >> > > the higher levels in the callback stack call the lower levels, this
> > >> > > allows each level the place the next level's callback properly, eg do
> > >> > > things before/after as necessary.
> > >> > >
> > >> > > Jason
> > >> >
> > >> > I tried to look up for discussion from the patchwork. These had appeared
> > >> > in v6. I guess I have to backtrack the discussions from my maidir
> > >> > because I honestly don't understand why class shutdown would have to
> > >> > call bus callback explicitly. There's nothing in the commit message
> > >> > about this nor  is there any comment in the code.
> > >> >
> > >> > This must be fairly recent development that I've missed?
> > >> >
> > >> > /Jarkko
> > >>
> > >> Found it:
> > >>
> > >> "Looking at this closer, now you definately have to change the TPM
> > >> patch to call through to the other shutdown methods. We can say
> > >> current TPM drivers have no driver->shutdown, but we cannot be sure
> > >> about the bus->shutdown, so may as well call both from tpm's
> > >> class->shutdown.
> > >>
> > >> I would say this should be done after the tpm2_shutdown completes as
> > >> lower level shutdowns could remove register access.
> > >>
> > >> Jason"
> > >>
> > >> And makes sense.
> > >>
> > >> This patch is a NAK for two reasons:
> > >>
> > >> 1. No comment explaining this.
> > >> 2. Patches are broken and they are in wrong order and cover letter is
> > >>    missing
> > >>
> > >> /Jarkko
> > >
> > > I *tried* to apply them myself after sending this in order to be helpful
> > > but they have compilation errors:
> > >
> > > drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
> > > drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
> > >    tpm2_shutdown(chip, TPM_SU_CLEAR);
> > >                        ^~~~~~~~~~~~
> > > drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
> > > drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
> > > drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
> > >   chip->dev.class.shutdown = tpm_shutdown;
> > >
> > > /Jarkko
> 
> ------------------------------------------------------------------------------
> 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

Re: [PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Jarkko Sakkinen]

On Tue, Jun 20, 2017 at 12:38:40AM +0200, Jarkko Sakkinen wrote:
> On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
> > On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> > > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> > > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> > > >  
> > > > > Feels weird that you have to call framework functions like that in the
> > > > > driver. You must have brilliant reason to do so and that should be very
> > > > > well documented in the code. This is terrible...
> > > > 
> > > > This was all discussed on the list. It the way these callbacks work,
> > > > the higher levels in the callback stack call the lower levels, this
> > > > allows each level the place the next level's callback properly, eg do
> > > > things before/after as necessary.
> > > > 
> > > > Jason
> > > 
> > > I tried to look up for discussion from the patchwork. These had appeared
> > > in v6. I guess I have to backtrack the discussions from my maidir
> > > because I honestly don't understand why class shutdown would have to
> > > call bus callback explicitly. There's nothing in the commit message
> > > about this nor  is there any comment in the code.
> > > 
> > > This must be fairly recent development that I've missed?
> > > 
> > > /Jarkko
> > 
> > Found it:
> > 
> > "Looking at this closer, now you definately have to change the TPM
> > patch to call through to the other shutdown methods. We can say
> > current TPM drivers have no driver->shutdown, but we cannot be sure
> > about the bus->shutdown, so may as well call both from tpm's
> > class->shutdown.
> > 
> > I would say this should be done after the tpm2_shutdown completes as
> > lower level shutdowns could remove register access.
> > 
> > Jason"
> > 
> > And makes sense.
> > 
> > This patch is a NAK for two reasons:
> > 
> > 1. No comment explaining this.
> > 2. Patches are broken and they are in wrong order and cover letter is
> >    missing
> > 
> > /Jarkko
> 
> I *tried* to apply them myself after sending this in order to be helpful
> but they have compilation errors:
> 
> drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
> drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
>    tpm2_shutdown(chip, TPM_SU_CLEAR);
>                        ^~~~~~~~~~~~
> drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
> drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
> drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
>   chip->dev.class.shutdown = tpm_shutdown;
> 
> /Jarkko

There's one more thing that I noticed. Before I can Cc these to the
stable these commits require the Fixes tag.

/Jarkko

------------------------------------------------------------------------------
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

[PATCH v6 1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

[Josh Zimmerman]

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. After the DA
counter gets sufficiently large, the TPM will 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.

Signed-off-by: Josh Zimmerman <joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

----
v2:
  - Properly split changes between this and another commit
  - Use proper locking primitive.
  - Fix commenting style
v3:
  - Re-fix commenting style
v4:
  - Update description and tags (Reviewed-by, Cc).
v5:
  - Update documentation.
v6:
  - Call device or bus shutdown from tpm_shutdown.
---
 drivers/char/tpm/tpm-chip.c  | 31 +++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm-sysfs.c |  3 +++
 2 files changed, 34 insertions(+)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 9dec9f551b83..62691d266f22 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -142,6 +142,36 @@ static void tpm_devs_release(struct device *dev)
        put_device(&chip->dev);
 }

+/**
+ * tpm_shutdown() - prepare the TPM device for loss of power.
+ * @dev: device to which the chip is associated.
+ *
+ * Issues a TPM2_Shutdown command prior to loss of power, as required by the
+ * TPM 2.0 spec.
+ *
+ * XXX: This codepath relies on the fact that sysfs is not enabled for
+ * TPM2: sysfs uses an implicit lock on chip->ops, so this could race if TPM2
+ * has sysfs support enabled before TPM sysfs's implicit locking is fixed.
+ */
+static void tpm_shutdown(struct device *dev)
+{
+       struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
+
+       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);
+       }
+       // Allow bus- and device-specific code to run. Note: since chip->ops
+       // is NULL, more-specific shutdown code will not be able to issue TPM
+       // commands.
+       if (dev->bus->shutdown)
+               dev->bus->shutdown(dev);
+       else if (dev->driver && dev->driver->shutdown)
+               dev->driver->shutdown(dev);
+}
+
 /**
  * tpm_chip_alloc() - allocate a new struct tpm_chip instance
  * @pdev: device to which the chip is associated
@@ -181,6 +211,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..59bd0b7b5959 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: If you wish to remove this restriction, you must first update
+        * tpm_sysfs to explicitly lock chip->ops.
+        */
        if (chip->flags & TPM_CHIP_FLAG_TPM2)
                return;

-- 
2.13.0.506.g27d5fe0cd-goog

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot