[U-Boot] [PATCH v2 10/19] tpm: add TPM2_SelfTest command support

[Miquel Raynal]

Hi Simon,

I am back on that topic, let me answer some of your questions before
addressing them in a next version.

On Fri, 30 Mar 2018 06:42:25 +0800, Simon Glass
<sjg at chromium.org> wrote:

> Hi Miquel,
> 
> On 29 March 2018 at 15:43, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> > Add support for the TPM2_Selftest command.
> >
> > Change the command file and the help accordingly.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> > ---
> >  include/tpm.h |  9 +++++++--
> >  lib/tpm.c     | 36 ++++++++++++++++++++++++++++--------
> >  2 files changed, 35 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/tpm.h b/include/tpm.h
> > index ba71bac885..38d7cb899d 100644
> > --- a/include/tpm.h
> > +++ b/include/tpm.h
> > @@ -31,6 +31,11 @@ enum tpm2_structures {
> >         TPM2_ST_SESSIONS        = 0x8002,
> >  };
> >
> > +enum tpm2_yes_no {
> > +       TPMI_YES                = 1,
> > +       TPMI_NO                 = 0,
> > +};
> > +
> >  enum tpm_startup_type {
> >         TPM_ST_CLEAR            = 0x0001,
> >         TPM_ST_STATE            = 0x0002,
> > @@ -476,14 +481,14 @@ int tpm_startup(enum tpm_startup_type mode);
> >   *
> >   * @return return code of the operation
> >   */
> > -uint32_t tpm_self_test_full(void);
> > +int tpm_self_test_full(void);
> >
> >  /**
> >   * Issue a TPM_ContinueSelfTest command.
> >   *
> >   * @return return code of the operation
> >   */
> > -uint32_t tpm_continue_self_test(void);
> > +int tpm_continue_self_test(void);
> >
> >  /**
> >   * Issue a TPM_NV_DefineSpace command.  The implementation is limited
> > diff --git a/lib/tpm.c b/lib/tpm.c
> > index 0c57ef8dc7..3cc2888267 100644
> > --- a/lib/tpm.c
> > +++ b/lib/tpm.c
> > @@ -341,20 +341,40 @@ int tpm_startup(enum tpm_startup_type mode)
> >         return 0;
> >  }
> >
> > -uint32_t tpm_self_test_full(void)
> > +int tpm_self_test_full(void)
> >  {
> > -       const uint8_t command[10] = {
> > -               0x0, 0xc1, 0x0, 0x0, 0x0, 0xa, 0x0, 0x0, 0x0, 0x50,
> > +       const u8 command_v1[10] = {
> > +               U16_TO_ARRAY(0xc1),  
> 
> Here I can see some benefit to your macros because the data is better
> structured. But why not use the pack_byte_string() function?

TPM buffers (1.0 and 2.0) are, most of the time, filled with data of
known length (1, 2 or 4 bytes). You can see that most of the time,
TPMv1 simple commands were just filling a buffer of bytes. I don't like
very much the fact that the user should split the data in byte array so
I introduced these macros that do it for me in a cleaner way. When you
get an hexadecimal value (like it was the case before) it is easy to
split a "0xabcd" in "0xab, 0xcd", but I prefer to use variables or
simply defines sometimes and writing "TPM2_ST_NO_SESSIONS" as
"TPM2_ST_NO_SESSIONS >> 8, TPM2_ST_NO_SESSIONS & 0xFF" is not readable
at all.

Now why did I not use the existing pack_byte_string() function. Well,
at first I did and realized it was a pain to have a decent
incrementation of the offsets, mostly when commands tend to be longer
and longer. Having such lists of offset/variable/length while you could
define them statically on the stack -which also saves some computing
time- is quickly annoying and hard to review. From my point of view this
function is a real asset when it comes to 'unknown'/'big' variable
length (like a password, an hmac, an user input, etc) but should be
avoided when not necessary: typically to fill a buffer with defined
values.

I am of course very open on the topic, this is my feeling but I don't
have that much experience and would carefully listen to yours.

Thank you,
Miquèl

> 
> > +               U32_TO_ARRAY(10),
> > +               U32_TO_ARRAY(0x50),
> >         };
> > -       return tpm_sendrecv_command(command, NULL, NULL);
> > +       const u8 command_v2[12] = {
> > +               U16_TO_ARRAY(TPM2_ST_NO_SESSIONS),
> > +               U32_TO_ARRAY(11),
> > +               U32_TO_ARRAY(TPM2_CC_SELF_TEST),
> > +               TPMI_YES,
> > +       };
> > +
> > +       return tpm_sendrecv_command(is_tpmv2 ? command_v2 : command_v1, NULL,
> > +                                   NULL);
> >  }
> >

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com