| Message ID | 20180329074401.8691-12-miquel.raynal@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Introduce SPI TPM v2.0 support | expand |
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 > + 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? 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... } > > - 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
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
Hi Miquel, On 24 April 2018 at 07:17, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > 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. Well you could have two separate files, if there really is no sharing of commands. But if there are common commands, you should have a 'common' file to implement them, rather than duplicating code. With the above static inline functions we can support: - v1 only (no code-size increment) - v2 only (minimal code size for what will be a common case) - v1 + v2 (e.g. for sandbox testing) Regards, Simon
Hi Simon, On Thu, 26 Apr 2018 08:40:27 -0600, Simon Glass <sjg@chromium.org> wrote: > Hi Miquel, > > On 24 April 2018 at 07:17, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > 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. > > Well you could have two separate files, if there really is no sharing > of commands. But if there are common commands, you should have a > 'common' file to implement them, rather than duplicating code. > > With the above static inline functions we can support: > > - v1 only (no code-size increment) > - v2 only (minimal code size for what will be a common case) > - v1 + v2 (e.g. for sandbox testing) I am changing the whole architecture as you suggested. Now v1 and v2 are chosen with a Kconfig menu, common code is in a tpm-common.c file, and commands/library functions for each specification is in tpm-v1.c and tpm-v2.c (with all the needed headers. This way TPMv1 code is absolutely untouched while it allows TPMv2 support to be introduced independently. You'll tell me how you find this alternative, I will soon send a v3. Otherwise, as suggested in a first answer, I changed U32_TO_array > > Regards, > Simon
Hi Miquel, On 27 April 2018 at 07:39, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Simon, > > On Thu, 26 Apr 2018 08:40:27 -0600, Simon Glass <sjg@chromium.org> > wrote: > >> Hi Miquel, >> >> On 24 April 2018 at 07:17, Miquel Raynal <miquel.raynal@bootlin.com> wrote: >> > 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. >> >> Well you could have two separate files, if there really is no sharing >> of commands. But if there are common commands, you should have a >> 'common' file to implement them, rather than duplicating code. >> >> With the above static inline functions we can support: >> >> - v1 only (no code-size increment) >> - v2 only (minimal code size for what will be a common case) >> - v1 + v2 (e.g. for sandbox testing) > > I am changing the whole architecture as you suggested. > > Now v1 and v2 are chosen with a Kconfig menu, common code is in a > tpm-common.c file, and commands/library functions for each > specification is in tpm-v1.c and tpm-v2.c (with all the needed headers. > This way TPMv1 code is absolutely untouched while it allows TPMv2 > support to be introduced independently. > > You'll tell me how you find this alternative, I will soon send a v3. > > Otherwise, as suggested in a first answer, I changed U32_TO_array I think it works OK as you have it now, with the rename. Regards, Simon
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 { + 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 */ - return tpm_sendrecv_command(command, NULL, NULL); + /* HANDLE */ + U32_TO_ARRAY(handle), /* TPM resource handle */ + + /* 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)
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(-)