| Message ID | 1521657828.6397.19.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> /** > + * 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 {
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
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 >
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
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
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
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
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 {
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(-)