Hi Reinhard, Replied below.
On Thu, Feb 28, 2013 at 6:50 AM, Pfau, Reinhard <p...@gdsys.de> wrote: > > 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] > I agree it is a bug in the current generic_lpc_tpm driver. And here tpm_send_command() should return TPM_LIB_ERROR (which is a distinguishable value from well-defined TPM result codes) in case of tis_sendrecv() returning non-zero value. >> + >> +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). > Done. >> + 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)? > Done. > > 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. > > > Regards, Che-Liang _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
