[PATCH v2 0/2] retry handling and intermittent self test failure fix

[PATCH v2 0/2] retry handling and intermittent self test failure fix

[James Bottomley]

The original fix for the intermittent self test failure contained a bug
in the retry loop that resent the command with a mangled header.  This
was discovered when coding up the TPM2_RC_RETRY handling, so this two
patch series bases the intermittent self test failure fix on the
working TPM2_RC_RETRY handling code.  This time I actually created an
emulator that sends TPM2_RC_TESTING and verified it does work.

James

---

James Bottomley (2):
  tpm: add retry logic
  tpm: fix intermittent failure with self tests

 drivers/char/tpm/tpm-interface.c | 87 +++++++++++++++++++++++++++++++++-------
 drivers/char/tpm/tpm.h           |  2 +
 drivers/char/tpm/tpm2-cmd.c      | 54 ++++++++-----------------
 3 files changed, 90 insertions(+), 53 deletions(-)

-- 
2.12.3

[PATCH v2 1/2] tpm: add retry logic

[James Bottomley]

TPM2 can return TPM2_RC_RETRY to any command and when it does we get
unexpected failures inside the kernel that surprise users (this is
mostly observed in the trusted key handling code).  The UEFI 2.6 spec
has advice on how to handle this:

    The firmware SHALL not return TPM2_RC_RETRY prior to the completion
    of the call to ExitBootServices().

    Implementer's Note: the implementation of this function should check
    the return value in the TPM response and, if it is TPM2_RC_RETRY,
    resend the command. The implementation may abort if a sufficient
    number of retries has been done.

So we follow that advice in our tpm_transmit() code using
TPM2_DURATION_SHORT as the initial wait duration and
TPM2_DURATION_LONG as the maximum wait time.  This should fix all the
in-kernel use cases and also means that user space TSS implementations
don't have to have their own retry handling.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: stable@vger.kernel.org

---

v2: renamed tpm_transmit_internal() to tpm_try_transmit()
---
 drivers/char/tpm/tpm-interface.c | 75 ++++++++++++++++++++++++++++++++--------
 drivers/char/tpm/tpm.h           |  1 +
 2 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index ddf7d937c77c..9e9bb62ae6b8 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -398,21 +398,10 @@ static void tpm_relinquish_locality(struct tpm_chip *chip)
 	chip->locality = -1;
 }
 
