Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Peter Huewe <peterhuewe@gmx.de>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	Andrey Pronin <apronin@chromium.org>,
	linux-kernel@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-integrity@vger.kernel.org, devicetree@vger.kernel.org,
	Duncan Laurie <dlaurie@chromium.org>,
	Guenter Roeck <groeck@chromium.org>,
	Matt Mackall <mpm@selenic.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	<linux-crypto@vger.kernel.org>
Subject: Re: [PATCH 1/8] tpm: block messages while suspended
Date: Thu, 20 Jun 2019 18:03:17 -0700
Message-ID: <5d0c2cd6.1c69fb81.e66af.32bf@mx.google.com> (raw)
In-Reply-To: <20190617225134.GA30762@ziepe.ca>

Quoting Jason Gunthorpe (2019-06-17 15:51:34)
> On Fri, Jun 14, 2019 at 11:12:36AM -0700, Stephen Boyd wrote:
> > Quoting Jason Gunthorpe (2019-06-13 16:26:13)
> > > On Thu, Jun 13, 2019 at 11:09:24AM -0700, Stephen Boyd wrote:
> > > > From: Andrey Pronin <apronin@chromium.org>
> > > > 
> > > > Other drivers or userspace may initiate sending a message to the tpm
> > > > while the device itself and the controller of the bus it is on are
> > > > suspended. That may break the bus driver logic.
> > > > Block sending messages while the device is suspended.
> > > > 
> > > > Signed-off-by: Andrey Pronin <apronin@chromium.org>
> > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > > 
> > > > I don't think this was ever posted before.
> > > 
> > > Use a real lock.
> > > 
> > 
> > To make sure the bit is tested under a lock so that suspend/resume can't
> > update the bit in parallel?
> 
> No, just use a real lock, don't make locks out of test bit/set bit
> 

Ok. I looked back on the history of this change in our kernel (seems it
wasn't attempted upstream for some time) and it looks like the problem
may have been that the khwrng kthread (i.e. hwrng_fill()) isn't frozen
across suspend/resume. This kthread runs concurrently with devices being
resumed, the cr50 hardware is still suspended, and then a tpm command is
sent and it hangs the I2C bus because the device hasn't been properly
resumed yet.

I suspect a better approach than trying to hold of all TPM commands
across suspend/resume would be to fix the caller here to not even try to
read the hwrng during this time. It's a general problem for other hwrngs
that have some suspend/resume hooks too. This kthread is going to be
running while suspend/resume is going on if the random entropy gets too
low, and that probably shouldn't be the case.

What do you think of the attached patch? I haven't tested it, but it
would make sure that the kthread is frozen so that the hardware can be
resumed before the kthread is thawed and tries to go touch the hardware.

----8<-----
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 95be7228f327..3b88af3149a7 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/freezer.h>
 #include <linux/fs.h>
 #include <linux/hw_random.h>
 #include <linux/kernel.h>
@@ -421,7 +422,9 @@ static int hwrng_fillfn(void *unused)
 {
 	long rc;
 
-	while (!kthread_should_stop()) {
+	set_freezable();
+
+	while (!kthread_freezable_should_stop(NULL)) {
 		struct hwrng *rng;
 
 		rng = get_current_rng();

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 18:09 [PATCH 0/8] tpm: Add driver for cr50 Stephen Boyd
2019-06-13 18:09 ` [PATCH 1/8] tpm: block messages while suspended Stephen Boyd
2019-06-13 23:26   ` Jason Gunthorpe
2019-06-14 18:12     ` Stephen Boyd
2019-06-17 22:51       ` Jason Gunthorpe
2019-06-21  1:03         ` Stephen Boyd [this message]
2019-06-24 14:26           ` Herbert Xu
2019-07-16 18:28             ` Stephen Boyd
2019-06-14 15:27   ` Jarkko Sakkinen
2019-06-14 18:13     ` Stephen Boyd
2019-06-17 16:38       ` Jarkko Sakkinen
2019-06-13 18:09 ` [PATCH 2/8] tpm_tis_core: add optional max xfer size check Stephen Boyd
2019-06-14 15:24   ` Jarkko Sakkinen
2019-06-13 18:09 ` [PATCH 3/8] tpm_tis_spi: add max xfer size Stephen Boyd
2019-06-14 15:25   ` Jarkko Sakkinen
2019-06-13 18:09 ` [PATCH 4/8] dt-bindings: tpm: document properties for cr50 Stephen Boyd
2019-07-09 14:41   ` Rob Herring
2019-06-13 18:09 ` [PATCH 5/8] tpm: add driver for cr50 on SPI Stephen Boyd
2019-06-13 18:09 ` [PATCH 6/8] tpm: Add driver for cr50 on I2C Stephen Boyd
2019-06-13 18:09 ` [PATCH 7/8] tpm: add sysfs attributes for tpm2 Stephen Boyd
2019-06-13 23:30   ` Jason Gunthorpe
2019-06-14 15:31   ` Jarkko Sakkinen
2019-06-13 18:09 ` [PATCH 8/8] tpm: add legacy " Stephen Boyd
2019-06-13 23:44   ` Jason Gunthorpe

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5d0c2cd6.1c69fb81.e66af.32bf@mx.google.com \
    --to=swboyd@chromium.org \
    --cc=apronin@chromium.org \
    --cc=arnd@arndb.de \
    --cc=devicetree@vger.kernel.org \
    --cc=dlaurie@chromium.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=groeck@chromium.org \
    --cc=herbert@gondor.apana.org.au \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jgg@ziepe.ca \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mpm@selenic.com \
    --cc=peterhuewe@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org linux-integrity@archiver.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/ public-inbox