Should we handle TPM_RC_RETRY internally?

Should we handle TPM_RC_RETRY internally?

[James Bottomley]

I was investigating an apparent bug in the trusted keys implementation
where periodically the key operation barfs and returns an error to
userspace.  It turns out this error is because the TPM returns
TPM_RC_RETRY to an operation.

The TPM spec is a bit unclear why the TPM would return TPM_RC_RETRY,
but it is clear that it may happen on a lot of operations.  I checked
with the microsoft reference implementation:

https://github.com/Microsoft/ms-tpm-20-ref/

Which implies it's only set if the lockout check is invoked by the
command and the previous TPM shutdown wasn't orderly.  It does seem to
me that I've only seen it involving objects with DA implications, which
explains why it's seen in trusted keys.

If I read the UEFI TPM API, it does automatic retries.  This is the
note:

    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.

I really think if UEFI does it, we should do it too (and it will fix my
trusted key bug).

What does everyone else think?  If it's agreed, I'll code up the patch.

James

Re: Should we handle TPM_RC_RETRY internally?

[Javier Martinez Canillas]

[adding Philip Tricca to the cc list]

Hello James,

On 03/15/2018 07:02 PM, James Bottomley wrote:
> I was investigating an apparent bug in the trusted keys implementation
> where periodically the key operation barfs and returns an error to
> userspace.  It turns out this error is because the TPM returns
> TPM_RC_RETRY to an operation.
> 
> The TPM spec is a bit unclear why the TPM would return TPM_RC_RETRY,
> but it is clear that it may happen on a lot of operations.  I checked
> with the microsoft reference implementation:
> 
> https://github.com/Microsoft/ms-tpm-20-ref/
> 
> Which implies it's only set if the lockout check is invoked by the
> command and the previous TPM shutdown wasn't orderly.  It does seem to
> me that I've only seen it involving objects with DA implications, which
> explains why it's seen in trusted keys.
> 
> If I read the UEFI TPM API, it does automatic retries.  This is the
> note:
> 
>     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.
> 
> I really think if UEFI does it, we should do it too (and it will fix my
> trusted key bug).
> 
> What does everyone else think?  If it's agreed, I'll code up the patch.
> 

That's a very good question. I don't know the answer but maybe Philip does
since he said that would ask the TCG TSS WG members for their thoughts on
this [0].

We had the same issue in the tpm2-software project, and at the end Philip
added a macro that retries sending commands on TPM_RC_RETRY responses [1].

If the kernel handles this, then such a macro won't be needed anymore for
user-space programs and will simplify things. So I'm in favour to handle
at the driver level but I'm not sure if is the correct thing to do or not.

> James
> 

[0]: https://github.com/tpm2-software/tpm2-tools/issues/469#issuecomment-340501128
[1]: https://github.com/tpm2-software/tpm2-tools/pull/585/commits/9b0f3e59bab0810cf3721bf6765bd083f23f811f

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: Should we handle TPM_RC_RETRY internally?

[Jarkko Sakkinen]

On Fri, Mar 16, 2018 at 11:36:28AM +0100, Javier Martinez Canillas wrote:
> [adding Philip Tricca to the cc list]
> 
> Hello James,
> 
> On 03/15/2018 07:02 PM, James Bottomley wrote:
> > I was investigating an apparent bug in the trusted keys implementation
> > where periodically the key operation barfs and returns an error to
> > userspace.  It turns out this error is because the TPM returns
> > TPM_RC_RETRY to an operation.
> > 
> > The TPM spec is a bit unclear why the TPM would return TPM_RC_RETRY,
> > but it is clear that it may happen on a lot of operations.  I checked
> > with the microsoft reference implementation:
> > 
> > https://github.com/Microsoft/ms-tpm-20-ref/
> > 
> > Which implies it's only set if the lockout check is invoked by the
> > command and the previous TPM shutdown wasn't orderly.  It does seem to
> > me that I've only seen it involving objects with DA implications, which
> > explains why it's seen in trusted keys.
> > 
> > If I read the UEFI TPM API, it does automatic retries.  This is the
> > note:
> > 
> >     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.
> > 
> > I really think if UEFI does it, we should do it too (and it will fix my
> > trusted key bug).
> > 
> > What does everyone else think?  If it's agreed, I'll code up the patch.
> > 
> 
> That's a very good question. I don't know the answer but maybe Philip does
> since he said that would ask the TCG TSS WG members for their thoughts on
> this [0].
> 
> We had the same issue in the tpm2-software project, and at the end Philip
> added a macro that retries sending commands on TPM_RC_RETRY responses [1].
> 
> If the kernel handles this, then such a macro won't be needed anymore for
> user-space programs and will simplify things. So I'm in favour to handle
> at the driver level but I'm not sure if is the correct thing to do or not.