-/**
- * tpm_transmit - Internal kernel interface to transmit TPM commands.
- *
- * @chip: TPM chip to use
- * @space: tpm space
- * @buf: TPM command buffer
- * @bufsiz: length of the TPM command buffer
- * @flags: tpm transmit flags - bitmap
- *
- * Return:
- *     0 when the operation is successful.
- *     A negative number for system errors (errno).
- */
-ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
-		     u8 *buf, size_t bufsiz, unsigned int flags)
+static ssize_t tpm_try_transmit(struct tpm_chip *chip,
+				struct tpm_space *space,
+				u8 *buf, size_t bufsiz,
+				unsigned int flags)
 {
 	struct tpm_output_header *header = (void *)buf;
 	int rc;
@@ -550,6 +539,62 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 }
 
 /**
+ * tpm_transmit - Internal kernel interface to transmit TPM commands.
+ *
+ * @chip: TPM chip to use
+ * @space: tpm space
+ * @buf: TPM command buffer
+ * @bufsiz: length of the TPM command buffer
+ * @flags: tpm transmit flags - bitmap
+ *
+ * A wrapper around tpm_try_transmit that handles TPM2_RC_RETRY
+ * returns from the TPM and retransmits the command after a delay up
+ * to a maximum wait of TPM2_DURATION_LONG.
+ *
+ * Note: TPM1 never returns TPM2_RC_RETRY so the retry logic is TPM2
+ * only
+ *
+ * Return:
+ *     the length of the return when the operation is successful.
+ *     A negative number for system errors (errno).
+ */
+ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
+		     u8 *buf, size_t bufsiz, unsigned int flags)
+{
+	struct tpm_output_header *header = (struct tpm_output_header *)buf;
+	/* space for header and handles */
+	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
+	unsigned int delay_msec = TPM2_DURATION_SHORT;
+	u32 rc = 0;
+	ssize_t ret;
+	const size_t save_size = min(space ? sizeof(save) : TPM_HEADER_SIZE,
+				     bufsiz);
+
+	/*
+	 * Subtlety here: if we have a space, the handles will be
+	 * transformed, so when we restore the header we also have to
+	 * restore the handles.
+	 */
+	memcpy(save, buf, save_size);
+
+	for (;;) {
+		ret = tpm_try_transmit(chip, space, buf, bufsiz, flags);
+		if (ret < 0)
+			break;
+		rc = be32_to_cpu(header->return_code);
+		if (rc != TPM2_RC_RETRY)
+			break;
+		delay_msec *= 2;
+		if (delay_msec > TPM2_DURATION_LONG) {
+			dev_err(&chip->dev, "TPM is in retry loop\n");
+			break;
+		}
+		tpm_msleep(delay_msec);
+		memcpy(buf, save, save_size);
+	}
+	return ret;
+}
+/**
  * tpm_transmit_cmd - send a tpm command to the device
  *    The function extracts tpm out header return code
  *
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index ab3861631d27..05967c1a1f32 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -112,6 +112,7 @@ enum tpm2_return_codes {
 	TPM2_RC_COMMAND_CODE    = 0x0143,
 	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
 	TPM2_RC_REFERENCE_H0	= 0x0910,
+	TPM2_RC_RETRY		= 0x0922,
 };
 
 enum tpm2_algorithms {
-- 
2.12.3

[PATCH v2 2/2] tpm: fix intermittent failure with self tests

[James Bottomley]

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>
Tested-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>

---

v2:

Take the original commit from the tpmdd tree (which didn't work
because of a failure to restore the header and handles) and integrate
the TPM2_RC_TESTING retry logic with TPM2_RC_RETRY handling.  Have
tested this version in the face of TPM2_RC_TESTING returns using an
emulator
---
 drivers/char/tpm/tpm-interface.c | 16 ++++++++++--
 drivers/char/tpm/tpm.h           |  1 +
 drivers/char/tpm/tpm2-cmd.c      | 54 ++++++++++++----------------------------
 3 files changed, 31 insertions(+), 40 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 9e9bb62ae6b8..62fee2c10013 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -569,6 +569,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
@@ -582,11 +584,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 05967c1a1f32..c542f877cea7 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -108,6 +108,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 47f612ded599..909150f6db7f 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -24,10 +24,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;
@@ -49,7 +45,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;
 };
@@ -864,16 +859,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
  *
@@ -889,27 +874,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 = 10;
-	long duration;
-	struct tpm2_cmd cmd;
 
-	duration = jiffies_to_msecs(
-		tpm2_calc_ordinal_duration(chip, TPM2_CC_SELF_TEST));
-
-	while (1) {
-		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");
+	for (full = 0; full < 2; full++) {
+		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_SELF_TEST);
+		if (rc)
+			return rc;
 
-		if (rc != TPM2_RC_TESTING || delay_msec >= duration)
-			break;
+		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 than before */
-		delay_msec *= 2;
-		tpm_msleep(delay_msec);
+		if (rc == TPM2_RC_TESTING)
+			rc = TPM2_RC_SUCCESS;
+		if (rc == TPM2_RC_INITIALIZE || rc == TPM2_RC_SUCCESS)
+			return rc;
 	}
 
 	return rc;
@@ -1095,10 +1077,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);
@@ -1106,10 +1086,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);
-- 
2.12.3

Re: [PATCH v2 1/2] tpm: add retry logic

[Jarkko Sakkinen]

