[PATCH] tpmevlogdump: add dumping sha1 log format event log

[Colin Ian King]

On 10/09/2020 09:08, Ivan Hu wrote:
> Signed-off-by: Ivan Hu <ivan.hu at canonical.com>
> ---
>  src/tpm/tpmevlogdump/tpmevlogdump.c | 36 +++++++++++++++++++++++++++++++-----
>  1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/src/tpm/tpmevlogdump/tpmevlogdump.c b/src/tpm/tpmevlogdump/tpmevlogdump.c
> index dd5e3a5..950554f 100644
> --- a/src/tpm/tpmevlogdump/tpmevlogdump.c
> +++ b/src/tpm/tpmevlogdump/tpmevlogdump.c
> @@ -411,6 +411,36 @@ static void tpmevlogdump_parser(fwts_framework *fw, uint8_t *data, size_t len)
>  	return;
>  }
>  
> +static void tpmevlogdump_event_dump(fwts_framework *fw, uint8_t *data, size_t len)
> +{
> +
> +	uint8_t *pdata = data;
> +	char *str_info;
> +
> +	while (len > 0) {
> +		/* check the data length for dumping */
> +		if (len < sizeof(fwts_pc_client_pcr_event)) {
> +			fwts_log_info(fw, "Cannot get enough length for dumping data.");

How about re-phrasing the message to make it easier to debug if it goes
wrong:

	fwts_log_info("Data too large (%zd bytes) for data buffer (%zd
bytes).", sizeof(fwts_pc_client_pcr_event), len);
	

> +			return;
> +		}
> +		fwts_pc_client_pcr_event *pc_event = (fwts_pc_client_pcr_event *)pdata;

I'd rather have variables declared at the start of a block of code, as
per the informal fwts coding style (much the same way as the kernel).


> +		str_info = tpmevlogdump_pcrindex_to_string(pc_event->pcr_index);
> +		fwts_log_info_verbatim(fw, "PCRIndex:	0x%8.8" PRIx32 "(%s)", pc_event->pcr_index, str_info);
> +		str_info = tpmevlogdump_evtype_to_string(pc_event->event_type);
> +		fwts_log_info_verbatim(fw, "EventType:	0x%8.8" PRIx32 "(%s)", pc_event->event_type, str_info);
> +		tpmevlogdump_data_hexdump(fw, pc_event->digest, sizeof(pc_event->digest), "Digest");
> +		fwts_log_info_verbatim(fw, "EventSize:	0x%8.8" PRIx32, pc_event->event_data_size);
> +		if (pc_event->event_data_size > 0) {
> +			tpmevlogdump_data_hexdump(fw, pc_event->event, pc_event->event_data_size, "Event");
> +		}

Single line if does not need { } braces, but that's my stylistic choice
I guess.

> +		pdata += sizeof(fwts_pc_client_pcr_event) + pc_event->event_data_size;
> +		len -= (sizeof(fwts_pc_client_pcr_event) + pc_event->event_data_size);

the len statement right hand side is in ( ) parenthesis but the pdata
one isn't. I'd prefer it if you used ( ) consistently.

> +	}
> +	return;
> +
> +}
> +
> +
>  static uint8_t *tpmevlogdump_load_file(const int fd, size_t *length)
>  {
>  	uint8_t *ptr = NULL, *tmp;
> @@ -495,11 +525,7 @@ static int tpmevlogdump_test1(fwts_framework *fw)
>  					if (strstr((char *)(data + sizeof(fwts_pc_client_pcr_event)), FWTS_TPM_EVENTLOG_V2_SIGNATURE))
>  						tpmevlogdump_parser(fw, data, length);
>  					else {
> -						fwts_log_info(fw, "Cannot find the tpm2 event log. Aborted.");
> -						free(data);
> -						(void)closedir(dir);
> -						(void)close(fd);
> -						return FWTS_ABORTED;
> +						(void)tpmevlogdump_event_dump(fw, data, length);
>  					}
>  					free(data);
>  				}
>