[U-Boot] [PATCH v2] tpm: Add TPM command library
Che-liang Chiou
clchiou at chromium.org
Thu Feb 28 20:47:56 CET 2013
Hi Reinhard,
Replied below.
On Thu, Feb 28, 2013 at 6:50 AM, Pfau, Reinhard <Pfau at 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-bounces at lists.denx.de
>> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Che-Liang Chiou
>> Sent: Friday, February 15, 2013 12:01 AM
>> To: u-boot at 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 <clchiou at 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
More information about the U-Boot
mailing list