On Wed, 2018-03-21 at 11:43 -0700, James Bottomley wrote:
> TPM2 can return TPM2_RC_RETRY to any command and when it does we get
> unexpected failures inside the kernel that surprise users (this is
> mostly observed in the trusted key handling code).  The UEFI 2.6 spec
> has advice on how to handle this:
> 
>     The firmware SHALL not return TPM2_RC_RETRY prior to the completion
>     of the call to ExitBootServices().
> 
>     Implementer's Note: the implementation of this function should check
>     the return value in the TPM response and, if it is TPM2_RC_RETRY,
>     resend the command. The implementation may abort if a sufficient
>     number of retries has been done.
> 
> So we follow that advice in our tpm_transmit() code using
> TPM2_DURATION_SHORT as the initial wait duration and
> TPM2_DURATION_LONG as the maximum wait time.  This should fix all the
> in-kernel use cases and also means that user space TSS implementations
> don't have to have their own retry handling.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v2 1/2] tpm: add retry logic

[Nayna Jain]

>   /**
> + * tpm_transmit - Internal kernel interface to transmit TPM commands.
> + *
> + * @chip: TPM chip to use
> + * @space: tpm space
> + * @buf: TPM command buffer
> + * @bufsiz: length of the TPM command buffer
> + * @flags: tpm transmit flags - bitmap
> + *
> + * A wrapper around tpm_try_transmit that handles TPM2_RC_RETRY
> + * returns from the TPM and retransmits the command after a delay up
> + * to a maximum wait of TPM2_DURATION_LONG.
> + *
> + * Note: TPM1 never returns TPM2_RC_RETRY so the retry logic is TPM2
> + * only
> + *
> + * Return:
> + *     the length of the return when the operation is successful.
> + *     A negative number for system errors (errno).
> + */
> +ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> +		     u8 *buf, size_t bufsiz, unsigned int flags)
> +{
> +	struct tpm_output_header *header = (struct tpm_output_header *)buf;
> +	/* space for header and handles */
> +	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
> +	unsigned int delay_msec = TPM2_DURATION_SHORT;
> +	u32 rc = 0;
> +	ssize_t ret;
> +	const size_t save_size = min(space ? sizeof(save) : TPM_HEADER_SIZE,
> +				     bufsiz);
> +
> +	/*
> +	 * Subtlety here: if we have a space, the handles will be
> +	 * transformed, so when we restore the header we also have to
> +	 * restore the handles.
> +	 */
> +	memcpy(save, buf, save_size);
> +
> +	for (;;) {
> +		ret = tpm_try_transmit(chip, space, buf, bufsiz, flags);
> +		if (ret < 0)
> +			break;
> +		rc = be32_to_cpu(header->return_code);
> +		if (rc != TPM2_RC_RETRY)
> +			break;
> +		delay_msec *= 2;

Was wondering if this increment could be moved after 
tpm_msleep(delay_msec) ?

Thanks & Regards,
    - Nayna

> +		if (delay_msec > TPM2_DURATION_LONG) {
> +			dev_err(&chip->dev, "TPM is in retry loop\n");
> +			break;
> +		}
> +		tpm_msleep(delay_msec);
> +		memcpy(buf, save, save_size);
> +	}
> +	return ret;
> +}
> +/**
>    * tpm_transmit_cmd - send a tpm command to the device
>    *    The function extracts tpm out header return code
>    *
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index ab3861631d27..05967c1a1f32 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -112,6 +112,7 @@ enum tpm2_return_codes {
>   	TPM2_RC_COMMAND_CODE    = 0x0143,
>   	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
>   	TPM2_RC_REFERENCE_H0	= 0x0910,
> +	TPM2_RC_RETRY		= 0x0922,
>   };
>
>   enum tpm2_algorithms {

Re: [PATCH v2 1/2] tpm: add retry logic

[James Bottomley]

On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
> 
> > 
> >   /**
> > + * tpm_transmit - Internal kernel interface to transmit TPM
> > commands.
> > + *
> > + * @chip: TPM chip to use
> > + * @space: tpm space
> > + * @buf: TPM command buffer
> > + * @bufsiz: length of the TPM command buffer
> > + * @flags: tpm transmit flags - bitmap
> > + *
> > + * A wrapper around tpm_try_transmit that handles TPM2_RC_RETRY
> > + * returns from the TPM and retransmits the command after a delay
> > up
> > + * to a maximum wait of TPM2_DURATION_LONG.
> > + *
> > + * Note: TPM1 never returns TPM2_RC_RETRY so the retry logic is
> > TPM2
> > + * only
> > + *
> > + * Return:
> > + *     the length of the return when the operation is successful.
> > + *     A negative number for system errors (errno).
> > + */
> > +ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space
> > *space,
> > +		     u8 *buf, size_t bufsiz, unsigned int flags)
> > +{
> > +	struct tpm_output_header *header = (struct
> > tpm_output_header *)buf;
> > +	/* space for header and handles */
> > +	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
> > +	unsigned int delay_msec = TPM2_DURATION_SHORT;
> > +	u32 rc = 0;
> > +	ssize_t ret;
> > +	const size_t save_size = min(space ? sizeof(save) :
> > TPM_HEADER_SIZE,
> > +				     bufsiz);
> > +
> > +	/*
> > +	 * Subtlety here: if we have a space, the handles will be
> > +	 * transformed, so when we restore the header we also have
> > to
> > +	 * restore the handles.
> > +	 */
> > +	memcpy(save, buf, save_size);
> > +
> > +	for (;;) {
> > +		ret = tpm_try_transmit(chip, space, buf, bufsiz,
> > flags);
> > +		if (ret < 0)
> > +			break;
> > +		rc = be32_to_cpu(header->return_code);
> > +		if (rc != TPM2_RC_RETRY)
> > +			break;
> > +		delay_msec *= 2;
> 
> Was wondering if this increment could be moved after 
> tpm_msleep(delay_msec) ?

I thought about that when I saw the logic in the original but what it
would do is double the maximum delay (which is already nearly double
TPM2_DURATION_LONG).  I figured doubling the initial timeout was fine
rather than trying to do complex logic to get the delay exact (and then
having to litter the file with comments explaining why).

James

Re: [PATCH v2 1/2] tpm: add retry logic

[Nayna Jain]

On 03/22/2018 10:01 PM, James Bottomley wrote:
> On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
>>>    /**
>>> + * tpm_transmit - Internal kernel interface to transmit TPM
>>> commands.
>>> + *
>>> + * @chip: TPM chip to use
>>> + * @space: tpm space
>>> + * @buf: TPM command buffer
>>> + * @bufsiz: length of the TPM command buffer
>>> + * @flags: tpm transmit flags - bitmap
>>> + *
>>> + * A wrapper around tpm_try_transmit that handles TPM2_RC_RETRY
>>> + * returns from the TPM and retransmits the command after a delay
>>> up
>>> + * to a maximum wait of TPM2_DURATION_LONG.
>>> + *
>>> + * Note: TPM1 never returns TPM2_RC_RETRY so the retry logic is
>>> TPM2
>>> + * only
>>> + *
>>> + * Return:
>>> + *     the length of the return when the operation is successful.
>>> + *     A negative number for system errors (errno).
>>> + */
>>> +ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space
>>> *space,
>>> +		     u8 *buf, size_t bufsiz, unsigned int flags)
>>> +{
>>> +	struct tpm_output_header *header = (struct
>>> tpm_output_header *)buf;
>>> +	/* space for header and handles */
>>> +	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
>>> +	unsigned int delay_msec = TPM2_DURATION_SHORT;
>>> +	u32 rc = 0;
>>> +	ssize_t ret;
>>> +	const size_t save_size = min(space ? sizeof(save) :
>>> TPM_HEADER_SIZE,
>>> +				     bufsiz);
>>> +
>>> +	/*
>>> +	 * Subtlety here: if we have a space, the handles will be
>>> +	 * transformed, so when we restore the header we also have
>>> to
>>> +	 * restore the handles.
>>> +	 */
>>> +	memcpy(save, buf, save_size);
>>> +
>>> +	for (;;) {
>>> +		ret = tpm_try_transmit(chip, space, buf, bufsiz,
>>> flags);
>>> +		if (ret < 0)
>>> +			break;
>>> +		rc = be32_to_cpu(header->return_code);
>>> +		if (rc != TPM2_RC_RETRY)
>>> +			break;
>>> +		delay_msec *= 2;
>> Was wondering if this increment could be moved after
>> tpm_msleep(delay_msec) ?
> I thought about that when I saw the logic in the original but what it
> would do is double the maximum delay (which is already nearly double
> TPM2_DURATION_LONG).  I figured doubling the initial timeout was fine
> rather than trying to do complex logic to get the delay exact (and then
> having to litter the file with comments explaining why).

Sorry James, I didn't understand exactly about what complex logic is 
involved. Can you please explain it more ?
My point was to just move "delay_msec *= 2"  after tpm_msleep() as shown 
below:

if (delay_msec > TPM2_DURATION_LONG) {
     dev_err(&chip->dev, "TPM is in retry loop\n");
     break;
}
tpm_msleep(delay_msec)
delay_msec *= 2;

And the first time sleep will use the already initialized value of 
delay_msec as:
unsigned int delay_msec = TPM2_DURATION_SHORT;

Thanks & Regards,
    - Nayna


>
> James
>

Re: [PATCH v2 1/2] tpm: add retry logic

[James Bottomley]

On Mon, 2018-03-26 at 19:41 +0530, Nayna Jain wrote:
> 
> On 03/22/2018 10:01 PM, James Bottomley wrote:
> > 
> > On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
[...]
> > > > +ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space
> > > > *space,
> > > > +		     u8 *buf, size_t bufsiz, unsigned int
> > > > flags)
> > > > +{
> > > > +	struct tpm_output_header *header = (struct
> > > > tpm_output_header *)buf;
> > > > +	/* space for header and handles */
> > > > +	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
> > > > +	unsigned int delay_msec = TPM2_DURATION_SHORT;
> > > > +	u32 rc = 0;
> > > > +	ssize_t ret;
> > > > +	const size_t save_size = min(space ? sizeof(save) :
> > > > TPM_HEADER_SIZE,
> > > > +				     bufsiz);
> > > > +
> > > > +	/*
> > > > +	 * Subtlety here: if we have a space, the handles will
> > > > be
> > > > +	 * transformed, so when we restore the header we also
> > > > have
> > > > to
> > > > +	 * restore the handles.
> > > > +	 */
> > > > +	memcpy(save, buf, save_size);
> > > > +
> > > > +	for (;;) {
> > > > +		ret = tpm_try_transmit(chip, space, buf,
> > > > bufsiz,
> > > > flags);
> > > > +		if (ret < 0)
> > > > +			break;
> > > > +		rc = be32_to_cpu(header->return_code);
> > > > +		if (rc != TPM2_RC_RETRY)
> > > > +			break;
> > > > +		delay_msec *= 2;
> > > Was wondering if this increment could be moved after
> > > tpm_msleep(delay_msec) ?
> > I thought about that when I saw the logic in the original but what
> > it would do is double the maximum delay (which is already nearly
> > double TPM2_DURATION_LONG).  I figured doubling the initial timeout
> > was fine rather than trying to do complex logic to get the delay
> > exact (and then having to litter the file with comments explaining
> > why).
> 
> Sorry James, I didn't understand exactly about what complex logic is 
> involved. Can you please explain it more ?
> My point was to just move "delay_msec *= 2"  after tpm_msleep() as
> shown below:
> 
> if (delay_msec > TPM2_DURATION_LONG) {
>      dev_err(&chip->dev, "TPM is in retry loop\n");
>      break;
> }
> tpm_msleep(delay_msec)
> delay_msec *= 2;

Yes, I understand the suggestion; however, if you simply do that,
you'll end up with a useless sleep at the end of the wait period before
you check to see if you've waited too long, which is even more
suboptimal than the current situation.  The cardinal rule is we should
only sleep if we know we're going to retry.

James

Re: [PATCH v2 1/2] tpm: add retry logic

[Mimi Zohar]

On Mon, 2018-03-26 at 07:28 -0700, James Bottomley wrote:
> On Mon, 2018-03-26 at 19:41 +0530, Nayna Jain wrote:
> > 
> > On 03/22/2018 10:01 PM, James Bottomley wrote:
> > > 
> > > On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
> [...]
> > > > > +ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space
> > > > > *space,
> > > > > +		     u8 *buf, size_t bufsiz, unsigned int
> > > > > flags)
> > > > > +{
> > > > > +	struct tpm_output_header *header = (struct
> > > > > tpm_output_header *)buf;
> > > > > +	/* space for header and handles */
> > > > > +	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
> > > > > +	unsigned int delay_msec = TPM2_DURATION_SHORT;
> > > > > +	u32 rc = 0;
> > > > > +	ssize_t ret;
> > > > > +	const size_t save_size = min(space ? sizeof(save) :
> > > > > TPM_HEADER_SIZE,
> > > > > +				     bufsiz);
> > > > > +
> > > > > +	/*
> > > > > +	 * Subtlety here: if we have a space, the handles will
> > > > > be
> > > > > +	 * transformed, so when we restore the header we also
> > > > > have
> > > > > to
> > > > > +	 * restore the handles.
> > > > > +	 */
> > > > > +	memcpy(save, buf, save_size);
> > > > > +
> > > > > +	for (;;) {
> > > > > +		ret = tpm_try_transmit(chip, space, buf,
> > > > > bufsiz,
> > > > > flags);
> > > > > +		if (ret < 0)
> > > > > +			break;
> > > > > +		rc = be32_to_cpu(header->return_code);
> > > > > +		if (rc != TPM2_RC_RETRY)
> > > > > +			break;
> > > > > +		delay_msec *= 2;
> > > > Was wondering if this increment could be moved after
> > > > tpm_msleep(delay_msec) ?
> > > I thought about that when I saw the logic in the original but what
> > > it would do is double the maximum delay (which is already nearly
> > > double TPM2_DURATION_LONG).  I figured doubling the initial timeout
> > > was fine rather than trying to do complex logic to get the delay
> > > exact (and then having to litter the file with comments explaining
> > > why).
> > 
> > Sorry James, I didn't understand exactly about what complex logic is 
> > involved. Can you please explain it more ?
> > My point was to just move "delay_msec *= 2"  after tpm_msleep() as
> > shown below:
> > 
> > if (delay_msec > TPM2_DURATION_LONG) {
> >      dev_err(&chip->dev, "TPM is in retry loop\n");
> >      break;
> > }
> > tpm_msleep(delay_msec)
> > delay_msec *= 2;
> 
> Yes, I understand the suggestion; however, if you simply do that,
> you'll end up with a useless sleep at the end of the wait period before
> you check to see if you've waited too long, which is even more
> suboptimal than the current situation.  The cardinal rule is we should
> only sleep if we know we're going to retry.

By the time delay_msec is incremented here, you've already finished
sleeping and will resend the command.  This is the reason I assume you
didn't use a normal for loop:

    for (delay_msec = TPM2_DURATION_SHORT; delay_msec <TPM2_DURATION_LONG;
         delay_msec *= 2)

Incrementing the sleep here will only affect the next sleep.

Mimi

Re: [PATCH v2 1/2] tpm: add retry logic

[James Bottomley]

On Mon, 2018-03-26 at 12:14 -0400, Mimi Zohar wrote:
> On Mon, 2018-03-26 at 07:28 -0700, James Bottomley wrote:
> > 
> > On Mon, 2018-03-26 at 19:41 +0530, Nayna Jain wrote:
> > > 
> > > 
> > > On 03/22/2018 10:01 PM, James Bottomley wrote:
> > > > 
> > > > 
> > > > On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
> > [...]
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > +ssize_t tpm_transmit(struct tpm_chip *chip, struct
> > > > > > tpm_space
> > > > > > *space,
> > > > > > +		     u8 *buf, size_t bufsiz, unsigned int
> > > > > > flags)
> > > > > > +{
> > > > > > +	struct tpm_output_header *header = (struct
> > > > > > tpm_output_header *)buf;
> > > > > > +	/* space for header and handles */
> > > > > > +	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
> > > > > > +	unsigned int delay_msec = TPM2_DURATION_SHORT;
> > > > > > +	u32 rc = 0;
> > > > > > +	ssize_t ret;
> > > > > > +	const size_t save_size = min(space ? sizeof(save)
> > > > > > :
> > > > > > TPM_HEADER_SIZE,
> > > > > > +				     bufsiz);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Subtlety here: if we have a space, the handles
> > > > > > will
> > > > > > be
> > > > > > +	 * transformed, so when we restore the header we
> > > > > > also
> > > > > > have
> > > > > > to
> > > > > > +	 * restore the handles.
> > > > > > +	 */
> > > > > > +	memcpy(save, buf, save_size);
> > > > > > +
> > > > > > +	for (;;) {
> > > > > > +		ret = tpm_try_transmit(chip, space, buf,
> > > > > > bufsiz,
> > > > > > flags);
> > > > > > +		if (ret < 0)
> > > > > > +			break;
> > > > > > +		rc = be32_to_cpu(header->return_code);
> > > > > > +		if (rc != TPM2_RC_RETRY)
> > > > > > +			break;
> > > > > > +		delay_msec *= 2;
> > > > > Was wondering if this increment could be moved after
> > > > > tpm_msleep(delay_msec) ?
> > > > I thought about that when I saw the logic in the original but
> > > > what
> > > > it would do is double the maximum delay (which is already
> > > > nearly
> > > > double TPM2_DURATION_LONG).  I figured doubling the initial
> > > > timeout
> > > > was fine rather than trying to do complex logic to get the
> > > > delay
> > > > exact (and then having to litter the file with comments
> > > > explaining
> > > > why).
> > > 
> > > Sorry James, I didn't understand exactly about what complex logic
> > > is 
> > > involved. Can you please explain it more ?
> > > My point was to just move "delay_msec *= 2"  after tpm_msleep()
> > > as
> > > shown below:
> > > 
> > > if (delay_msec > TPM2_DURATION_LONG) {
> > >      dev_err(&chip->dev, "TPM is in retry loop\n");
> > >      break;
> > > }
> > > tpm_msleep(delay_msec)
> > > delay_msec *= 2;
> > 
> > Yes, I understand the suggestion; however, if you simply do that,
> > you'll end up with a useless sleep at the end of the wait period
> > before
> > you check to see if you've waited too long, which is even more
> > suboptimal than the current situation.  The cardinal rule is we
> > should
> > only sleep if we know we're going to retry.
> 
> By the time delay_msec is incremented here, you've already finished
> sleeping and will resend the command.  This is the reason I assume
> you
> didn't use a normal for loop:
> 
>     for (delay_msec = TPM2_DURATION_SHORT; delay_msec
> <TPM2_DURATION_LONG;
>          delay_msec *= 2)
> 
> Incrementing the sleep here will only affect the next sleep.

Yes, looking at it again, that seems to be true.  I remember thinking
the same of the original code when I looked at it, but saw there was
some problem.  However, it's so long ago, I can't remember what it
actually was and I can't see it now.

James

Re: [PATCH v2 1/2] tpm: add retry logic

[Mimi Zohar]

On Tue, 2018-03-27 at 08:39 -0700, James Bottomley wrote:
> On Mon, 2018-03-26 at 12:14 -0400, Mimi Zohar wrote:
> > On Mon, 2018-03-26 at 07:28 -0700, James Bottomley wrote:
> > > 
> > > On Mon, 2018-03-26 at 19:41 +0530, Nayna Jain wrote:
> > > > 
> > > > 
> > > > On 03/22/2018 10:01 PM, James Bottomley wrote:
> > > > > 
> > > > > 
> > > > > On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
> > > [...]
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > +ssize_t tpm_transmit(struct tpm_chip *chip, struct
> > > > > > > tpm_space
> > > > > > > *space,
> > > > > > > +		     u8 *buf, size_t bufsiz, unsigned int
> > > > > > > flags)
> > > > > > > +{
> > > > > > > +	struct tpm_output_header *header = (struct
> > > > > > > tpm_output_header *)buf;
> > > > > > > +	/* space for header and handles */
> > > > > > > +	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
> > > > > > > +	unsigned int delay_msec = TPM2_DURATION_SHORT;
> > > > > > > +	u32 rc = 0;
> > > > > > > +	ssize_t ret;
> > > > > > > +	const size_t save_size = min(space ? sizeof(save)
> > > > > > > :
> > > > > > > TPM_HEADER_SIZE,
> > > > > > > +				     bufsiz);
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * Subtlety here: if we have a space, the handles
> > > > > > > will
> > > > > > > be
> > > > > > > +	 * transformed, so when we restore the header we
> > > > > > > also
> > > > > > > have
> > > > > > > to
> > > > > > > +	 * restore the handles.
> > > > > > > +	 */
> > > > > > > +	memcpy(save, buf, save_size);
> > > > > > > +
> > > > > > > +	for (;;) {
> > > > > > > +		ret = tpm_try_transmit(chip, space, buf,
> > > > > > > bufsiz,
> > > > > > > flags);
> > > > > > > +		if (ret < 0)
> > > > > > > +			break;
> > > > > > > +		rc = be32_to_cpu(header->return_code);
> > > > > > > +		if (rc != TPM2_RC_RETRY)
> > > > > > > +			break;
> > > > > > > +		delay_msec *= 2;
> > > > > > Was wondering if this increment could be moved after
> > > > > > tpm_msleep(delay_msec) ?
> > > > > I thought about that when I saw the logic in the original but
> > > > > what
> > > > > it would do is double the maximum delay (which is already
> > > > > nearly
> > > > > double TPM2_DURATION_LONG).  I figured doubling the initial
> > > > > timeout
> > > > > was fine rather than trying to do complex logic to get the
> > > > > delay
> > > > > exact (and then having to litter the file with comments
> > > > > explaining
> > > > > why).
> > > > 
> > > > Sorry James, I didn't understand exactly about what complex logic
> > > > is 
> > > > involved. Can you please explain it more ?
> > > > My point was to just move "delay_msec *= 2"  after tpm_msleep()
> > > > as
> > > > shown below:
> > > > 
> > > > if (delay_msec > TPM2_DURATION_LONG) {
> > > >      dev_err(&chip->dev, "TPM is in retry loop\n");
> > > >      break;
> > > > }
> > > > tpm_msleep(delay_msec)
> > > > delay_msec *= 2;
> > > 
> > > Yes, I understand the suggestion; however, if you simply do that,
> > > you'll end up with a useless sleep at the end of the wait period
> > > before
> > > you check to see if you've waited too long, which is even more
> > > suboptimal than the current situation.  The cardinal rule is we
> > > should
> > > only sleep if we know we're going to retry.
> > 
> > By the time delay_msec is incremented here, you've already finished
> > sleeping and will resend the command.  This is the reason I assume
> > you
> > didn't use a normal for loop:
> > 
> >     for (delay_msec = TPM2_DURATION_SHORT; delay_msec
> > <TPM2_DURATION_LONG;
> >          delay_msec *= 2)
> > 
> > Incrementing the sleep here will only affect the next sleep.
> 
> Yes, looking at it again, that seems to be true.  I remember thinking
> the same of the original code when I looked at it, but saw there was
> some problem.  However, it's so long ago, I can't remember what it
> actually was and I can't see it now.

As this patch is already in the security's next-testing branch, it's
too late to change it.  I assume Nayna should post a patch that moves
it.

Mimi