[PATCH v2 1/1] tpm: clear state post probing

[Simon Glass]

Hi Heinrich,

On Wed, 17 Nov 2021 at 12:34, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
>
>
> On 11/17/21 20:30, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 17 Nov 2021 at 12:28, Heinrich Schuchardt
> > <heinrich.schuchardt at canonical.com> wrote:
> >>
> >>
> >>
> >> On 11/17/21 20:22, Ilias Apalodimas wrote:
> >>> Hi Simon,
> >>>
> >>>
> >>> On Wed, 17 Nov 2021 at 19:20, Simon Glass <sjg at chromium.org> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> (replying to both of you)
> >>>>
> >>>> On Wed, 17 Nov 2021 at 01:18, Ilias Apalodimas
> >>>> <ilias.apalodimas at linaro.org> wrote:
> >>>>>
> >>>>> Hi Simon,
> >>>>>
> >>>>>
> >>>>> On Wed, 17 Nov 2021 at 04:48, Simon Glass <sjg at chromium.org> wrote:
> >>>>>>
> >>>>>> Hi Heinrich,
> >>>>>>
> >>>>>> On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas
> >>>>>> <ilias.apalodimas at linaro.org> wrote:
> >>>>>>>
> >>>>>>> On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
> >>>>>>>> Before we can start measuring the TPM must be cleared. Do this in the
> >>>>>>>> post_probe() method of the uclass.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> >>>>>>>> ---
> >>>>>>>> v2:
> >>>>>>>>         tpm_startup2() is not available on all boards.
> >>>>>>>>         tpm_startup() takes care of translating the call.
> >>>>>>>> ---
> >>>>>>>>    drivers/tpm/tpm-uclass.c | 13 +++++++++++++
> >>>>>>>>    1 file changed, 13 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> >>>>>>>> index f67fe1019b..abd9ce35e8 100644
> >>>>>>>> --- a/drivers/tpm/tpm-uclass.c
> >>>>>>>> +++ b/drivers/tpm/tpm-uclass.c
> >>>>>>>> @@ -11,6 +11,7 @@
> >>>>>>>>    #include <log.h>
> >>>>>>>>    #include <linux/delay.h>
> >>>>>>>>    #include <linux/unaligned/be_byteshift.h>
> >>>>>>>> +#include <tpm_api.h>
> >>>>>>>>    #include <tpm-v1.h>
> >>>>>>>>    #include <tpm-v2.h>
> >>>>>>>>    #include "tpm_internal.h"
> >>>>>>>> @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> >>>>>>>>         return 0;
> >>>>>>>>    }
> >>>>>>>>
> >>>>>>>> +static int dm_tpm_post_probe(struct udevice *dev)
> >>>>>>
> >>>>>> Please drop the dm_
> >>>>>>
> >>>>>>>> +{
> >>>>>>>> +     /*
> >>>>>>>> +      * Clearing the TPM state is only possible once after a hard reset.
> >>>>>>>> +      * As we do not know if the TPM has been cleared by a prior boot stage
> >>>>>>>> +      * ignore the return value here.
> >>>>>>>> +      */
> >>>>>>>> +     tpm_startup(dev, TPM_ST_CLEAR);
> >>>>>>
> >>>>>> blank line before final return
> >>>>>>
> >>>>>>>> +     return 0;
> >>>>>>>> +}
> >>>>>>
> >>>>>> This should only happen once and if the TPM is set up in SPL then this
> >>>>>> seems to cause a failure if done again.
> >>>>>>
> >>>>>
> >>>>> Not really. If you run the tpm_startup twice and the TPM is already
> >>>>> initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you
> >>>>> can easily check against and decide.
> >>>>>
> >>>>>> Is there another way we can deal with this? Could the TPM user decide
> >>>>>> whether it needs to be set?
> >>>>>
> >>>>> Why? Part of the TPM init is making it usable.  We are trying to move
> >>>>> away from having to add something in the command line to make the
> >>>>> device usable.
> >>>>
> >>>> For a start, we should not start up the TPM until we need it. That
> >>>> goes against the U-Boot lazy-init approach. We already have a command
> >>>> to do it, as well as a TPM-API thing, so let's use that.
> >>
> >> The TPM is started exactly when we need it. When the user executes
> >> bootefi the TPM is probed if the TCG2 protocol is enabled because we
> >> need to measure binaries. Probing always has initialized TPMs. With this
> >> patch we only additionally clear it.
> >>
> >> This patch does not contradict lazy initialization.
> >
> > I think it does, because you are calling a TPM operation in the
> > probe() method. Apart from the double-init problem you are creating, I
> > may not want to start the TPM at this point.
> >
>
> A TPM is nothing that is 'started'. It is a coprocessor that is running
> once you switch on the board.
>
> If the TPM is probed, you have executed a command that makes use of the
> TPM. Why should it not be cleared?

Why should it be? You haven't explained the problem here.

Starting the TPM can take a noticeable amount of time.

Regards,
Simon