linux-kernel - [PATCH 3/4] tpm: Fix test for interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 22 Feb 2021 01:56:33 +0100
From:   Lino Sanfilippo <LinoSanfilippo@....de>
To:     peterhuewe@....de, jarkko@...nel.org, jgg@...pe.ca
Cc:     stefanb@...ux.vnet.ibm.com, James.Bottomley@...senpartnership.com,
        jsnitsel@...hat.com, linux-integrity@...r.kernel.org,
        linux-kernel@...r.kernel.org, LinoSanfilippo@....de
Subject: [PATCH 3/4] tpm: Fix test for interrupts

The current test for functional interrupts is broken in multiple ways:
1. The needed flag TPM_CHIP_FLAG_IRQ is never set, so the test is never
executed.
2. The test includes the check for two variables and the check is done for
each transmission which is unnecessarily complicated.
3. Part of the test is setting a bool variable in an interrupt handler and
then check the value in the main thread. However there is nothing that
guarantees the visibility of the value set in the interrupt handler for
any other thread. Some kind of synchronization primitive is required for
this purpose.

Fix all these issues by a reimplementation:
Instead of doing the test in tpm_tis_send() which is called for each
transmission do it only once in tpm_tis_probe_irq_single(). Furthermore
use proper accessor functions like get_bit()/set_bit() which include the
required synchronization primitives to guarantee visibility between the
interrupt handler and threads.
Finally remove one function which is no longer needed.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@....de>
---
 drivers/char/tpm/tpm_tis_core.c | 37 ++++++++++---------------------------
 drivers/char/tpm/tpm_tis_core.h |  1 -
 include/linux/tpm.h             |  2 +-
 3 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e2f8585..d98f8e8 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -464,7 +464,7 @@ static void disable_interrupts(struct tpm_chip *chip)
  * tpm.c can skip polling for the data to be available as the interrupt is
  * waited for here
  */
-static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
+static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc;
@@ -497,29 +497,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
 	return rc;
 }
 
-static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
-{
-	int rc, irq;
-	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
-
-	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
-		return tpm_tis_send_main(chip, buf, len);
-
-	/* Verify receipt of the expected IRQ */
-	irq = priv->irq;
-	priv->irq = 0;
-	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
-	rc = tpm_tis_send_main(chip, buf, len);
-	priv->irq = irq;
-	chip->flags |= TPM_CHIP_FLAG_IRQ;
-	if (!priv->irq_tested)
-		tpm_msleep(1);
-	if (!priv->irq_tested)
-		disable_interrupts(chip);
-	priv->irq_tested = true;
-	return rc;
-}
-
 struct tis_vendor_durations_override {
 	u32 did_vid;
 	struct tpm1_version version;
@@ -721,7 +698,8 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
 	if (interrupt == 0)
 		return IRQ_NONE;
 
-	priv->irq_tested = true;
+	set_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
+
 	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
 		wake_up_interruptible(&priv->read_queue);
 	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -801,7 +779,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	if (rc < 0)
 		return rc;
 
-	priv->irq_tested = false;
+	clear_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
 
 	/* Generate an interrupt by having the core call through to
 	 * tpm_tis_send
@@ -810,6 +788,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	if (rc < 0)
 		return rc;
 
+	tpm_msleep(1);
+
+	if (!test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags))
+		disable_interrupts(chip);
+
 	/* tpm_tis_send will either confirm the interrupt is working or it
 	 * will call disable_irq which undoes all of the above.
 	 */
@@ -1080,7 +1063,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		if (irq) {
 			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
 						 irq);
-			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+			if (!test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags)) {
 				dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
 
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 9b2d32a..dc5f92b 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -89,7 +89,6 @@ struct tpm_tis_data {
 	u16 manufacturer_id;
 	int locality;
 	int irq;
-	bool irq_tested;
 	unsigned int flags;
 	void __iomem *ilb_base_addr;
 	u16 clkrun_enabled;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 8f4ff39..3e77c9d 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -126,7 +126,7 @@ struct tpm_chip {
 	struct tpm_chip_seqops bin_log_seqops;
 	struct tpm_chip_seqops ascii_log_seqops;
 
-	unsigned int flags;
+	unsigned long flags;
 
 	int dev_num;		/* /dev/tpm# */
 	unsigned long is_open;	/* only one allowed */
-- 
2.7.4