[1/1] tpm: fix intermittent failure with self tests
diff mbox series

Message ID 1550706760-18689-2-git-send-email-tyhicks@canonical.com
State New
Headers show
Series
  • TPM on Dell XPS 13 stopped working after upgrade to 18.04 (LP: #1762672)
Related show

Commit Message

Tyler Hicks Feb. 20, 2019, 11:52 p.m. UTC
From: James Bottomley <James.Bottomley@HansenPartnership.com>

BugLink: https://bugs.launchpad.net/bugs/1762672

My Nuvoton 6xx in a Dell XPS-13 has been intermittently failing to work
(necessitating a reboot). The problem seems to be that the TPM gets into a
state where the partial self-test doesn't return TPM_RC_SUCCESS (meaning
all tests have run to completion), but instead returns TPM_RC_TESTING
(meaning some tests are still running in the background).  There are
various theories that resending the self-test command actually causes the
tests to restart and thus triggers more TPM_RC_TESTING returns until the
timeout is exceeded.

There are several issues here: firstly being we shouldn't slow down the
boot sequence waiting for the self test to complete once the TPM
backgrounds them.  It will actually make available all functions that have
passed and if it gets a failure return TPM_RC_FAILURE to every subsequent
command.  So the fix is to kick off self tests once and if they return
TPM_RC_TESTING log that as a backgrounded self test and continue on.  In
order to prevent other tpm users from seeing any TPM_RC_TESTING returns
(which it might if they send a command that needs a TPM subsystem which is
still under test), we loop in tpm_transmit_cmd until either a timeout or we
don't get a TPM_RC_TESTING return.

Finally, there have been observations of strange returns from a partial
test. One Nuvoton is occasionally returning TPM_RC_COMMAND_CODE, so treat
any unexpected return from a partial self test as an indication we need to
run a full self test.

[jarkko.sakkinen@linux.intel.com: cleaned up some klog messages and
 dropped tpm_transmit_check() helper function from James' original
 commit.]

Fixes: 2482b1bba5122 ("tpm: Trigger only missing TPM 2.0 self tests")
Cc: stable@vger.kernel.org
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>

(backported from commit 2be8ffed093b91536d52b5cd2c99b52f605c9ba6)
[tyhicks: Backport to Bionic:
 - Bionic is missing upstream commit 0b66f2a05a80 which modified the
   self test retry loop in tpm2_do_selftest() but the entirety of that
   loop is rewritten by this patch, anyways.]
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 drivers/char/tpm/tpm-interface.c | 16 ++++++++++--
 drivers/char/tpm/tpm.h           |  1 +
 drivers/char/tpm/tpm2-cmd.c      | 56 ++++++++++++----------------------------
 3 files changed, 31 insertions(+), 42 deletions(-)

Comments

