[PATCH 1/2 v3] tpm: add a function that performs selftest + startup

[Ilias Apalodimas]

Hi Simon,

Apologies for the late reply.

On Tue, Feb 07, 2023 at 06:38:54AM -0700, Simon Glass wrote:
> Hi Ilias,
>
> On Mon, 6 Feb 2023 at 06:25, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Simon,
> > On Sat, Jan 28, 2023 at 03:01:22PM -0700, Simon Glass wrote:
> > > Hi Ilias,
> > >
> > > On Thu, 26 Jan 2023 at 01:18, Ilias Apalodimas
> > > <ilias.apalodimas at linaro.org> wrote:
> > > >
> > > > As described in [0] if a command requires use of an untested algorithm
> > > > or functional module, the TPM performs the test and then completes the
> > > > command actions.
> > > >
> > > > Since we don't check for TPM_RC_NEEDS_TEST (which is the return code of
> > > > the TPM in that case) and even if we would, it would complicate our TPM
> > > > code for no apparent reason,  add a wrapper function that performs both
> > > > the selftest and the startup sequence of the TPM.
> > > >
> > > > It's worth noting that this is implemented on TPMv2.0.  The code for
> > > > 1.2 would look similar,  but I don't have a device available to test.
> > > >
> > > > [0]
> > > > https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> > > > §12.3 Self-test modes
> > > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > > ---
> > > > Changes since v2:
> > > > - add tpm_init() to auto start
> > > >
> > > > Changes since v1:
> > > > - Remove a superfluous if statement
> > > > - Move function comments to the header file
> > > >  include/tpm-v2.h  | 19 +++++++++++++++++++
> > > >  include/tpm_api.h |  8 ++++++++
> > > >  lib/tpm-v2.c      | 24 ++++++++++++++++++++++++
> > > >  lib/tpm_api.c     |  8 ++++++++
> > > >  4 files changed, 59 insertions(+)
> > >
> > > I think this is a good idea, but it should be implemented at the API
> > > level. Please see below.
> >
> > It is implemented at the API level.  I could try testing this in QEMU using
> > swtpm, but the driver I added for mmio is TPMv2 only.  So I don't really feel
> > comfortable adding support for a device I can't test.  If someone has a
> > tpm1.2 and is willing to test it, I can modify the patch accordingly.
>
> swtpm is a bit of a pain, as we discussed and as you are showing by
> this comment. IMO it is good enough to use the sandbox testing for
> this function. See below.
>

The comment says the exact opposite.  It never hints on any swtpm
limitations or problems.

fwiw swtpm is fine.  It even offers a socket we could hook against instead of
re-inventing the wheel with the sandbox tpm (which is missing 95% of the
tpm functionality anyway).  The reason we can't use it is that the u-boot
driver only implements 2.0.

[...]

> > > > +       rc = tpm_init(dev);
> > > > +       if (rc && rc != -EBUSY)
> > >
> > > Does that work, with rc being unsigned?
> >
> >
> > Yes integer promotion is fine here.  As long as we don't try to do anything
> > silly e.g start doing math on the result or compare it against bigger types
> > this is fine.
>
> OK
>
> > There was a patch from Sughosh in the past that you rejected and it was
> > converting enough of the TPM API return calls to int.  I think the reason
> > the API works with u32, is that the spec return codes from the device
> > itself are described as u32.  But that shouldn't affect the internal U-Boot
> > API.  I can send a patch afterwards moving the U-Boot TPM API functions and
> > return int.
>
> Well, the question is, what is the return value? If it is the actual
> TPM return code, then u32 is (unfortunately) correct. If it is an
> errno then it should be int. The challenge is that some code wants to
> know about the TPM internals, special error codes, etc. so we cannot
> easily move to errno. Is that right?

Maybe, I'll have to take a closer look, but in principle I don't see why
the internal API should return device error codes verbatim.  If we care
about the tpm error that much we can pass a u32 * and still return an int.
In any case as we discussed integer promotion is fine here.

>
> >
> > >
> > > > +               return rc;
> > > > +       rc = tpm2_self_test(dev, TPMI_YES);
> > >
> > > Can we call tpm_self_test_full() ? If not, please update the API as needed.
> > >
> > > > +
> > > > +       if (rc == TPM2_RC_INITIALIZE) {
> > > > +               rc = tpm2_startup(dev, TPM2_SU_CLEAR);
> > >
> > > Should call tpm_startup()
> >
> >
> > Same logic applies to both of these.  Yes that makes sense to use the top
> > level API but only if we add support for 1.2 as well.  Otherwise the only
> > thing this is going to create is circular dependencies between the v2 code
> > and the API library.
>
> Then just add 1.2 support.

That doesn't make any sense and I personally don't agree with the comment.
The patch improves and fixes problems we have in TPM 2.0 which is what
(hopefully) all devices use the last couple of years.

Unfortunately I don't have the time to fix 1.2 and I don't see why this
should hinder our efforts for decent TPM2.0 support.

>
> >
> > >
> > > > +               if (rc)
> > > > +                       return rc;
> > > > +
> > > > +               rc = tpm2_self_test(dev, TPMI_YES);
> > >
> > > Again, tpm_self_test_full(). We are trying to provide a TPM API that
> > > covers v1 and v2, to the extent possible.
> >
> > See above.
> >
> > >
> > > > +       }
> > > > +
> > > > +       return rc;
> > > > +}
> > > > +
> > >
> > > Also please add a test for this to test/dm/tpm.c
> >
> > sure
> >
> > >
> > > >  u32 tpm2_clear(struct udevice *dev, u32 handle, const char *pw,
> > > >                const ssize_t pw_sz)
> > > >  {
> > > > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > > > index 7e8df8795ef3..5b2c11a277cc 100644
> > > > --- a/lib/tpm_api.c
> > > > +++ b/lib/tpm_api.c
> > > > @@ -35,6 +35,14 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> > > >         }
> > > >  }
> > > >
> > > > +u32 tpm_auto_start(struct udevice *dev)
> > > > +{
> > > > +       if (tpm_is_v2(dev))
> > > > +               return tpm2_auto_start(dev);
> > >
> > > Hopefully all the code from above can move here.
> >
> > Repeating myself here, but I don't want to add untested code.  So I really
> > prefer the patch as is, until someone verifies the 1.2 init sequence for
> > me.
>
> I don't see any tests in this patch anyway. We do have some very basic
> tests for the TPM in test/dm/tpm.c and I'm sure you can add your
> function there. That should be enough for now. It really doesn't make
> sense to work around the existing API just as a way of not
> implementing it for something you cannot manually test. It just adds
> confusion when some functions use the API and some don't.

I've already added the tests and having selftests in test/dm/tpm.c makes
sense.  What doesn't is requesting to fix 1.2 as well.  I'll send a v2 with
the tests included, but I can't change the API side of things.
I can send an RFC that fixes tpm1.2 if someone can test that on
real hardware.  But I don't have time to spend adding 1.2 support in sandbox.

Regards
/Ilias

>
> Regards,
> Simon