tpmdd-devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Shaikh, Azhar" <azhar.shaikh@intel.com>
To: "jarkko.sakkinen@linux.intel.com"
	<jarkko.sakkinen@linux.intel.com>,
	"jgunthorpe@obsidianresearch.com"
	<jgunthorpe@obsidianresearch.com>,
	"peterhuewe@gmx.de" <peterhuewe@gmx.de>
Cc: "linux-security-module@vger.kernel.org"
	<linux-security-module@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"tpmdd-devel@lists.sourceforge.net"
	<tpmdd-devel@lists.sourceforge.net>
Subject: RE: [PATCH v4 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()
Date: Wed, 22 Nov 2017 19:57:32 +0000
Message-ID: <5FFFAD06ADE1CA4381B3F0F7C6AF582896F1F2@ORSMSX109.amr.corp.intel.com> (raw)
In-Reply-To: <1511303964-56294-2-git-send-email-azhar.shaikh@intel.com>

I found an issue with this patch. 

In tpm_transmit() function after clk_enable is called with false, the assumption was whenever the next TPM transaction happens, the clock will be stopped in tpm_platform_end_xfer(). But in cases where after tpm_transmit is completed and there is no TPM transaction until  next TPM command is being sent, the clocks will be running till then.

Can we write a dummy byte after clk_enable is set to false? Is this possible/correct way?
Or else as Jason suggested earlier, will get rid of tpm_platform_begin_xfer() and tpm_platform_end_xfer() and have those implemented in the clk_enable() function and have a high level action of enabling and disabling clocks.

Regards,
Azhar Shaikh
 

>-----Original Message-----
>From: Shaikh, Azhar
>Sent: Tuesday, November 21, 2017 2:39 PM
>To: jarkko.sakkinen@linux.intel.com; jgunthorpe@obsidianresearch.com;
>peterhuewe@gmx.de
>Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
>tpmdd-devel@lists.sourceforge.net; Shaikh, Azhar <azhar.shaikh@intel.com>
>Subject: [PATCH v4 1/2] tpm: Keep CLKRUN enabled throughout the duration
>of transmit_cmd()
>
>Commit 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell
>systems") disabled CLKRUN protocol during TPM transactions and re-enabled
>once the transaction is completed. But there were still some corner cases
>observed where, reading of TPM header failed for savestate command while
>going to suspend, which resulted in suspend failure.
>To fix this issue keep the CLKRUN protocol disabled for the entire duration of a
>single TPM command and not disabling and re-enabling again for every TPM
>transaction. For the other TPM accesses outside the flow of TPM command
>processing, disable and re-enable CLKRUN protocol for every TPM access.
>
>Fixes: 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell
>systems")
>
>Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
>Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>---
> drivers/char/tpm/tpm-interface.c |  6 ++++++
> drivers/char/tpm/tpm_tis.c       | 44 ++++++++++++++++++++++++----------------
> drivers/char/tpm/tpm_tis_core.c  | 21 +++++++++++++++++++
>drivers/char/tpm/tpm_tis_core.h  |  1 +
> include/linux/tpm.h              |  1 +
> 5 files changed, 55 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-
>interface.c
>index 1d6729be4cd6..4661a810eab7 100644
>--- a/drivers/char/tpm/tpm-interface.c
>+++ b/drivers/char/tpm/tpm-interface.c
>@@ -413,6 +413,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
>tpm_space *space,
> 	if (chip->dev.parent)
> 		pm_runtime_get_sync(chip->dev.parent);
>
>+	if (chip->ops->clk_enable != NULL)
>+		chip->ops->clk_enable(chip, true);
>+
> 	/* Store the decision as chip->locality will be changed. */
> 	need_locality = chip->locality == -1;
>
>@@ -489,6 +492,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
>tpm_space *space,
> 		chip->locality = -1;
> 	}
> out_no_locality:
>+	if (chip->ops->clk_enable != NULL)
>+		chip->ops->clk_enable(chip, false);
>+
> 	if (chip->dev.parent)
> 		pm_runtime_put_sync(chip->dev.parent);
>
>diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index
>e2d1055fb814..76a7b64195c8 100644
>--- a/drivers/char/tpm/tpm_tis.c
>+++ b/drivers/char/tpm/tpm_tis.c
>@@ -46,6 +46,7 @@ struct tpm_info {
> struct tpm_tis_tcg_phy {
> 	struct tpm_tis_data priv;
> 	void __iomem *iobase;
>+	bool begin_xfer_done;
> };
>
> static inline struct tpm_tis_tcg_phy *to_tpm_tis_tcg_phy(struct tpm_tis_data
>*data) @@ -148,12 +149,15 @@ static inline bool is_bsw(void)
>
> /**
>  * tpm_platform_begin_xfer() - clear LPC CLKRUN_EN i.e. clocks will be
>running
>+ * @data:	struct tpm_tis_data instance
>  */
>-static void tpm_platform_begin_xfer(void)
>+static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
> {
> 	u32 clkrun_val;
>+	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>-	if (!is_bsw())
>+	if (!is_bsw() || ((data->flags & TPM_TIS_CLK_ENABLE) &&
>+					phy->begin_xfer_done))
> 		return;
>
> 	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET); @@
>-168,16 +172,21 @@ static void tpm_platform_begin_xfer(void)
> 	 */
> 	outb(0xCC, 0x80);
>
>+	if (!(data->flags & TPM_TIS_CLK_ENABLE))
>+		phy->begin_xfer_done = false;
>+	else
>+		phy->begin_xfer_done = true;
> }
>
> /**
>  * tpm_platform_end_xfer() - set LPC CLKRUN_EN i.e. clocks can be turned off
>+ * @data:	struct tpm_tis_data instance
>  */
>-static void tpm_platform_end_xfer(void)
>+static void tpm_platform_end_xfer(struct tpm_tis_data *data)
> {
> 	u32 clkrun_val;
>
>-	if (!is_bsw())
>+	if (!is_bsw() || (data->flags & TPM_TIS_CLK_ENABLE))
> 		return;
>
> 	clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET); @@
>-193,17 +202,16 @@ static void tpm_platform_end_xfer(void)
> 	outb(0xCC, 0x80);
>
> }
>+
> #else
> static inline bool is_bsw(void)
> {
> 	return false;
> }
>-
>-static void tpm_platform_begin_xfer(void)
>+static void tpm_platform_begin_xfer(struct tpm_tis_data *data)
> {
> }
>-
>-static void tpm_platform_end_xfer(void)
>+static void tpm_platform_end_xfer(struct tpm_tis_data *data)
> {
> }
> #endif
>@@ -213,12 +221,12 @@ static int tpm_tcg_read_bytes(struct tpm_tis_data
>*data, u32 addr, u16 len,  {
> 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>-	tpm_platform_begin_xfer();
>+	tpm_platform_begin_xfer(data);
>
> 	while (len--)
> 		*result++ = ioread8(phy->iobase + addr);
>
>-	tpm_platform_end_xfer();
>+	tpm_platform_end_xfer(data);
>
> 	return 0;
> }
>@@ -228,12 +236,12 @@ static int tpm_tcg_write_bytes(struct tpm_tis_data
>*data, u32 addr, u16 len,  {
> 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>-	tpm_platform_begin_xfer();
>+	tpm_platform_begin_xfer(data);
>
> 	while (len--)
> 		iowrite8(*value++, phy->iobase + addr);
>
>-	tpm_platform_end_xfer();
>+	tpm_platform_end_xfer(data);
>
> 	return 0;
> }
>@@ -242,11 +250,11 @@ static int tpm_tcg_read16(struct tpm_tis_data
>*data, u32 addr, u16 *result)  {
> 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>-	tpm_platform_begin_xfer();
>+	tpm_platform_begin_xfer(data);
>
> 	*result = ioread16(phy->iobase + addr);
>
>-	tpm_platform_end_xfer();
>+	tpm_platform_end_xfer(data);
>
> 	return 0;
> }
>@@ -255,11 +263,11 @@ static int tpm_tcg_read32(struct tpm_tis_data
>*data, u32 addr, u32 *result)  {
> 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>-	tpm_platform_begin_xfer();
>+	tpm_platform_begin_xfer(data);
>
> 	*result = ioread32(phy->iobase + addr);
>
>-	tpm_platform_end_xfer();
>+	tpm_platform_end_xfer(data);
>
> 	return 0;
> }
>@@ -268,11 +276,11 @@ static int tpm_tcg_write32(struct tpm_tis_data
>*data, u32 addr, u32 value)  {
> 	struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
>
>-	tpm_platform_begin_xfer();
>+	tpm_platform_begin_xfer(data);
>
> 	iowrite32(value, phy->iobase + addr);
>
>-	tpm_platform_end_xfer();
>+	tpm_platform_end_xfer(data);
>
> 	return 0;
> }
>diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c index fdde971bc810..64f5b46f5bf2
>100644
>--- a/drivers/char/tpm/tpm_tis_core.c
>+++ b/drivers/char/tpm/tpm_tis_core.c
>@@ -661,6 +661,26 @@ void tpm_tis_remove(struct tpm_chip *chip)  }
>EXPORT_SYMBOL_GPL(tpm_tis_remove);
>
>+/**
>+ * tpm_tis_clkrun_enable() - Keep clkrun protocol disabled for entire
>duration
>+ *                           of a single TPM command
>+ * @chip:	TPM chip to use
>+ * @value:	1 - Disable CLKRUN protocol, so that clocks are free running
>+ *		0 - Enable CLKRUN protocol
>+ */
>+static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) {
>+	struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
>+
>+	if (!IS_ENABLED(CONFIG_X86))
>+		return;
>+
>+	if (value)
>+		data->flags |= TPM_TIS_CLK_ENABLE;
>+	else
>+		data->flags &= ~TPM_TIS_CLK_ENABLE;
>+}
>+
> static const struct tpm_class_ops tpm_tis = {
> 	.flags = TPM_OPS_AUTO_STARTUP,
> 	.status = tpm_tis_status,
>@@ -673,6 +693,7 @@ void tpm_tis_remove(struct tpm_chip *chip)
> 	.req_canceled = tpm_tis_req_canceled,
> 	.request_locality = request_locality,
> 	.relinquish_locality = release_locality,
>+	.clk_enable = tpm_tis_clkrun_enable,
> };
>
> int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, diff
>--git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>index 6bbac319ff3b..281f744a1a44 100644
>--- a/drivers/char/tpm/tpm_tis_core.h
>+++ b/drivers/char/tpm/tpm_tis_core.h
>@@ -81,6 +81,7 @@ enum tis_defaults {
>
> enum tpm_tis_flags {
> 	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
>+	TPM_TIS_CLK_ENABLE		= BIT(1),
> };
>
> struct tpm_tis_data {
>diff --git a/include/linux/tpm.h b/include/linux/tpm.h index
>5a090f5ab335..881312d85574 100644
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -50,6 +50,7 @@ struct tpm_class_ops {
> 				unsigned long *timeout_cap);
> 	int (*request_locality)(struct tpm_chip *chip, int loc);
> 	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
>+	void (*clk_enable)(struct tpm_chip *chip, bool value);
> };
>
> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
>--
>1.9.1


  reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-21 22:39 [PATCH v4 0/2] ] Fix corner cases with disabling CLKRUN in tpm_tis Azhar Shaikh
2017-11-21 22:39 ` [PATCH v4 1/2] tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd() Azhar Shaikh
2017-11-22 19:57   ` Shaikh, Azhar [this message]
2017-11-21 22:39 ` [PATCH v4 2/2] tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy Azhar Shaikh

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=5FFFAD06ADE1CA4381B3F0F7C6AF582896F1F2@ORSMSX109.amr.corp.intel.com \
    --to=azhar.shaikh@intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jgunthorpe@obsidianresearch.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=peterhuewe@gmx.de \
    --cc=tpmdd-devel@lists.sourceforge.net \
    /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

tpmdd-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/tpmdd-devel/0 tpmdd-devel/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 tpmdd-devel tpmdd-devel/ https://lore.kernel.org/tpmdd-devel \
		tpmdd-devel@lists.sourceforge.net tpmdd-devel@archiver.kernel.org
	public-inbox-index tpmdd-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/net.sourceforge.lists.tpmdd-devel


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