[U-Boot] [PATCH v2 11/19] tpm: add TPM2_Clear command support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Simon,

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

> Hi Miquel,
> 
> On 29 March 2018 at 15:43, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > Add support for the TPM2_Clear command.
> >
> > Change the command file and the help accordingly.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  cmd/tpm.c      | 29 ++++++++++++++++++++++++++---
> >  cmd/tpm_test.c |  6 +++---
> >  include/tpm.h  | 21 +++++++++++++++++----
> >  lib/tpm.c      | 42 ++++++++++++++++++++++++++++++++++++++----
> >  4 files changed, 84 insertions(+), 14 deletions(-)
> >
> > diff --git a/cmd/tpm.c b/cmd/tpm.c
> > index fc9ef9d4a3..32921e1a70 100644
> > --- a/cmd/tpm.c
> > +++ b/cmd/tpm.c
> > @@ -454,6 +454,29 @@ static int do_tpm_init(cmd_tbl_t *cmdtp, int flag,
> >         return report_return_code(tpm_init());
> >  }
> >
> > +static int do_tpm_force_clear(cmd_tbl_t *cmdtp, int flag,
> > +                             int argc, char * const argv[])
> > +{
> > +       u32 handle = 0;
> > +       const char *pw = (argc < 3) ? NULL : argv[2];
> > +       const ssize_t pw_sz = pw ? strlen(pw) : 0;
> > +
> > +       if (argc < 2 || argc > 3)
> > +               return CMD_RET_USAGE;
> > +
> > +       if (pw_sz > TPM2_DIGEST_LENGTH)
> > +               return -EINVAL;
> > +
> > +       if (!strcasecmp("TPM2_RH_LOCKOUT", argv[1]))
> > +               handle = TPM2_RH_LOCKOUT;
> > +       else if (!strcasecmp("TPM2_RH_PLATFORM", argv[1]))
> > +               handle = TPM2_RH_PLATFORM;
> > +       else
> > +               return CMD_RET_USAGE;
> > +
> > +       return report_return_code(tpm_force_clear(handle, pw, pw_sz));
> > +}
> > +
> >  #define TPM_COMMAND_NO_ARG(cmd)                                \
> >  static int do_##cmd(cmd_tbl_t *cmdtp, int flag,                \
> >                 int argc, char * const argv[])          \
> > @@ -465,7 +488,6 @@ static int do_##cmd(cmd_tbl_t *cmdtp, int flag,             \
> >
> >  TPM_COMMAND_NO_ARG(tpm_self_test_full)
> >  TPM_COMMAND_NO_ARG(tpm_continue_self_test)
> > -TPM_COMMAND_NO_ARG(tpm_force_clear)
> >  TPM_COMMAND_NO_ARG(tpm_physical_enable)
> >  TPM_COMMAND_NO_ARG(tpm_physical_disable)
> >
> > @@ -951,8 +973,9 @@ U_BOOT_CMD(tpm, CONFIG_SYS_MAXARGS, 1, do_tpm,
> >  "  physical_set_deactivated 0|1\n"
> >  "    - Set deactivated flag.\n"
> >  "Admin Ownership Commands:\n"
> > -"  force_clear\n"
> > -"    - Issue TPM_ForceClear command.\n"
> > +"  force_clear [<type>]\n"
> > +"    - Issue TPM_[Force]Clear command, with <type> one of (TPMv2 only):\n"
> > +"          * TPM2_RH_LOCKOUT, TPM2_RH_PLATFORM.\n"
> >  "  tsc_physical_presence flags\n"
> >  "    - Set TPM device's Physical Presence flags to <flags>.\n"
> >  "The Capability Commands:\n"
> > diff --git a/cmd/tpm_test.c b/cmd/tpm_test.c
> > index 37ad2ff33d..da40dbc423 100644
> > --- a/cmd/tpm_test.c
> > +++ b/cmd/tpm_test.c
> > @@ -176,7 +176,7 @@ static int test_fast_enable(void)
> >         TPM_CHECK(tpm_get_flags(&disable, &deactivated, NULL));
> >         printf("\tdisable is %d, deactivated is %d\n", disable, deactivated);
> >         for (i = 0; i < 2; i++) {
> > -               TPM_CHECK(tpm_force_clear());
> > +               TPM_CHECK(tpm_force_clear(0, NULL, 0));
> >                 TPM_CHECK(tpm_get_flags(&disable, &deactivated, NULL));
> >                 printf("\tdisable is %d, deactivated is %d\n", disable,
> >                        deactivated);
> > @@ -458,7 +458,7 @@ static int test_write_limit(void)
> >         TPM_CHECK(TlclStartupIfNeeded());
> >         TPM_CHECK(tpm_self_test_full());
> >         TPM_CHECK(tpm_tsc_physical_presence(PRESENCE));
> > -       TPM_CHECK(tpm_force_clear());
> > +       TPM_CHECK(tpm_force_clear(0, NULL, 0));
> >         TPM_CHECK(tpm_physical_enable());
> >         TPM_CHECK(tpm_physical_set_deactivated(0));
> >
> > @@ -477,7 +477,7 @@ static int test_write_limit(void)
> >         }
> >
> >         /* Reset write count */
> > -       TPM_CHECK(tpm_force_clear());
> > +       TPM_CHECK(tpm_force_clear(0, NULL, 0));
> >         TPM_CHECK(tpm_physical_enable());
> >         TPM_CHECK(tpm_physical_set_deactivated(0));
> >
> > diff --git a/include/tpm.h b/include/tpm.h
> > index 38d7cb899d..2f17166662 100644
> > --- a/include/tpm.h
> > +++ b/include/tpm.h
> > @@ -43,13 +43,23 @@ enum tpm_startup_type {
> >  };
> >
> >  enum tpm2_startup_types {
> > -       TPM2_SU_CLEAR   = 0x0000,
> > -       TPM2_SU_STATE   = 0x0001,
> > +       TPM2_SU_CLEAR           = 0x0000,
> > +       TPM2_SU_STATE           = 0x0001,
> > +};
> > +
> > +enum tpm2_handles {  
> 
> Please add comment to enum

Fine, I will document all of them. Just for you to know, these are
values extracted from the (publicly available) specification, I did
not change any of them.

> 
> > +       TPM2_RH_OWNER           = 0x40000001,
> > +       TPM2_RS_PW              = 0x40000009,
> > +       TPM2_RH_LOCKOUT         = 0x4000000A,
> > +       TPM2_RH_ENDORSEMENT     = 0x4000000B,
> > +       TPM2_RH_PLATFORM        = 0x4000000C,
> >  };
> >
> >  enum tpm2_command_codes {
> >         TPM2_CC_STARTUP         = 0x0144,
> >         TPM2_CC_SELF_TEST       = 0x0143,
> > +       TPM2_CC_CLEAR           = 0x0126,
> > +       TPM2_CC_CLEARCONTROL    = 0x0127,
> >         TPM2_CC_GET_CAPABILITY  = 0x017A,
> >         TPM2_CC_PCR_READ        = 0x017E,
> >         TPM2_CC_PCR_EXTEND      = 0x0182,
> > @@ -567,11 +577,14 @@ uint32_t tpm_tsc_physical_presence(uint16_t presence);
> >  uint32_t tpm_read_pubek(void *data, size_t count);
> >
> >  /**
> > - * Issue a TPM_ForceClear command.
> > + * Issue a TPM_ForceClear or a TPM2_Clear command.
> >   *
> > + * @param handle       Handle
> > + * @param pw           Password
> > + * @param pw_sz                Length of the password
> >   * @return return code of the operation
> >   */
> > -uint32_t tpm_force_clear(void);
> > +int tpm_force_clear(u32 handle, const char *pw, const ssize_t pw_sz);
> >
> >  /**
> >   * Issue a TPM_PhysicalEnable command.
> > diff --git a/lib/tpm.c b/lib/tpm.c
> > index 3cc2888267..9a46ac09e6 100644
> > --- a/lib/tpm.c
> > +++ b/lib/tpm.c
> > @@ -608,13 +608,47 @@ uint32_t tpm_read_pubek(void *data, size_t count)
> >         return 0;
> >  }
> >
> > -uint32_t tpm_force_clear(void)
> > +int tpm_force_clear(u32 handle, const char *pw, const ssize_t pw_sz)
> >  {
> > -       const uint8_t command[10] = {
> > -               0x0, 0xc1, 0x0, 0x0, 0x0, 0xa, 0x0, 0x0, 0x0, 0x5d,
> > +       const u8 command_v1[10] = {
> > +               U16_TO_ARRAY(0xc1),
> > +               U32_TO_ARRAY(10),
> > +               U32_TO_ARRAY(0x5d),
> >         };
> > +       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> > +               U16_TO_ARRAY(TPM2_ST_SESSIONS), /* TAG */
> > +               U32_TO_ARRAY(27 + pw_sz),       /* Length */
> > +               U32_TO_ARRAY(TPM2_CC_CLEAR),    /* Command code */  
> 
> Here we have both v1 and v2 commands. Perhaps we should have a Kconfig
> option to enable v2 support? Or do you think it is not a big addition
> in terms of code side?

It is a big addition in terms of code side.

> 
> One way to do this would be to have an inline function which can tell
> if you are using v2:
> 
> static inline bool tpm_v2(void) {
>    return IS_ENABLED(CONFIG_TPM_V2) && further things...
> }
> 
> static inline bool tpm_v1(void) {
>    return IS_ENABLED(CONFIG_TPM_V1) && further things...
> }

I like this way of choosing one specification or the other, I could
make them mutually exclusive. It would prevent the need for a global
variable (or an additional field in uclass).

Would it be fine to actually rename the file (with a 'tpm1' suffix) and
create a new one specific for TPMv2 commands ? Only one file would be
compiled/linked depending on the configuration, avoiding the possibility
to call a v1 command from a v2 chip and vice versa.

At first I thought a lot of code would be shared but I don't think we
would add so much by having one function per revision now.

> 
> >
> > -       return tpm_sendrecv_command(command, NULL, NULL);
> > +               /* HANDLE */
> > +               U32_TO_ARRAY(handle),           /* TPM resource handle */  
> 
> If we really need these, perhaps tpm_u32() is a better name that
> U32_TO_ARRAY() ?
> 
> > +
> > +               /* AUTH_SESSION */
> > +               U32_TO_ARRAY(9 + pw_sz),        /* Authorization size */
> > +               U32_TO_ARRAY(TPM2_RS_PW),       /* Session handle */
> > +               U16_TO_ARRAY(0),                /* Size of <nonce> */
> > +                                               /* <nonce> (if any) */
> > +               0,                              /* Attributes: Cont/Excl/Rst */
> > +               U16_TO_ARRAY(pw_sz),            /* Size of <hmac/password> */
> > +               /* STRING(pw)                      <hmac/password> (if any) */
> > +       };
> > +       unsigned int offset = 27;
> > +       int ret;
> > +
> > +       if (!is_tpmv2)
> > +               tpm_sendrecv_command(command_v1, NULL, NULL);
> > +
> > +       /*
> > +        * Fill the command structure starting from the first buffer:
> > +        *     - the password (if any)
> > +        */
> > +       ret = pack_byte_string(command_v2, sizeof(command_v2), "s",
> > +                              offset, pw, pw_sz);
> > +       offset += pw_sz;
> > +       if (ret)
> > +               return TPM_LIB_ERROR;
> > +
> > +       return tpm_sendrecv_command(command_v2, NULL, NULL);
> >  }
> >
> >  uint32_t tpm_physical_enable(void)
> > --
> > 2.14.1
> >  
> 
> Regards,
> Simon

Thanks,
Miquèl


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