Can we get input from TCG how to fix this properly and request to
update their specs accordingly?

/Jarkko

Re: Should we handle TPM_RC_RETRY internally?

[Philip Tricca]

On Fri, Mar 16, 2018 at 11:36:28AM +0100, Javier Martinez Canillas wrote:
> [adding Philip Tricca to the cc list]
> 
> Hello James,
> 
> On 03/15/2018 07:02 PM, James Bottomley wrote:
> > I was investigating an apparent bug in the trusted keys implementation
> > where periodically the key operation barfs and returns an error to
> > userspace.  It turns out this error is because the TPM returns
> > TPM_RC_RETRY to an operation.
> > 
> > The TPM spec is a bit unclear why the TPM would return TPM_RC_RETRY,
> > but it is clear that it may happen on a lot of operations.  I checked
> > with the microsoft reference implementation:
> > 
> > https://github.com/Microsoft/ms-tpm-20-ref/
> > 
> > Which implies it's only set if the lockout check is invoked by the
> > command and the previous TPM shutdown wasn't orderly.  It does seem to
> > me that I've only seen it involving objects with DA implications, which
> > explains why it's seen in trusted keys.
> > 
> > If I read the UEFI TPM API, it does automatic retries.  This is the
> > note:
> > 
> >     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.
> > 
> > I really think if UEFI does it, we should do it too (and it will fix my
> > trusted key bug).
> > 
> > What does everyone else think?  If it's agreed, I'll code up the patch.
> > 
> 
> That's a very good question. I don't know the answer but maybe Philip does
> since he said that would ask the TCG TSS WG members for their thoughts on
> this [0].
> 
> We had the same issue in the tpm2-software project, and at the end Philip
> added a macro that retries sending commands on TPM_RC_RETRY responses [1].

We've been using this macro without indicent for some time in
applications using the tss2-sys library. If memory serves I think the
new tss2-esys library handles this internally so from the perspective of
the user space I'd written this issue off as solved.

> If the kernel handles this, then such a macro won't be needed anymore for
> user-space programs and will simplify things. So I'm in favour to handle
> at the driver level but I'm not sure if is the correct thing to do or not.

I can't say for sure but I'd imagine the implications of implementing a
retry loop in the kernel are different than user space. We retry
indefinitely but I would think having some bounded number of retries in
the kernel would be prudent.

Philip

Re: Should we handle TPM_RC_RETRY internally?

[Jarkko Sakkinen]

On Sun, Mar 18, 2018 at 09:40:56PM -0700, Philip Tricca wrote:
> I can't say for sure but I'd imagine the implications of implementing a
> retry loop in the kernel are different than user space. We retry
> indefinitely but I would think having some bounded number of retries in
> the kernel would be prudent.

Yes, even if we would support handlig RC_RETRY we could not make promise
to the user space that this error code will never fall through.

/Jarkko

Re: Should we handle TPM_RC_RETRY internally?

[Jarkko Sakkinen]

