| Message ID | 20170616172531.28464-1-joshz@google.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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;
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(+)