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

Commit Message

Doug Anderson July 11, 2019, 4:29 p.m. UTC

From: Vadim Sukhomlinov <[e-mail]>

commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.

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.

Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
Cc: [e-mail]
Signed-off-by: Vadim Sukhomlinov <[e-mail]>
[dianders: resolved merge conflicts with mainline]
Signed-off-by: Douglas Anderson <[e-mail]>
Reviewed-by: Jarkko Sakkinen <[e-mail]>
Signed-off-by: Jarkko Sakkinen <[e-mail]>
---
This is the backport of the patch referenced above to 4.19 as was done
in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
presumably applies to some older kernels.  NOTE that the problem
itself has existed for a long time, but continuing to backport this
exact solution to super old kernels is out of scope for me.  For those
truly interested feel free to reference the past discussion [1].

Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
chip power gating out of tpm_transmit()") and commit 719b7d81f204
("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
seem like a good idea to backport 17 patches to avoid the conflict.

[1] https://lkml.kernel.org/r/CAD=FV=[e-mail]

 drivers/char/tpm/tpm-chip.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Jason Gunthorpe July 11, 2019, 4:39 p.m. UTC | #1

On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> From: Vadim Sukhomlinov <[e-mail]>
> 
> commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> 
> 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.
> 
> Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> Cc: [e-mail]
> Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> [dianders: resolved merge conflicts with mainline]
> Signed-off-by: Douglas Anderson <[e-mail]>
> Reviewed-by: Jarkko Sakkinen <[e-mail]>
> Signed-off-by: Jarkko Sakkinen <[e-mail]>
> This is the backport of the patch referenced above to 4.19 as was done
> in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> presumably applies to some older kernels.  NOTE that the problem
> itself has existed for a long time, but continuing to backport this
> exact solution to super old kernels is out of scope for me.  For those
> truly interested feel free to reference the past discussion [1].
> 
> Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> chip power gating out of tpm_transmit()") and commit 719b7d81f204
> ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> seem like a good idea to backport 17 patches to avoid the conflict.

Careful with this, you can't backport this to any kernels that don't
have the sysfs ops locking changes or they will crash in sysfs code.

Jason

Doug Anderson July 11, 2019, 4:41 p.m. UTC | #2

Hi,

On Thu, Jul 11, 2019 at 9:39 AM Jason Gunthorpe <[e-mail]> wrote:
>
> On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > From: Vadim Sukhomlinov <[e-mail]>
> >
> > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> >
> > 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.
> >
> > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > Cc: [e-mail]
> > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > [dianders: resolved merge conflicts with mainline]
> > Signed-off-by: Douglas Anderson <[e-mail]>
> > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > This is the backport of the patch referenced above to 4.19 as was done
> > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > presumably applies to some older kernels.  NOTE that the problem
> > itself has existed for a long time, but continuing to backport this
> > exact solution to super old kernels is out of scope for me.  For those
> > truly interested feel free to reference the past discussion [1].
> >
> > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > seem like a good idea to backport 17 patches to avoid the conflict.
>
> Careful with this, you can't backport this to any kernels that don't
> have the sysfs ops locking changes or they will crash in sysfs code.

Ah, got it.  Thanks for catching!  Should we just give up on trying to
get this to stable then, or are the sysfs ops locking patches also
easy to queue up?

-Doug

Greg KH July 11, 2019, 5:04 p.m. UTC | #3

On Thu, Jul 11, 2019 at 01:39:15PM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > From: Vadim Sukhomlinov <[e-mail]>
> > 
> > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> > 
> > 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.
> > 
> > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > Cc: [e-mail]
> > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > [dianders: resolved merge conflicts with mainline]
> > Signed-off-by: Douglas Anderson <[e-mail]>
> > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > This is the backport of the patch referenced above to 4.19 as was done
> > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > presumably applies to some older kernels.  NOTE that the problem
> > itself has existed for a long time, but continuing to backport this
> > exact solution to super old kernels is out of scope for me.  For those
> > truly interested feel free to reference the past discussion [1].
> > 
> > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > seem like a good idea to backport 17 patches to avoid the conflict.
> 
> Careful with this, you can't backport this to any kernels that don't
> have the sysfs ops locking changes or they will crash in sysfs code.

And what commit added that?

thanks,

greg k-h

Jason Gunthorpe July 11, 2019, 5:17 p.m. UTC | #4

On Thu, Jul 11, 2019 at 07:04:37PM +0200, Greg KH wrote:
> On Thu, Jul 11, 2019 at 01:39:15PM -0300, Jason Gunthorpe wrote:
> > On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > > From: Vadim Sukhomlinov <[e-mail]>
> > > 
> > > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> > > 
> > > 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.
> > > 
> > > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > > Cc: [e-mail]
> > > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > > [dianders: resolved merge conflicts with mainline]
> > > Signed-off-by: Douglas Anderson <[e-mail]>
> > > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > > This is the backport of the patch referenced above to 4.19 as was done
> > > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > > presumably applies to some older kernels.  NOTE that the problem
> > > itself has existed for a long time, but continuing to backport this
> > > exact solution to super old kernels is out of scope for me.  For those
> > > truly interested feel free to reference the past discussion [1].
> > > 
> > > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > > seem like a good idea to backport 17 patches to avoid the conflict.
> > 
> > Careful with this, you can't backport this to any kernels that don't
> > have the sysfs ops locking changes or they will crash in sysfs code.
> 
> And what commit added that?

commit 2677ca98ae377517930c183248221f69f771c921
Author: Jarkko Sakkinen <[e-mail]>
Date:   Sun Nov 4 11:38:27 2018 +0200

    tpm: use tpm_try_get_ops() in tpm-sysfs.c.
    
    Use tpm_try_get_ops() in tpm-sysfs.c so that we can consider moving
    other decorations (locking, localities, power management for example)
    inside it. This direction can be of course taken only after other call
    sites for tpm_transmit() have been treated in the same way.

The last sentence suggests there are other patches needed too though..

Jason

Greg KH July 11, 2019, 5:26 p.m. UTC | #5

On Thu, Jul 11, 2019 at 02:17:26PM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 11, 2019 at 07:04:37PM +0200, Greg KH wrote:
> > On Thu, Jul 11, 2019 at 01:39:15PM -0300, Jason Gunthorpe wrote:
> > > On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > > > From: Vadim Sukhomlinov <[e-mail]>
> > > > 
> > > > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> > > > 
> > > > 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.
> > > > 
> > > > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > > > Cc: [e-mail]
> > > > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > > > [dianders: resolved merge conflicts with mainline]
> > > > Signed-off-by: Douglas Anderson <[e-mail]>
> > > > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > > > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > > > This is the backport of the patch referenced above to 4.19 as was done
> > > > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > > > presumably applies to some older kernels.  NOTE that the problem
> > > > itself has existed for a long time, but continuing to backport this
> > > > exact solution to super old kernels is out of scope for me.  For those
> > > > truly interested feel free to reference the past discussion [1].
> > > > 
> > > > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > > > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > > > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > > > seem like a good idea to backport 17 patches to avoid the conflict.
> > > 
> > > Careful with this, you can't backport this to any kernels that don't
> > > have the sysfs ops locking changes or they will crash in sysfs code.
> > 
> > And what commit added that?
> 
> commit 2677ca98ae377517930c183248221f69f771c921
> Author: Jarkko Sakkinen <[e-mail]>
> Date:   Sun Nov 4 11:38:27 2018 +0200
> 
>     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>     
>     Use tpm_try_get_ops() in tpm-sysfs.c so that we can consider moving
>     other decorations (locking, localities, power management for example)
>     inside it. This direction can be of course taken only after other call
>     sites for tpm_transmit() have been treated in the same way.
> 
> The last sentence suggests there are other patches needed too though..

So 5.1.  So does this original patch need to go into the 5.2 and 5.1
kernels?

thanks,

greg k-h

Doug Anderson July 11, 2019, 5:28 p.m. UTC | #6

Hi,

On Thu, Jul 11, 2019 at 10:26 AM Greg KH <[e-mail]> wrote:
>
> On Thu, Jul 11, 2019 at 02:17:26PM -0300, Jason Gunthorpe wrote:
> > On Thu, Jul 11, 2019 at 07:04:37PM +0200, Greg KH wrote:
> > > On Thu, Jul 11, 2019 at 01:39:15PM -0300, Jason Gunthorpe wrote:
> > > > On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > > > > From: Vadim Sukhomlinov <[e-mail]>
> > > > >
> > > > > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> > > > >
> > > > > 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.
> > > > >
> > > > > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > > > > Cc: [e-mail]
> > > > > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > > > > [dianders: resolved merge conflicts with mainline]
> > > > > Signed-off-by: Douglas Anderson <[e-mail]>
> > > > > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > > > > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > > > > This is the backport of the patch referenced above to 4.19 as was done
> > > > > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > > > > presumably applies to some older kernels.  NOTE that the problem
> > > > > itself has existed for a long time, but continuing to backport this
> > > > > exact solution to super old kernels is out of scope for me.  For those
> > > > > truly interested feel free to reference the past discussion [1].
> > > > >
> > > > > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > > > > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > > > > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > > > > seem like a good idea to backport 17 patches to avoid the conflict.
> > > >
> > > > Careful with this, you can't backport this to any kernels that don't
> > > > have the sysfs ops locking changes or they will crash in sysfs code.
> > >
> > > And what commit added that?
> >
> > commit 2677ca98ae377517930c183248221f69f771c921
> > Author: Jarkko Sakkinen <[e-mail]>
> > Date:   Sun Nov 4 11:38:27 2018 +0200
> >
> >     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> >
> >     Use tpm_try_get_ops() in tpm-sysfs.c so that we can consider moving
> >     other decorations (locking, localities, power management for example)
> >     inside it. This direction can be of course taken only after other call
> >     sites for tpm_transmit() have been treated in the same way.
> >
> > The last sentence suggests there are other patches needed too though..
>
> So 5.1.  So does this original patch need to go into the 5.2 and 5.1
> kernels?

The patch ("Fix TPM 1.2 Shutdown sequence to prevent future TPM
operations")?  It's already done.  It just got merge conflicts when
going back to 4.19 which is why I sent the backport.

-Doug

Jarkko Sakkinen July 11, 2019, 6:34 p.m. UTC | #7

On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> From: Vadim Sukhomlinov <[e-mail]>
> 
> commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> 
> 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.
> 
> Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> Cc: [e-mail]
> Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> [dianders: resolved merge conflicts with mainline]
> Signed-off-by: Douglas Anderson <[e-mail]>
> Reviewed-by: Jarkko Sakkinen <[e-mail]>
> Signed-off-by: Jarkko Sakkinen <[e-mail]>
> ---
> This is the backport of the patch referenced above to 4.19 as was done
> in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> presumably applies to some older kernels.  NOTE that the problem
> itself has existed for a long time, but continuing to backport this
> exact solution to super old kernels is out of scope for me.  For those
> truly interested feel free to reference the past discussion [1].
> 
> Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> chip power gating out of tpm_transmit()") and commit 719b7d81f204
> ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> seem like a good idea to backport 17 patches to avoid the conflict.

Acked-by: Jarkko Sakkinen <[e-mail]>

/Jarkko

Jarkko Sakkinen July 11, 2019, 6:35 p.m. UTC | #8

On Thu, Jul 11, 2019 at 01:39:15PM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > From: Vadim Sukhomlinov <[e-mail]>
> > 
> > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> > 
> > 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.
> > 
> > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > Cc: [e-mail]
> > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > [dianders: resolved merge conflicts with mainline]
> > Signed-off-by: Douglas Anderson <[e-mail]>
> > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > This is the backport of the patch referenced above to 4.19 as was done
> > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > presumably applies to some older kernels.  NOTE that the problem
> > itself has existed for a long time, but continuing to backport this
> > exact solution to super old kernels is out of scope for me.  For those
> > truly interested feel free to reference the past discussion [1].
> > 
> > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > seem like a good idea to backport 17 patches to avoid the conflict.
> 
> Careful with this, you can't backport this to any kernels that don't
> have the sysfs ops locking changes or they will crash in sysfs code.

Oops, I was way too fast! Thanks Jason.

/Jarkko

Jarkko Sakkinen July 11, 2019, 7:43 p.m. UTC | #9

On Thu, Jul 11, 2019 at 09:35:33PM +0300, Jarkko Sakkinen wrote:
> > Careful with this, you can't backport this to any kernels that don't
> > have the sysfs ops locking changes or they will crash in sysfs code.
> 
> Oops, I was way too fast! Thanks Jason.

Hmm... hold on a second.

How would the crash realize? I mean this is at the point when user space
should not be active. Secondly, why the crash would not realize with
TPM2? The only thing the fix is doing is to do the same thing with TPM1
essentially.

/Jarkko

Jason Gunthorpe July 11, 2019, 7:46 p.m. UTC | #10

On Thu, Jul 11, 2019 at 10:43:13PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 11, 2019 at 09:35:33PM +0300, Jarkko Sakkinen wrote:
> > > Careful with this, you can't backport this to any kernels that don't
> > > have the sysfs ops locking changes or they will crash in sysfs code.
> > 
> > Oops, I was way too fast! Thanks Jason.
> 
> Hmm... hold on a second.
> 
> How would the crash realize? I mean this is at the point when user space
> should not be active. 

Not strictly, AFAIK

> Secondly, why the crash would not realize with
> TPM2? The only thing the fix is doing is to do the same thing with TPM1
> essentially.

TPM2 doesn't use the unlocked sysfs path

Jason

Doug Anderson July 11, 2019, 7:55 p.m. UTC | #11

Hi,

On Thu, Jul 11, 2019 at 12:43 PM Jarkko Sakkinen
<[e-mail]> wrote:
>
> On Thu, Jul 11, 2019 at 09:35:33PM +0300, Jarkko Sakkinen wrote:
> > > Careful with this, you can't backport this to any kernels that don't
> > > have the sysfs ops locking changes or they will crash in sysfs code.
> >
> > Oops, I was way too fast! Thanks Jason.
>
> Hmm... hold on a second.
>
> How would the crash realize? I mean this is at the point when user space
> should not be active. Secondly, why the crash would not realize with
> TPM2? The only thing the fix is doing is to do the same thing with TPM1
> essentially.

I will continue to remind that I'm pretty TPM-clueless (mostly I just
took someone else's patch and posted it), but I will note that people
on the Chrome OS team seemed concerned by the sysfs locking too.
After seeing Jason's message this morning I dug a little bit and found
<https://crbug.com/819265>

-Doug

Jarkko Sakkinen July 12, 2019, 3:31 a.m. UTC | #12

On Thu, Jul 11, 2019 at 04:46:26PM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 11, 2019 at 10:43:13PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Jul 11, 2019 at 09:35:33PM +0300, Jarkko Sakkinen wrote:
> > > > Careful with this, you can't backport this to any kernels that don't
> > > > have the sysfs ops locking changes or they will crash in sysfs code.
> > > 
> > > Oops, I was way too fast! Thanks Jason.
> > 
> > Hmm... hold on a second.
> > 
> > How would the crash realize? I mean this is at the point when user space
> > should not be active. 
> 
> Not strictly, AFAIK
> 
> > Secondly, why the crash would not realize with
> > TPM2? The only thing the fix is doing is to do the same thing with TPM1
> > essentially.
> 
> TPM2 doesn't use the unlocked sysfs path

Gah, sorry :-) I should have known that.

I can go through the patches needed when I come back from my leave after
two weeks.

/Jarkko

Jarkko Sakkinen July 12, 2019, 3:35 a.m. UTC | #13

On Fri, Jul 12, 2019 at 06:31:38AM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 11, 2019 at 04:46:26PM -0300, Jason Gunthorpe wrote:
> > On Thu, Jul 11, 2019 at 10:43:13PM +0300, Jarkko Sakkinen wrote:
> > > On Thu, Jul 11, 2019 at 09:35:33PM +0300, Jarkko Sakkinen wrote:
> > > > > Careful with this, you can't backport this to any kernels that don't
> > > > > have the sysfs ops locking changes or they will crash in sysfs code.
> > > > 
> > > > Oops, I was way too fast! Thanks Jason.
> > > 
> > > Hmm... hold on a second.
> > > 
> > > How would the crash realize? I mean this is at the point when user space
> > > should not be active. 
> > 
> > Not strictly, AFAIK
> > 
> > > Secondly, why the crash would not realize with
> > > TPM2? The only thing the fix is doing is to do the same thing with TPM1
> > > essentially.
> > 
> > TPM2 doesn't use the unlocked sysfs path
> 
> Gah, sorry :-) I should have known that.
> 
> I can go through the patches needed when I come back from my leave after
> two weeks.