On Thu, Mar 15, 2018 at 11:02:11AM -0700, James Bottomley wrote:
> I was investigating an apparent bug in the trusted keys implementation
> where periodically the key operation barfs and returns an error to
> userspace.  It turns out this error is because the TPM returns
> TPM_RC_RETRY to an operation.
> 
> The TPM spec is a bit unclear why the TPM would return TPM_RC_RETRY,
> but it is clear that it may happen on a lot of operations.  I checked
> with the microsoft reference implementation:
> 
> https://github.com/Microsoft/ms-tpm-20-ref/
> 
> Which implies it's only set if the lockout check is invoked by the
> command and the previous TPM shutdown wasn't orderly.  It does seem to
> me that I've only seen it involving objects with DA implications, which
> explains why it's seen in trusted keys.
> 
> If I read the UEFI TPM API, it does automatic retries.  This is the
> note:
> 
>     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.
> 
> I really think if UEFI does it, we should do it too (and it will fix my
> trusted key bug).
> 
> What does everyone else think?  If it's agreed, I'll code up the patch.
> 
> James

I think I agree but what worries me is that this error code is almost not
documented at all in TCG specifications.

/Jarkko

Re: Should we handle TPM_RC_RETRY internally?

[James Bottomley]

On Fri, 2018-03-16 at 16:29 +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 15, 2018 at 11:02:11AM -0700, James Bottomley wrote:
> > 
> > I was investigating an apparent bug in the trusted keys
> > implementation where periodically the key operation barfs and
> > returns an error to userspace.  It turns out this error is because
> > the TPM returns TPM_RC_RETRY to an operation.
> > 
> > The TPM spec is a bit unclear why the TPM would return
> > TPM_RC_RETRY, but it is clear that it may happen on a lot of
> > operations.  I checked with the microsoft reference implementation:
> > 
> > https://github.com/Microsoft/ms-tpm-20-ref/
> > 
> > Which implies it's only set if the lockout check is invoked by the
> > command and the previous TPM shutdown wasn't orderly.  It does seem
> > to me that I've only seen it involving objects with DA
> > implications, which explains why it's seen in trusted keys.
> > 
> > If I read the UEFI TPM API, it does automatic retries.  This is the
> > note:
> > 
> >     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.
> > 
> > I really think if UEFI does it, we should do it too (and it will
> > fix my trusted key bug).
> > 
> > What does everyone else think?  If it's agreed, I'll code up the
> > patch.
> > 
> > James
> 
> I think I agree but what worries me is that this error code is almost
> not documented at all in TCG specifications.

Yes, it's cryptic, it just says "the TPM was not able to start the
command", which is why I checked the reference implementation.  I think
the UEFI ref is the clearest because it provides implementation
guidance (basically to retry the command until timeout) which I see no
reason we can't follow in the kernel.

The alternative is that we handle it in the TSS and I have to patch
trusted keys.  However none of the core TPM routines that use
tpm_transmit_cmd() have any retry logic, so if we find a TPM that
returns it to a kernel sent command we'll get random unexplained
failures (based on the ref implementation I don't think this is likely,
but by the spec it's possible, especially for a manufacturer who isn't
using the reference), so we may end up having to add retry logic to
every place in the kernel where we use tpm_transmit_cmd() ... then
we'll wish we'd done it generically inside that routine.

Based on this, I think the UEFI spec gives us the safety of knowing
that internal handling is correct and the advantages of doing so now
are that we don't get a potentially exploding problem later, so it
seems like a net win.  I'll code up a patch.

James

Re: Should we handle TPM_RC_RETRY internally?

[Jarkko Sakkinen]

