[PATCH v3 2/6] tpm: Support boot measurements

[Eddie James]

On 1/16/23 06:00, Ilias Apalodimas wrote:
> Hi Eddie
>
>
>> +static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a)
>> +{
>> +	switch (a) {
>> +	case TPM2_ALG_SHA1:
>> +		return TPM2_SHA1_DIGEST_SIZE;
>> +	case TPM2_ALG_SHA256:
>> +		return TPM2_SHA256_DIGEST_SIZE;
>> +	case TPM2_ALG_SHA384:
>> +		return TPM2_SHA384_DIGEST_SIZE;
>> +	case TPM2_ALG_SHA512:
>> +		return TPM2_SHA512_DIGEST_SIZE;
>> +	default:
>> +		return 0;
>> +	}
>> +}
> Any reason we can't move the static 'const struct digest_info
> hash_algo_list' from the efi_tcg.c here?  We can then move the
> functions defined in there alg_to_mask and alg_to_len.
>
> And since alg_to_mask is really just a bitshift maybe replace that?


Hi,

It seems more efficient to keep the switch statement rather than iterate 
through the structure array looking for the matching hash algorithm? I 
could remove the 'static const struct digest_info hash_algo_list' in 
efi_tcg2.c and instead use the tpm2_algorithm_to_len and 
tpm2_algorithm_to_mask in efi_tcg2.c. What do you think?