It might require a number of patches but maybe it makes also overally sense
to fix the racy sysfs code in stable kernels.

/Jarkko

Greg KH July 12, 2019, 11:50 a.m. UTC | #14

On Thu, Jul 11, 2019 at 10:28:01AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jul 11, 2019 at 10:26 AM Greg KH <[e-mail]> wrote:
> >
> > On Thu, Jul 11, 2019 at 02:17:26PM -0300, Jason Gunthorpe wrote:
> > > On Thu, Jul 11, 2019 at 07:04:37PM +0200, Greg KH wrote:
> > > > On Thu, Jul 11, 2019 at 01:39:15PM -0300, Jason Gunthorpe wrote:
> > > > > On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > > > > > From: Vadim Sukhomlinov <[e-mail]>
> > > > > >
> > > > > > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > > > > > Cc: [e-mail]
> > > > > > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > > > > > [dianders: resolved merge conflicts with mainline]
> > > > > > Signed-off-by: Douglas Anderson <[e-mail]>
> > > > > > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > > > > > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > > > > > This is the backport of the patch referenced above to 4.19 as was done
> > > > > > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > > > > > presumably applies to some older kernels.  NOTE that the problem
> > > > > > itself has existed for a long time, but continuing to backport this
> > > > > > exact solution to super old kernels is out of scope for me.  For those
> > > > > > truly interested feel free to reference the past discussion [1].
> > > > > >
> > > > > > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > > > > > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > > > > > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > > > > > seem like a good idea to backport 17 patches to avoid the conflict.
> > > > >
> > > > > Careful with this, you can't backport this to any kernels that don't
> > > > > have the sysfs ops locking changes or they will crash in sysfs code.
> > > >
> > > > And what commit added that?
> > >
> > > commit 2677ca98ae377517930c183248221f69f771c921
> > > Author: Jarkko Sakkinen <[e-mail]>
> > > Date:   Sun Nov 4 11:38:27 2018 +0200
> > >
> > >     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> > >
> > >     Use tpm_try_get_ops() in tpm-sysfs.c so that we can consider moving
> > >     other decorations (locking, localities, power management for example)
> > >     inside it. This direction can be of course taken only after other call
> > >     sites for tpm_transmit() have been treated in the same way.
> > >
> > > The last sentence suggests there are other patches needed too though..
> >
> > So 5.1.  So does this original patch need to go into the 5.2 and 5.1
> > kernels?
> 
> The patch ("Fix TPM 1.2 Shutdown sequence to prevent future TPM
> operations")?  It's already done.  It just got merge conflicts when
> going back to 4.19 which is why I sent the backport.

But the sysfs comment means I should not apply this backport then?

Totally confused by this long thread, sorry.

What am I supposed to do for the stable trees here?

thanks,

greg k-h

Jason Gunthorpe July 12, 2019, 11:58 a.m. UTC | #15

On Fri, Jul 12, 2019 at 06:35:56AM +0300, Jarkko Sakkinen wrote:
> On Fri, Jul 12, 2019 at 06:31:38AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Jul 11, 2019 at 04:46:26PM -0300, Jason Gunthorpe wrote:
> > > On Thu, Jul 11, 2019 at 10:43:13PM +0300, Jarkko Sakkinen wrote:
> > > > On Thu, Jul 11, 2019 at 09:35:33PM +0300, Jarkko Sakkinen wrote:
> > > > > > Careful with this, you can't backport this to any kernels that don't
> > > > > > have the sysfs ops locking changes or they will crash in sysfs code.
> > > > > 
> > > > > Oops, I was way too fast! Thanks Jason.
> > > > 
> > > > Hmm... hold on a second.
> > > > 
> > > > How would the crash realize? I mean this is at the point when user space
> > > > should not be active. 
> > > 
> > > Not strictly, AFAIK
> > > 
> > > > Secondly, why the crash would not realize with
> > > > TPM2? The only thing the fix is doing is to do the same thing with TPM1
> > > > essentially.
> > > 
> > > TPM2 doesn't use the unlocked sysfs path
> > 
> > Gah, sorry :-) I should have known that.
> > 
> > I can go through the patches needed when I come back from my leave after
> > two weeks.
> 
> It might require a number of patches but maybe it makes also overally sense
> to fix the racy sysfs code in stable kernels.