On Fri, Mar 16, 2018 at 08:48:24AM -0700, James Bottomley wrote:
> On Fri, 2018-03-16 at 16:29 +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 15, 2018 at 11:02:11AM -0700, James Bottomley wrote:
> > > 
> > > I was investigating an apparent bug in the trusted keys
> > > implementation where periodically the key operation barfs and
> > > returns an error to userspace.  It turns out this error is because
> > > the TPM returns TPM_RC_RETRY to an operation.
> > > 
> > > The TPM spec is a bit unclear why the TPM would return
> > > TPM_RC_RETRY, but it is clear that it may happen on a lot of
> > > operations.  I checked with the microsoft reference implementation:
> > > 
> > > https://github.com/Microsoft/ms-tpm-20-ref/
> > > 
> > > Which implies it's only set if the lockout check is invoked by the
> > > command and the previous TPM shutdown wasn't orderly.  It does seem
> > > to me that I've only seen it involving objects with DA
> > > implications, which explains why it's seen in trusted keys.
> > > 
> > > If I read the UEFI TPM API, it does automatic retries.  This is the
> > > note:
> > > 
> > >     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.
> > > 
> > > I really think if UEFI does it, we should do it too (and it will
> > > fix my trusted key bug).
> > > 
> > > What does everyone else think?  If it's agreed, I'll code up the
> > > patch.
> > > 
> > > James
> > 
> > I think I agree but what worries me is that this error code is almost
> > not documented at all in TCG specifications.
> 
> Yes, it's cryptic, it just says "the TPM was not able to start the
> command", which is why I checked the reference implementation.  I think
> the UEFI ref is the clearest because it provides implementation
> guidance (basically to retry the command until timeout) which I see no
> reason we can't follow in the kernel.

I agree with your reasoning.

/Jarkko

Re: Should we handle TPM_RC_RETRY internally?

[Jarkko Sakkinen]

On Sat, Mar 17, 2018 at 01:21:08AM +0000, Andrey Pronin wrote:
>    If the TSS starts retrying commands itself, then it's more than just
>    handling TPM_RC_RETRY.
>    There is also even more cryptic TPM_RC_YIELDED, which allows (suggests?)
>    retrying, and TPM_RC_NV_RATE, which covers one of the few legitimate and
>    easily explainable reasons why a command can fail now but succeed later.
>    And what about TPM_RC_MEMORY, TPM_RC_NV_UNAVAILABLE and such?
>    Though quite exotic, they are all warnings, and all indicate a possibly
>    temporary lack of access/resources. Unlike TPM_RC_*_HANDLES, CONTEXT_GAP
>    etc, they don't require RM to do anything with objects and handles to
>    recover, and may succeed if simply retried after a delay.
>    Shall these retries be also handled in tpm_transmit_cmd() or left up to
>    the caller to take care of? And how are they different from TPM_RC_RETRY
>    case, if the latter?
>    If you are interested in the actual in-kernel use of the commands that
>    can face such codes, look no further than tpm2_load_context().
>    The referenceA implementation may return TPM_RC_NV* fromA TPM2_ContextLoad
>    since it callsA NvIsAvailable().
>    --
>    Andrey

Hey, thanks for this feedback. It changed my point of view.

tpm_transmit_cmd() should be able to handle exactly the subset that in-kernel
clients of the TPM need, not more or less.

I do no want TPM_RC_RETRY logic at this point to tpm_transmit() path. We can
consider that later (maybe). It is at least too late to consider it in 4.17
timeline.

/Jarkko

Re: Should we handle TPM_RC_RETRY internally?

[James Bottomley]

Your original email is missing from the list because it has a html
part, which vger takes as spam.  You have to send text/plain only to
get email on the list

