diff mbox series

tpm: Fix TPM 1.2 Shutdown sequence to prevent future TPM operations

Message ID 20190610220118.5530-1-dianders@chromium.org (mailing list archive)
State New, archived
Headers show
Series tpm: Fix TPM 1.2 Shutdown sequence to prevent future TPM operations | expand

Commit Message

Doug Anderson June 10, 2019, 10:01 p.m. UTC 
From: Vadim Sukhomlinov <sukhomlinov@google.com>

TPM 2.0 Shutdown involve sending TPM2_Shutdown to TPM chip and disabling
future TPM operations. TPM 1.2 behavior was different, future TPM
operations weren't disabled, causing rare issues. This patch ensures
that future TPM operations are disabled.

Signed-off-by: Vadim Sukhomlinov <sukhomlinov@google.com>
[dianders: resolved merge conflicts with mainline]
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/char/tpm/tpm-chip.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Jarkko Sakkinen June 12, 2019, 7:16 p.m. UTC | #1 
On Mon, Jun 10, 2019 at 03:01:18PM -0700, Douglas Anderson wrote:
> From: Vadim Sukhomlinov <sukhomlinov@google.com>
> 
> TPM 2.0 Shutdown involve sending TPM2_Shutdown to TPM chip and disabling
> future TPM operations. TPM 1.2 behavior was different, future TPM
> operations weren't disabled, causing rare issues. This patch ensures
> that future TPM operations are disabled.
> 
> Signed-off-by: Vadim Sukhomlinov <sukhomlinov@google.com>
> [dianders: resolved merge conflicts with mainline]
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Nice catch. Thank you.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko
Jarkko Sakkinen June 13, 2019, 1:58 p.m. UTC | #2 
On Wed, Jun 12, 2019 at 10:16:18PM +0300, Jarkko Sakkinen wrote:
> On Mon, Jun 10, 2019 at 03:01:18PM -0700, Douglas Anderson wrote:
> > From: Vadim Sukhomlinov <sukhomlinov@google.com>
> > 
> > TPM 2.0 Shutdown involve sending TPM2_Shutdown to TPM chip and disabling
> > future TPM operations. TPM 1.2 behavior was different, future TPM
> > operations weren't disabled, causing rare issues. This patch ensures
> > that future TPM operations are disabled.
> > 
> > Signed-off-by: Vadim Sukhomlinov <sukhomlinov@google.com>
> > [dianders: resolved merge conflicts with mainline]
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> 
> Nice catch. Thank you.
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Applied to my master branch. I also added a fixes tag.

Can you check that it looks legit to you?

/Jarkko
Doug Anderson June 13, 2019, 3:20 p.m. UTC | #3 
Hi,

On Thu, Jun 13, 2019 at 6:59 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Wed, Jun 12, 2019 at 10:16:18PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Jun 10, 2019 at 03:01:18PM -0700, Douglas Anderson wrote:
> > > From: Vadim Sukhomlinov <sukhomlinov@google.com>
> > >
> > > TPM 2.0 Shutdown involve sending TPM2_Shutdown to TPM chip and disabling
> > > future TPM operations. TPM 1.2 behavior was different, future TPM
> > > operations weren't disabled, causing rare issues. This patch ensures
> > > that future TPM operations are disabled.
> > >
> > > Signed-off-by: Vadim Sukhomlinov <sukhomlinov@google.com>
> > > [dianders: resolved merge conflicts with mainline]
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >
> > Nice catch. Thank you.
> >
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> Applied to my master branch. I also added a fixes tag.
>
> Can you check that it looks legit to you?

Found the patch in your tree at
<http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/41f15a4f02092d531fb34b42a06e9a1603a7df27>.
I'm decidedly a non-expert here, mostly just wrangling a patch that
someone else came up with.  :-)  ...but let's see...

I think you're asking if the "Fixes" looks sane.  I guess it depends
on what you're trying to accomplish.  Certainly what you've tagged in
"Fixes" marks the point where it would be easiest to backport this fix
to.  ...but I think the problem is much older than that patch.

As I understand it, this problem has existed for much longer.  I
believe that ${SUBJECT} patch evolved from an investigation that Luigi
Semenzato did back in 2013 when we got back some Chromebooks whose
TPMs claimed that they had been "attacked".  Said another way, I
believe it is an evolution of the patch <https://crrev.com/c/57988>
("CHROMIUM: workaround for Infineon TPM broken defensive timeout").

...so technically someone ought to want this on all old kernels.
Maybe keep the "Cc: stable" but remove the "Fixes"?


-Doug
Jarkko Sakkinen June 14, 2019, 3:15 p.m. UTC | #4 
On Thu, Jun 13, 2019 at 08:20:41AM -0700, Doug Anderson wrote:
> Found the patch in your tree at
> <http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/41f15a4f02092d531fb34b42a06e9a1603a7df27>.
> I'm decidedly a non-expert here, mostly just wrangling a patch that
> someone else came up with.  :-)  ...but let's see...
> 
> I think you're asking if the "Fixes" looks sane.  I guess it depends
> on what you're trying to accomplish.  Certainly what you've tagged in
> "Fixes" marks the point where it would be easiest to backport this fix
> to.  ...but I think the problem is much older than that patch.
> 
> As I understand it, this problem has existed for much longer.  I
> believe that ${SUBJECT} patch evolved from an investigation that Luigi
> Semenzato did back in 2013 when we got back some Chromebooks whose
> TPMs claimed that they had been "attacked".  Said another way, I
> believe it is an evolution of the patch <https://crrev.com/c/57988>
> ("CHROMIUM: workaround for Infineon TPM broken defensive timeout").
> 
> ...so technically someone ought to want this on all old kernels.
> Maybe keep the "Cc: stable" but remove the "Fixes"?

I guess that is what we have to do then.

/Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8804c9e916fd..f566fa8bf704 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -294,15 +294,15 @@  static int tpm_class_shutdown(struct device *dev)
 {
 	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
 
+	down_write(&chip->ops_sem);
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		down_write(&chip->ops_sem);
 		if (!tpm_chip_start(chip)) {
 			tpm2_shutdown(chip, TPM2_SU_CLEAR);
 			tpm_chip_stop(chip);
 		}
-		chip->ops = NULL;
-		up_write(&chip->ops_sem);
 	}
+	chip->ops = NULL;
+	up_write(&chip->ops_sem);
 
 	return 0;
 }