Hi, While digging through the code, some question arises. So let me drop some notes about the patch:
( I apologize for some weird word wraps in the quoted text, but I have to use a well known UMA which seems to be too stupid to keep lines of text without additional word wraps...) > -----Original Message----- > From: u-boot-boun...@lists.denx.de > [mailto:u-boot-boun...@lists.denx.de] On Behalf Of Che-Liang Chiou > Sent: Friday, February 15, 2013 12:01 AM > To: u-boot@lists.denx.de > Subject: [U-Boot] [PATCH v2] tpm: Add TPM command library > > TPM command library implements a subset of TPM commands defined in TCG > Main Specification 1.2 that are useful for implementing secure boot. > More TPM commands could be added out of necessity. > > You may exercise these commands through the 'tpm' command. > However, the > raw TPM commands are too primitive for writing secure boot in command > interpreter scripts; so the 'tpm' command also provides > helper functions > to make scripting easier. > > For example, to define a counter in TPM non-volatile storage and > initialize it to zero: > > $ tpm init > $ tpm startup TPM_ST_CLEAR > $ tpm nv_define d 0x1001 0x1 > $ tpm nv_write d 0x1001 0 > > And then increment the counter by one: > > $ tpm nv_read d 0x1001 i > $ setexpr.l i $i + 1 > $ tpm nv_write d 0x1001 $i > > Signed-off-by: Che-Liang Chiou <clch...@chromium.org> > --- > common/cmd_tpm.c | 709 > +++++++++++++++++++++++++++++++++++++++-------- > include/{tpm.h => tis.h} | 8 +- > include/tpm.h | 197 ++++++++++--- > lib/Makefile | 1 + > lib/tpm.c | 569 +++++++++++++++++++++++++++++++++++++ > 5 files changed, 1339 insertions(+), 145 deletions(-) > copy include/{tpm.h => tis.h} (95%) > create mode 100644 lib/tpm.c > [snip] > diff --git a/lib/tpm.c b/lib/tpm.c > new file mode 100644 > index 0000000..7d13951 > --- /dev/null > +++ b/lib/tpm.c [snip] > +/** > + * Send a TPM command and return response's return code. > + * > + * @param command byte string of TPM command > + * @return return code of the TPM response > + */ > +static uint32_t tpm_send_command(const void *command) > +{ > + uint8_t response[COMMAND_BUFFER_SIZE]; > + size_t response_length = sizeof(response); > + uint32_t err; > + > + err = tis_sendrecv(command, tpm_command_size(command), > + response, &response_length); > + if (err) > + return err; > + > + return tpm_return_code(response); > +} Using the result of tis_sendrecv would be OK with the I2C-TPM patch posted on this ML about a year ago. The implementation of tis_sendrecv in generic_lpc_tpm.c returns the value (1) on error instead of -1 (as documented in the header file). Since 1 is a well defined TPM result code (TPM_AUTHFAIL) this could result in some problems. (But I think, this is a bug in the current generic_lpc_tpm driver...) [snip] > + > +uint32_t tpm_nv_read_value(uint32_t index, void *data, > uint32_t count) > +{ > + const uint8_t command[22] = { > + 0x0, 0xc1, 0x0, 0x0, 0x0, 0x16, 0x0, 0x0, 0x0, 0xcf, > + }; > + const size_t index_offset = 10; > + const size_t length_offset = 18; > + const size_t data_size_offset = 10; > + const size_t data_offset = 14; > + uint8_t buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > + uint32_t response_length = sizeof(response), data_size; > + uint32_t err; > + > + if (pack_byte_string(buf, sizeof(buf), "sdd", > + 0, command, sizeof(command), > + index_offset, index, > + length_offset, count)) > + return TPM_LIB_ERROR; > + err = tis_sendrecv(buf, tpm_command_size(buf), > + response, &response_length); > + if (err) > + return err; At this point we should add something like: err = tpm_return_code(response); if (err) return err; to return the "real" result code from the TPM in case of an error. Else the following code will map all TPM errors to TPM_LIB_ERROR (since unpack_byte_string will fail). Same in the other funcs which interpret the result of the command (tpm_extend, tpm_pcr_read, rtpm_read_pubek, tpm_get_capability). > + if (unpack_byte_string(response, response_length, "d", > + data_size_offset, &data_size)) > + return TPM_LIB_ERROR; > + if (data_size > count) > + return TPM_LIB_ERROR; > + if (unpack_byte_string(response, response_length, "s", > + data_offset, data, data_size)) > + return TPM_LIB_ERROR; > + > + return 0; > +} > + > +uint32_t tpm_nv_write_value(uint32_t index, const void > *data, uint32_t length) > +{ > + const uint8_t command[256] = { > + 0x0, 0xc1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xcd, > + }; > + const size_t command_size_offset = 2; > + const size_t index_offset = 10; > + const size_t length_offset = 18; > + const size_t data_offset = 22; > + const size_t write_info_size = 12; > + const uint32_t total_length = > + TPM_REQUEST_HEADER_LENGTH + write_info_size + length; > + uint8_t buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE]; > + uint32_t response_length = sizeof(response); > + uint32_t err; > + > + if (pack_byte_string(buf, sizeof(buf), "sddds", > + 0, command, sizeof(command), > + command_size_offset, total_length, > + index_offset, index, > + length_offset, length, > + data_offset, data, length)) > + return TPM_LIB_ERROR; > + err = tis_sendrecv(buf, tpm_command_size(buf), > + response, &response_length); > + if (err) > + return err; > + > + return 0; Why not just call tpm_send_command(buf) here like in the other funcs where the Result does not need further interpretation (beside the return code)? Beside theese remarks: I like Your patch, since one of our upcoming boards also uses a TPM to check system integrity at boot time :-) Greetings, Reinhard Pfau.
<<winmail.dat>>
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