On Sat, 2018-03-17 at 01:21 +0000, Andrey Pronin wrote:
> On Fri, Mar 16, 2018 at 8:48 AM James Bottomley <
> James.Bottomley@hansenpartnership.com> wrote:
> > 
> > 
> > On Fri, 2018-03-16 at 16:29 +0200, Jarkko Sakkinen wrote:
> > > 
> > > On Thu, Mar 15, 2018 at 11:02:11AM -0700, James Bottomley wrote:
> > > > 
> > > > 
> > > > I was investigating an apparent bug in the trusted keys
> > > > implementation where periodically the key operation barfs and
> > > > returns an error to userspace.  It turns out this error is
> > > > because the TPM returns TPM_RC_RETRY to an operation.
> > > > 
> > > > The TPM spec is a bit unclear why the TPM would return
> > > > TPM_RC_RETRY, but it is clear that it may happen on a lot of
> > > > operations.  I checked with the microsoft reference
> > > > implementation:
> > > > 
> > > > https://github.com/Microsoft/ms-tpm-20-ref/
> > > > 
> > > > Which implies it's only set if the lockout check is invoked by
> > > > the command and the previous TPM shutdown wasn't orderly.  It
> > > > does seem to me that I've only seen it involving objects with
> > > > DA implications, which explains why it's seen in trusted keys.
> > > > 
> > > > If I read the UEFI TPM API, it does automatic retries.  This is
> > > > the note:
> > > > 
> > > >     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.
> > > > 
> > > > I really think if UEFI does it, we should do it too (and it
> > > > will fix my trusted key bug).
> > > > 
> > > > What does everyone else think?  If it's agreed, I'll code up
> > > > the patch.
> > > > 
> > > > James
> > > 
> > > I think I agree but what worries me is that this error code is
> > > almost not documented at all in TCG specifications.
> > 
> > Yes, it's cryptic, it just says "the TPM was not able to start the
> > command", which is why I checked the reference implementation.  I
> > think the UEFI ref is the clearest because it provides
> > implementation guidance (basically to retry the command until
> > timeout) which I see no reason we can't follow in the kernel.
> > 
> > The alternative is that we handle it in the TSS and I have to patch
> > trusted keys.  However none of the core TPM routines that use
> > tpm_transmit_cmd() have any retry logic, so if we find a TPM that
> > returns it to a kernel sent command we'll get random unexplained
> > failures (based on the ref implementation I don't think this is
> > likely, but by the spec it's possible, especially for a
> > manufacturer who isn't using the reference), so we may end up
> > having to add retry logic to every place in the kernel where we use
> > tpm_transmit_cmd() ... then we'll wish we'd done it generically
> > inside that routine.
> > 
> > Based on this, I think the UEFI spec gives us the safety of knowing
> > that internal handling is correct and the advantages of doing so
> > now are that we don't get a potentially exploding problem later, so
> > it seems like a net win.  I'll code up a patch.
> > 
> > James
> > 
> 
> If the TSS starts retrying commands itself, then it's more than just
> handling TPM_RC_RETRY. There is also even more cryptic
> TPM_RC_YIELDED, which allows (suggests?) retrying,

This is only for software based TPMs, so I suppose intel PTT could do
it, but, as the note says, it's not issued by the reference
implementation at all.  I'd say ignore it until we find an actual
implementor.

>  and TPM_RC_NV_RATE, which covers one of the few legitimate and
> easily explainable reasons why a command can fail now but succeed
> later.

NV_RATE is only for commands that touch NV memory, so that updates can
be rate limited.  The docs imply it's only for NV commands, which we
don't issue in the kernel.

> And what about TPM_RC_MEMORY,

It means there are too many objects in the TPM and is not fixable by a
retry only by unloading something.

>  TPM_RC_NV_UNAVAILABLE and such?

That means the platform component reports the NV memory isn't present.
 This is only for TPMs with external NV chips, I believe and it means
there's a NV connection or provisioning problem.

> Though quite exotic, they are all warnings, and all indicate a
> possibly temporary lack of access/resources. Unlike TPM_RC_*_HANDLES,
> CONTEXT_GAP etc, they don't require RM to do anything with objects
> and handles to recover, and may succeed if simply retried after a
> delay.

No ... the only one you've cited that may be retry amenable is
RC_YIELDED ... the rest indicate other problems a retry won't fix and
as such should be exposed to the user.

> Shall these retries be also handled in tpm_transmit_cmd() or left up
> to the caller to take care of? And how are they different from
> TPM_RC_RETRY case, if the latter?
> 
> If you are interested in the actual in-kernel use of the commands
> that can face such codes, look no further than tpm2_load_context().
> The reference implementation may return TPM_RC_NV* from
> TPM2_ContextLoad since it calls NvIsAvailable().

You may be looking at a different emulator.  In the Microsoft one
it's RETURN_IF_NV_IS_NOT_AVAILABLE and it can't actually be returned by
context loading or saving.

In general I'd say we do the pragmatic not the theoretical thing, so
handle retry returns that have actually been seen and which have caused
problems.  That set is currently only RC_RETRY.

James