Colin King Feb. 21, 2019, 8:20 a.m. UTC | #1
On 20/02/2019 23:52, Tyler Hicks wrote:
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1762672
> 
> My Nuvoton 6xx in a Dell XPS-13 has been intermittently failing to work
> (necessitating a reboot). The problem seems to be that the TPM gets into a
> state where the partial self-test doesn't return TPM_RC_SUCCESS (meaning
> all tests have run to completion), but instead returns TPM_RC_TESTING
> (meaning some tests are still running in the background).  There are
> various theories that resending the self-test command actually causes the
> tests to restart and thus triggers more TPM_RC_TESTING returns until the
> timeout is exceeded.
> 
> There are several issues here: firstly being we shouldn't slow down the
> boot sequence waiting for the self test to complete once the TPM
> backgrounds them.  It will actually make available all functions that have
> passed and if it gets a failure return TPM_RC_FAILURE to every subsequent
> command.  So the fix is to kick off self tests once and if they return
> TPM_RC_TESTING log that as a backgrounded self test and continue on.  In
> order to prevent other tpm users from seeing any TPM_RC_TESTING returns
> (which it might if they send a command that needs a TPM subsystem which is
> still under test), we loop in tpm_transmit_cmd until either a timeout or we
> don't get a TPM_RC_TESTING return.
> 
> Finally, there have been observations of strange returns from a partial
> test. One Nuvoton is occasionally returning TPM_RC_COMMAND_CODE, so treat
> any unexpected return from a partial self test as an indication we need to
> run a full self test.
> 
> [jarkko.sakkinen@linux.intel.com: cleaned up some klog messages and
>  dropped tpm_transmit_check() helper function from James' original
>  commit.]
> 
> Fixes: 2482b1bba5122 ("tpm: Trigger only missing TPM 2.0 self tests")
> Cc: stable@vger.kernel.org
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>
> 
> (backported from commit 2be8ffed093b91536d52b5cd2c99b52f605c9ba6)
> [tyhicks: Backport to Bionic:
>  - Bionic is missing upstream commit 0b66f2a05a80 which modified the
>    self test retry loop in tpm2_do_selftest() but the entirety of that
>    loop is rewritten by this patch, anyways.]
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  drivers/char/tpm/tpm-interface.c | 16 ++++++++++--
>  drivers/char/tpm/tpm.h           |  1 +
>  drivers/char/tpm/tpm2-cmd.c      | 56 ++++++++++++----------------------------
>  3 files changed, 31 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index dba5259def60..bfaafcb7abd2 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -564,6 +564,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  	ssize_t ret;
>  	const size_t save_size = min(space ? sizeof(save) : TPM_HEADER_SIZE,
>  				     bufsiz);
> +	/* the command code is where the return code will be */
> +	u32 cc = be32_to_cpu(header->return_code);
>  
>  	/*
>  	 * Subtlety here: if we have a space, the handles will be
> @@ -577,11 +579,21 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>  		if (ret < 0)
>  			break;
>  		rc = be32_to_cpu(header->return_code);
> -		if (rc != TPM2_RC_RETRY)
> +		if (rc != TPM2_RC_RETRY && rc != TPM2_RC_TESTING)
> +			break;
> +		/*
> +		 * return immediately if self test returns test
> +		 * still running to shorten boot time.
> +		 */
> +		if (rc == TPM2_RC_TESTING && cc == TPM2_CC_SELF_TEST)
>  			break;
>  		delay_msec *= 2;
>  		if (delay_msec > TPM2_DURATION_LONG) {
> -			dev_err(&chip->dev, "TPM is in retry loop\n");
> +			if (rc == TPM2_RC_RETRY)
> +				dev_err(&chip->dev, "in retry loop\n");
> +			else
> +				dev_err(&chip->dev,
> +					"self test is still running\n");
>  			break;
>  		}
>  		tpm_msleep(delay_msec);
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index a5ae42d5a5ab..235714c9351c 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -102,6 +102,7 @@ enum tpm2_return_codes {
>  	TPM2_RC_HASH		= 0x0083, /* RC_FMT1 */
>  	TPM2_RC_HANDLE		= 0x008B,
>  	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
> +	TPM2_RC_FAILURE		= 0x0101,
>  	TPM2_RC_DISABLED	= 0x0120,
>  	TPM2_RC_COMMAND_CODE    = 0x0143,
>  	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index f6be08483ae6..89a5397b18d2 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -31,10 +31,6 @@ struct tpm2_startup_in {
>  	__be16	startup_type;
>  } __packed;
>  
> -struct tpm2_self_test_in {
> -	u8	full_test;
> -} __packed;
> -
>  struct tpm2_get_tpm_pt_in {
>  	__be32	cap_id;
>  	__be32	property_id;
> @@ -60,7 +56,6 @@ struct tpm2_get_random_out {
>  
>  union tpm2_cmd_params {
>  	struct	tpm2_startup_in		startup_in;
> -	struct	tpm2_self_test_in	selftest_in;
>  	struct	tpm2_get_tpm_pt_in	get_tpm_pt_in;
>  	struct	tpm2_get_tpm_pt_out	get_tpm_pt_out;
>  	struct	tpm2_get_random_in	getrandom_in;
> @@ -827,16 +822,6 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
>  }
>  EXPORT_SYMBOL_GPL(tpm2_calc_ordinal_duration);
>  
> -#define TPM2_SELF_TEST_IN_SIZE \
> -	(sizeof(struct tpm_input_header) + \
> -	 sizeof(struct tpm2_self_test_in))
> -
> -static const struct tpm_input_header tpm2_selftest_header = {
> -	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
> -	.length = cpu_to_be32(TPM2_SELF_TEST_IN_SIZE),
> -	.ordinal = cpu_to_be32(TPM2_CC_SELF_TEST)
> -};
> -
>  /**
>   * tpm2_do_selftest() - ensure that all self tests have passed
>   *
> @@ -852,29 +837,24 @@ static const struct tpm_input_header tpm2_selftest_header = {
>   */
>  static int tpm2_do_selftest(struct tpm_chip *chip)
>  {
> +	struct tpm_buf buf;
> +	int full;
>  	int rc;
> -	unsigned int delay_msec = 20;
> -	long duration;
> -	struct tpm2_cmd cmd;
>  
> -	duration = jiffies_to_msecs(
> -		tpm2_calc_ordinal_duration(chip, TPM2_CC_SELF_TEST));
> -
> -	while (duration > 0) {
> -		cmd.header.in = tpm2_selftest_header;
> -		cmd.params.selftest_in.full_test = 0;
> -
> -		rc = tpm_transmit_cmd(chip, NULL, &cmd, TPM2_SELF_TEST_IN_SIZE,
> -				      0, 0, "continue selftest");
> -
> -		if (rc != TPM2_RC_TESTING)
> -			break;
> +	for (full = 0; full < 2; full++) {
> +		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_SELF_TEST);
> +		if (rc)
> +			return rc;
>  
> -		tpm_msleep(delay_msec);
> -		duration -= delay_msec;
> +		tpm_buf_append_u8(&buf, full);
> +		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
> +				      "attempting the self test");
> +		tpm_buf_destroy(&buf);
>  
> -		/* wait longer the next round */
> -		delay_msec *= 2;
> +		if (rc == TPM2_RC_TESTING)
> +			rc = TPM2_RC_SUCCESS;
> +		if (rc == TPM2_RC_INITIALIZE || rc == TPM2_RC_SUCCESS)
> +			return rc;
>  	}
>  
>  	return rc;
> @@ -1060,10 +1040,8 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>  		goto out;
>  
>  	rc = tpm2_do_selftest(chip);
> -	if (rc != 0 && rc != TPM2_RC_INITIALIZE) {
> -		dev_err(&chip->dev, "TPM self test failed\n");
> +	if (rc && rc != TPM2_RC_INITIALIZE)
>  		goto out;
> -	}
>  
>  	if (rc == TPM2_RC_INITIALIZE) {
>  		rc = tpm_startup(chip);
> @@ -1071,10 +1049,8 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>  			goto out;
>  
>  		rc = tpm2_do_selftest(chip);
> -		if (rc) {
> -			dev_err(&chip->dev, "TPM self test failed\n");
> +		if (rc)
>  			goto out;
> -		}
>  	}
>  
>  	rc = tpm2_get_pcr_allocation(chip);
> 

Looks good, stable fix too.

Acked-by: Colin Ian King <colin.king@canonical.com>

Patch
diff mbox series

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index dba5259def60..bfaafcb7abd2 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -564,6 +564,8 @@  ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	ssize_t ret;
 	const size_t save_size = min(space ? sizeof(save) : TPM_HEADER_SIZE,
 				     bufsiz);
+	/* the command code is where the return code will be */
+	u32 cc = be32_to_cpu(header->return_code);
 
 	/*
 	 * Subtlety here: if we have a space, the handles will be
@@ -577,11 +579,21 @@  ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		if (ret < 0)
 			break;
 		rc = be32_to_cpu(header->return_code);
-		if (rc != TPM2_RC_RETRY)
+		if (rc != TPM2_RC_RETRY && rc != TPM2_RC_TESTING)
+			break;
+		/*
+		 * return immediately if self test returns test
+		 * still running to shorten boot time.
+		 */
+		if (rc == TPM2_RC_TESTING && cc == TPM2_CC_SELF_TEST)
 			break;
 		delay_msec *= 2;
 		if (delay_msec > TPM2_DURATION_LONG) {
-			dev_err(&chip->dev, "TPM is in retry loop\n");
+			if (rc == TPM2_RC_RETRY)
+				dev_err(&chip->dev, "in retry loop\n");
+			else
+				dev_err(&chip->dev,
+					"self test is still running\n");
 			break;
 		}
 		tpm_msleep(delay_msec);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a5ae42d5a5ab..235714c9351c 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -102,6 +102,7 @@  enum tpm2_return_codes {
 	TPM2_RC_HASH		= 0x0083, /* RC_FMT1 */
 	TPM2_RC_HANDLE		= 0x008B,
 	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
+	TPM2_RC_FAILURE		= 0x0101,
 	TPM2_RC_DISABLED	= 0x0120,
 	TPM2_RC_COMMAND_CODE    = 0x0143,
 	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index f6be08483ae6..89a5397b18d2 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -31,10 +31,6 @@  struct tpm2_startup_in {
 	__be16	startup_type;
 } __packed;
 
-struct tpm2_self_test_in {
-	u8	full_test;
-} __packed;
-
 struct tpm2_get_tpm_pt_in {
 	__be32	cap_id;
 	__be32	property_id;
@@ -60,7 +56,6 @@  struct tpm2_get_random_out {
 
 union tpm2_cmd_params {
 	struct	tpm2_startup_in		startup_in;
-	struct	tpm2_self_test_in	selftest_in;
 	struct	tpm2_get_tpm_pt_in	get_tpm_pt_in;
 	struct	tpm2_get_tpm_pt_out	get_tpm_pt_out;
 	struct	tpm2_get_random_in	getrandom_in;
@@ -827,16 +822,6 @@  unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 }
 EXPORT_SYMBOL_GPL(tpm2_calc_ordinal_duration);
 
-#define TPM2_SELF_TEST_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_self_test_in))
-
-static const struct tpm_input_header tpm2_selftest_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_SELF_TEST_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_SELF_TEST)
-};
-
 /**
  * tpm2_do_selftest() - ensure that all self tests have passed
  *
@@ -852,29 +837,24 @@  static const struct tpm_input_header tpm2_selftest_header = {
  */
 static int tpm2_do_selftest(struct tpm_chip *chip)
 {
+	struct tpm_buf buf;
+	int full;
 	int rc;
-	unsigned int delay_msec = 20;
-	long duration;
-	struct tpm2_cmd cmd;
 
-	duration = jiffies_to_msecs(
-		tpm2_calc_ordinal_duration(chip, TPM2_CC_SELF_TEST));
-
-	while (duration > 0) {
-		cmd.header.in = tpm2_selftest_header;
-		cmd.params.selftest_in.full_test = 0;
-
-		rc = tpm_transmit_cmd(chip, NULL, &cmd, TPM2_SELF_TEST_IN_SIZE,
-				      0, 0, "continue selftest");
-
-		if (rc != TPM2_RC_TESTING)
-			break;
+	for (full = 0; full < 2; full++) {
+		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_SELF_TEST);
+		if (rc)
+			return rc;
 
-		tpm_msleep(delay_msec);
-		duration -= delay_msec;
+		tpm_buf_append_u8(&buf, full);
+		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
+				      "attempting the self test");
+		tpm_buf_destroy(&buf);
 
-		/* wait longer the next round */
-		delay_msec *= 2;
+		if (rc == TPM2_RC_TESTING)
+			rc = TPM2_RC_SUCCESS;
+		if (rc == TPM2_RC_INITIALIZE || rc == TPM2_RC_SUCCESS)
+			return rc;
 	}
 
 	return rc;
@@ -1060,10 +1040,8 @@  int tpm2_auto_startup(struct tpm_chip *chip)
 		goto out;
 
 	rc = tpm2_do_selftest(chip);
-	if (rc != 0 && rc != TPM2_RC_INITIALIZE) {
-		dev_err(&chip->dev, "TPM self test failed\n");
+	if (rc && rc != TPM2_RC_INITIALIZE)
 		goto out;
-	}
 
 	if (rc == TPM2_RC_INITIALIZE) {
 		rc = tpm_startup(chip);
@@ -1071,10 +1049,8 @@  int tpm2_auto_startup(struct tpm_chip *chip)
 			goto out;
 
 		rc = tpm2_do_selftest(chip);
-		if (rc) {
-			dev_err(&chip->dev, "TPM self test failed\n");
+		if (rc)
 			goto out;
-		}
 	}
 
 	rc = tpm2_get_pcr_allocation(chip);