[U-Boot] [PATCH v2] tpm: Add TPM command library

[Pfau, Reinhard]

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]

> +
> +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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/ms-tnef
Size: 5088 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130228/fb2e6c5b/attachment.bin>