The sysfs isn't racy, it justs used a different locking scheme

Jason

Doug Anderson July 12, 2019, 3 p.m. UTC | #16

Hi,

On Fri, Jul 12, 2019 at 4:50 AM Greg KH <[e-mail]> wrote:
>
> On Thu, Jul 11, 2019 at 10:28:01AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jul 11, 2019 at 10:26 AM Greg KH <[e-mail]> wrote:
> > >
> > > On Thu, Jul 11, 2019 at 02:17:26PM -0300, Jason Gunthorpe wrote:
> > > > On Thu, Jul 11, 2019 at 07:04:37PM +0200, Greg KH wrote:
> > > > > On Thu, Jul 11, 2019 at 01:39:15PM -0300, Jason Gunthorpe wrote:
> > > > > > On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > > > > > > From: Vadim Sukhomlinov <[e-mail]>
> > > > > > >
> > > > > > > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > > > > > > Cc: [e-mail]
> > > > > > > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > > > > > > [dianders: resolved merge conflicts with mainline]
> > > > > > > Signed-off-by: Douglas Anderson <[e-mail]>
> > > > > > > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > > > > > > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > > > > > > This is the backport of the patch referenced above to 4.19 as was done
> > > > > > > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > > > > > > presumably applies to some older kernels.  NOTE that the problem
> > > > > > > itself has existed for a long time, but continuing to backport this
> > > > > > > exact solution to super old kernels is out of scope for me.  For those
> > > > > > > truly interested feel free to reference the past discussion [1].
> > > > > > >
> > > > > > > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > > > > > > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > > > > > > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > > > > > > seem like a good idea to backport 17 patches to avoid the conflict.
> > > > > >
> > > > > > Careful with this, you can't backport this to any kernels that don't
> > > > > > have the sysfs ops locking changes or they will crash in sysfs code.
> > > > >
> > > > > And what commit added that?
> > > >
> > > > commit 2677ca98ae377517930c183248221f69f771c921
> > > > Author: Jarkko Sakkinen <[e-mail]>
> > > > Date:   Sun Nov 4 11:38:27 2018 +0200
> > > >
> > > >     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> > > >
> > > >     Use tpm_try_get_ops() in tpm-sysfs.c so that we can consider moving
> > > >     other decorations (locking, localities, power management for example)
> > > >     inside it. This direction can be of course taken only after other call
> > > >     sites for tpm_transmit() have been treated in the same way.
> > > >
> > > > The last sentence suggests there are other patches needed too though..
> > >
> > > So 5.1.  So does this original patch need to go into the 5.2 and 5.1
> > > kernels?
> >
> > The patch ("Fix TPM 1.2 Shutdown sequence to prevent future TPM
> > operations")?  It's already done.  It just got merge conflicts when
> > going back to 4.19 which is why I sent the backport.
>
> But the sysfs comment means I should not apply this backport then?
>
> Totally confused by this long thread, sorry.
>
> What am I supposed to do for the stable trees here?

I think the answer is to drop my backport for now and Jarkko says
he'll take a fresh look at it in 2 weeks when he's back from his
leave.  Thus my understanding:

* On mainline: fixed

* On 5.2 / 5.1: you've already got this picked to stable.  Good

* On 4.14 / 4.19: Jarkko will look at in 2 weeks.

* On 4.9 and older: I'd propose skipping unless someone is known to
need a solution here.

-Doug

Jarkko Sakkinen July 12, 2019, 3:21 p.m. UTC | #17

On Fri, 2019-07-12 at 13:50 +0200, Greg KH wrote:
> But the sysfs comment means I should not apply this backport then?
> 
> Totally confused by this long thread, sorry.
> 
> What am I supposed to do for the stable trees here?

I'll work out a proper patch set for stable kernels with necessary
patches and cover letter with a brief summary in the week starting
29th of this month when I come back from vacation.

/Jarkko

Greg KH July 12, 2019, 3:27 p.m. UTC | #18

On Fri, Jul 12, 2019 at 08:00:12AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jul 12, 2019 at 4:50 AM Greg KH <[e-mail]> wrote:
> >
> > On Thu, Jul 11, 2019 at 10:28:01AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, Jul 11, 2019 at 10:26 AM Greg KH <[e-mail]> wrote:
> > > >
> > > > On Thu, Jul 11, 2019 at 02:17:26PM -0300, Jason Gunthorpe wrote:
> > > > > On Thu, Jul 11, 2019 at 07:04:37PM +0200, Greg KH wrote:
> > > > > > On Thu, Jul 11, 2019 at 01:39:15PM -0300, Jason Gunthorpe wrote:
> > > > > > > On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > > > > > > > From: Vadim Sukhomlinov <[e-mail]>
> > > > > > > >
> > > > > > > > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> > > > > > > >
> > > > > > > > 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.
> > > > > > > >
> > > > > > > > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > > > > > > > Cc: [e-mail]
> > > > > > > > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > > > > > > > [dianders: resolved merge conflicts with mainline]
> > > > > > > > Signed-off-by: Douglas Anderson <[e-mail]>
> > > > > > > > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > > > > > > > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > > > > > > > This is the backport of the patch referenced above to 4.19 as was done
> > > > > > > > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > > > > > > > presumably applies to some older kernels.  NOTE that the problem
> > > > > > > > itself has existed for a long time, but continuing to backport this
> > > > > > > > exact solution to super old kernels is out of scope for me.  For those
> > > > > > > > truly interested feel free to reference the past discussion [1].
> > > > > > > >
> > > > > > > > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > > > > > > > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > > > > > > > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > > > > > > > seem like a good idea to backport 17 patches to avoid the conflict.
> > > > > > >
> > > > > > > Careful with this, you can't backport this to any kernels that don't
> > > > > > > have the sysfs ops locking changes or they will crash in sysfs code.
> > > > > >
> > > > > > And what commit added that?
> > > > >
> > > > > commit 2677ca98ae377517930c183248221f69f771c921
> > > > > Author: Jarkko Sakkinen <[e-mail]>
> > > > > Date:   Sun Nov 4 11:38:27 2018 +0200
> > > > >
> > > > >     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> > > > >
> > > > >     Use tpm_try_get_ops() in tpm-sysfs.c so that we can consider moving
> > > > >     other decorations (locking, localities, power management for example)
> > > > >     inside it. This direction can be of course taken only after other call
> > > > >     sites for tpm_transmit() have been treated in the same way.
> > > > >
> > > > > The last sentence suggests there are other patches needed too though..
> > > >
> > > > So 5.1.  So does this original patch need to go into the 5.2 and 5.1
> > > > kernels?
> > >
> > > The patch ("Fix TPM 1.2 Shutdown sequence to prevent future TPM
> > > operations")?  It's already done.  It just got merge conflicts when
> > > going back to 4.19 which is why I sent the backport.
> >
> > But the sysfs comment means I should not apply this backport then?
> >
> > Totally confused by this long thread, sorry.
> >
> > What am I supposed to do for the stable trees here?
> 
> I think the answer is to drop my backport for now and Jarkko says
> he'll take a fresh look at it in 2 weeks when he's back from his
> leave.  Thus my understanding:
> 
> * On mainline: fixed
> 
> * On 5.2 / 5.1: you've already got this picked to stable.  Good
> 
> * On 4.14 / 4.19: Jarkko will look at in 2 weeks.
> 
> * On 4.9 and older: I'd propose skipping unless someone is known to
> need a solution here.

Thanks, that makes sense now.

greg k-h

Jarkko Sakkinen July 12, 2019, 3:47 p.m. UTC | #19

On Fri, 2019-07-12 at 08:00 -0700, Doug Anderson wrote:
> * On 5.2 / 5.1: you've already got this picked to stable.  Good
> 
> * On 4.14 / 4.19: Jarkko will look at in 2 weeks.
> 
> * On 4.9 and older: I'd propose skipping unless someone is known to
> need a solution here.

I'll prioritize 4.14 and 4.19.

If it doesn't become a too big struggle, I'll try to fix also older
but no final word on that at this point.

/Jarkko

Jarkko Sakkinen Aug. 5, 2019, 9:05 p.m. UTC | #20

On Fri, Jul 12, 2019 at 05:27:34PM +0200, Greg KH wrote:
> On Fri, Jul 12, 2019 at 08:00:12AM -0700, Doug Anderson wrote:
> > Hi,
> > 
> > On Fri, Jul 12, 2019 at 4:50 AM Greg KH <[e-mail]> wrote:
> > >
> > > On Thu, Jul 11, 2019 at 10:28:01AM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Thu, Jul 11, 2019 at 10:26 AM Greg KH <[e-mail]> wrote:
> > > > >
> > > > > On Thu, Jul 11, 2019 at 02:17:26PM -0300, Jason Gunthorpe wrote:
> > > > > > On Thu, Jul 11, 2019 at 07:04:37PM +0200, Greg KH wrote:
> > > > > > > On Thu, Jul 11, 2019 at 01:39:15PM -0300, Jason Gunthorpe wrote:
> > > > > > > > On Thu, Jul 11, 2019 at 09:29:19AM -0700, Douglas Anderson wrote:
> > > > > > > > > From: Vadim Sukhomlinov <[e-mail]>
> > > > > > > > >
> > > > > > > > > commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream.
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
> > > > > > > > > Cc: [e-mail]
> > > > > > > > > Signed-off-by: Vadim Sukhomlinov <[e-mail]>
> > > > > > > > > [dianders: resolved merge conflicts with mainline]
> > > > > > > > > Signed-off-by: Douglas Anderson <[e-mail]>
> > > > > > > > > Reviewed-by: Jarkko Sakkinen <[e-mail]>
> > > > > > > > > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > > > > > > > > This is the backport of the patch referenced above to 4.19 as was done
> > > > > > > > > in Chrome OS.  See <https://crrev.com/c/1495114> for details.  It
> > > > > > > > > presumably applies to some older kernels.  NOTE that the problem
> > > > > > > > > itself has existed for a long time, but continuing to backport this
> > > > > > > > > exact solution to super old kernels is out of scope for me.  For those
> > > > > > > > > truly interested feel free to reference the past discussion [1].
> > > > > > > > >
> > > > > > > > > Reason for backport: mainline has commit a3fbfae82b4c ("tpm: take TPM
> > > > > > > > > chip power gating out of tpm_transmit()") and commit 719b7d81f204
> > > > > > > > > ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") and it didn't
> > > > > > > > > seem like a good idea to backport 17 patches to avoid the conflict.
> > > > > > > >
> > > > > > > > Careful with this, you can't backport this to any kernels that don't
> > > > > > > > have the sysfs ops locking changes or they will crash in sysfs code.
> > > > > > >
> > > > > > > And what commit added that?
> > > > > >
> > > > > > commit 2677ca98ae377517930c183248221f69f771c921
> > > > > > Author: Jarkko Sakkinen <[e-mail]>
> > > > > > Date:   Sun Nov 4 11:38:27 2018 +0200
> > > > > >
> > > > > >     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> > > > > >
> > > > > >     Use tpm_try_get_ops() in tpm-sysfs.c so that we can consider moving
> > > > > >     other decorations (locking, localities, power management for example)
> > > > > >     inside it. This direction can be of course taken only after other call
> > > > > >     sites for tpm_transmit() have been treated in the same way.
> > > > > >
> > > > > > The last sentence suggests there are other patches needed too though..
> > > > >
> > > > > So 5.1.  So does this original patch need to go into the 5.2 and 5.1
> > > > > kernels?
> > > >
> > > > The patch ("Fix TPM 1.2 Shutdown sequence to prevent future TPM
> > > > operations")?  It's already done.  It just got merge conflicts when
> > > > going back to 4.19 which is why I sent the backport.
> > >
> > > But the sysfs comment means I should not apply this backport then?
> > >
> > > Totally confused by this long thread, sorry.
> > >
> > > What am I supposed to do for the stable trees here?
> > 
> > I think the answer is to drop my backport for now and Jarkko says
> > he'll take a fresh look at it in 2 weeks when he's back from his
> > leave.  Thus my understanding:
> > 
> > * On mainline: fixed
> > 
> > * On 5.2 / 5.1: you've already got this picked to stable.  Good
> > 
> > * On 4.14 / 4.19: Jarkko will look at in 2 weeks.
> > 
> > * On 4.9 and older: I'd propose skipping unless someone is known to
> > need a solution here.
> 
> Thanks, that makes sense now.
> 
> greg k-h

I have not forgotten this but might have to postpone the backport after
Linux Plumbers. Just have lots of stuff in my queue ATM but right after
the conference I have good slot to do the backports.

/Jarkko

Patch

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 46caadca916a..f784b6fd93b4 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -187,12 +187,11 @@  static int tpm_class_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);
+	down_write(&chip->ops_sem);
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		tpm2_shutdown(chip, TPM2_SU_CLEAR);
-		chip->ops = NULL;
-		up_write(&chip->ops_sem);
-	}
+	chip->ops = NULL;
+	up_write(&chip->ops_sem);
 
 	return 0;
 }