>> +
>> +#define tpm2_algorithm_to_mask(a)	(1 << (a))
>> +
>>   /* NV index attributes */
>>   enum tpm_index_attrs {
>>   	TPMA_NV_PPWRITE		= 1UL << 0,
>> @@ -419,6 +481,142 @@ enum {
>>   	HR_NV_INDEX		= TPM_HT_NV_INDEX << HR_SHIFT,
>>   };
>>
>> +/**
>> + * struct tcg2_event_log - Container for managing the platform event log
>> + *
>> + * @log:		Address of the log
>> + * @log_position:	Current entry position
>> + * @log_size:		Log space available
>> + */
>> +struct tcg2_event_log {
>> +	u8 *log;
>> +	u32 log_position;
<snip>
>> -		}
>> -	}
>> -
>> -	*pcr_banks = pcrs.count;
>> -
>> -	return 0;
>> -out:
>> -	return -1;
>> -}
>> -
>>   /**
>>    * __get_active_pcr_banks() - returns the currently active PCR banks
>>    *
>> @@ -638,7 +378,7 @@ static efi_status_t __get_active_pcr_banks(u32 *active_pcr_banks)
>>   	efi_status_t ret;
>>   	int err;
>>
>> -	ret = platform_get_tpm2_device(&dev);
>> +	ret = tcg2_platform_get_tpm2(&dev);
>>   	if (ret != EFI_SUCCESS)
>>   		goto out;
>>
> We can get rid of this entirely and just define the
> efi_tcg2_get_active_pcr_banks in the efi_tcg.c now.
> __get_active_pcr_banks == tcg2_get_active_pcr_banks with the only
> diffence being the udevice which is now an argument


Ack.


>
>> @@ -654,70 +394,6 @@ out:
>>   	return ret;
>>   }
>>
>>    * efi_tcg2_get_capability() - protocol capability information and state information
>>    *
>> @@ -759,7 +435,7 @@ efi_tcg2_get_capability(struct efi_tcg2_protocol *this,
>>   	capability->protocol_version.major = 1;
>>   	capability->protocol_version.minor = 1;
<snip>
>> +
>> +static int tcg2_log_append_check(struct tcg2_event_log *elog, u32 pcr_index,
>> +				 u32 event_type,
>> +				 struct tpml_digest_values *digest_list,
>> +				 u32 size, const u8 *event)
>> +{
>> +	u32 event_size;
>> +	u8 *log;
>> +
>> +	event_size = size + tcg2_event_get_size(digest_list);
>> +	if (elog->log_position + event_size > elog->log_size) {
>> +		printf("%s: log too large: %u + %u > %u\n", __func__,
>> +		       elog->log_position, event_size, elog->log_size);
>> +		return -ENOBUFS;
>> +	}
>> +
>> +	log = elog->log + elog->log_position;
>> +	elog->log_position += event_size;
>> +
>> +	tcg2_log_append(pcr_index, event_type, digest_list, size, event, log);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog)
>> +{
> I think we need to re-use efi_init_event_log here.  The reason is that on
> Arm devices TF-A is capable of constructing an eventlog and passing it
> along in memory.  That code takes that into account and tries to reuse the
> existing EventLog passed from previous boot stages.
>
> The main difference between the EFI function and this one
> is the allocated memory of the EventLog itself.  But even in this case, it
> would be better to tweak the EFI code and do
> create log -> Allocate EFI memory -> copy log and then use that for EFI


OK... I'll try and get that to work. I see some potential issues like 
the fact that EFI finds the platform event log differently.


>
>> +	struct tcg_efi_spec_id_event *ev;
>> +	struct tcg_pcr_event *log;
>> +	u32 event_size;
>> +	u32 count = 0;
>> +	u32 log_size;
>> +	u32 active;
>> +	u32 mask;
>> +	size_t i;
>> +	u16 len;
>> +	int rc;
>> +
>> +	rc = tcg2_get_active_pcr_banks(dev, &active);
>> +	if (rc)
>> +		return rc;
>> +
>> +	event_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes);
>> +	for (i = 0; i < ARRAY_SIZE(tcg2algos); ++i) {
>> +		mask = tpm2_algorithm_to_mask(tcg2algos[i]);
>> +
>> +		if (!(active & mask))
>> +			continue;
>> +
>> +		switch (tcg2algos[i]) {
>> +		case TPM2_ALG_SHA1:
>> +		case TPM2_ALG_SHA256:
>> +		case TPM2_ALG_SHA384:
>> +		case TPM2_ALG_SHA512:
>> +			count++;
>> +			break;
>> +		default:
>> +			continue;
>> +		}
>> +	}
>> +
>> +	event_size += 1 +
>> +		(sizeof(struct tcg_efi_spec_id_event_algorithm_size) * count);
>> +	log_size = offsetof(struct tcg_pcr_event, event) + event_size;
>> +
>> +	if (log_size > elog->log_size) {
>> +		printf("%s: log too large: %u > %u\n", __func__, log_size,
>> +		       elog->log_size);
>> +		return -ENOBUFS;
>> +	}
>> +
<snip>
>> +
>> +int tcg2_measure_event(struct udevice *dev, struct tcg2_event_log *elog,
>> +		       u32 pcr_index, u32 event_type, u32 size,
>> +		       const u8 *event)
>> +{
>> +	struct tpml_digest_values digest_list;
>> +	int rc;
>> +
>> +	rc = tcg2_create_digest(dev, event, size, &digest_list);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = tcg2_pcr_extend(dev, pcr_index, &digest_list);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return tcg2_log_append_check(elog, pcr_index, event_type, &digest_list,
>> +				     size, event);
>> +}
> There's a static efi_status_t tcg2_measure_event(...) left in efi_tcg.c
> which breaks compilation.  WE should just use the one you added in tpm-v2.c


Hmm. The EFI version does a slightly different way of event logging, so 
I'm not sure it's that simple. There is EFI specific stuff 
(ExitBootServices?) so I'm not sure it can be common... I can change the 
name in efi_tcg2.c to compile correctly.


Thanks,

Eddie


>
>> +
>> +	return 0;
>> +}
>> +
>>   u32 tpm2_dam_reset(struct udevice *dev, const char *pw, const ssize_t pw_sz)
>>   {
>>   	u8 command_v2[COMMAND_BUFFER_SIZE] = {
>> --
>> 2.31.1
>>
> Regards
> /Ilias