[PATCH v7 3/6] tpm: Support boot measurements

[Eddie James]

On 3/2/23 14:22, Ilias Apalodimas wrote:
> Hi Eddie,
>
> I found the issue.  I still think we could squeeze things even more in our
> abstraction.  Specifically the measure_event() tcg2_agile_log_append()
> contain some efi specific bits and I am trying to figure out if we can make
> those more generic.  However, that's not a show stopper for me.
>
> [...]
>
>> +int tcg2_init_log(struct udevice *dev, struct tcg2_event_log *elog);
> We have tcg2_init_log() and tcg2_log_init().  This is a bit confusing when
> reading the code.
> Since tcg2_log_init() is actually initializing the EventLog can we do
> tcg2_init_log -> tcg2_prepare_log_buf or something along those lines?


Sure, sounds good.


>
>> +
>> +/**
>> + * Begin measurements.
>> + *
>> + * @dev		TPM device
> [...]
>
>> +static int tcg2_log_parse(struct udevice *dev, struct tcg2_event_log *elog)
>> +{
>> +	struct tpml_digest_values digest_list;
>> +	struct tcg_efi_spec_id_event *event;
>> +	struct tcg_pcr_event *log;
>> +	u32 calc_size;
>> +	u32 active;
>> +	u32 count;
>> +	u32 evsz;
>> +	u32 mask;
>> +	u16 algo;
>> +	u16 len;
>> +	int rc;
>> +	u32 i;
>> +	u16 j;
>> +
>> +	if (elog->log_size <= offsetof(struct tcg_pcr_event, event))
>> +		return 0;
>> +
>> +	log = (struct tcg_pcr_event *)elog->log;
>> +	if (get_unaligned_le32(&log->pcr_index) != 0 ||
>> +	    get_unaligned_le32(&log->event_type) != EV_NO_ACTION)
>> +		return 0;
>> +
>> +	for (i = 0; i < sizeof(log->digest); i++) {
>> +		if (log->digest[i])
>> +			return 0;
>> +	}
>> +
>> +	evsz = get_unaligned_le32(&log->event_size);
>> +	if (evsz < offsetof(struct tcg_efi_spec_id_event, digest_sizes) ||
>> +	    evsz + offsetof(struct tcg_pcr_event, event) > elog->log_size)
>> +		return 0;
>> +
>> +	event = (struct tcg_efi_spec_id_event *)log->event;
>> +	if (memcmp(event->signature, TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03,
>> +		   sizeof(TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03)))
>> +		return 0;
>> +
>> +	if (event->spec_version_minor != TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2 ||
>> +	    event->spec_version_major != TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2)
>> +		return 0;
>> +
>> +	count = get_unaligned_le32(&event->number_of_algorithms);
>> +	if (count > ARRAY_SIZE(tpm2_supported_algorithms))
>> +		return 0;
>> +
>> +	calc_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes) +
>> +		(sizeof(struct tcg_efi_spec_id_event_algorithm_size) * count) +
>> +		1;
>> +	if (evsz != calc_size)
>> +		return 0;
>> +
>> +	rc = tcg2_get_active_pcr_banks(dev, &active);
>> +	if (rc)
>> +		return rc;
> There was a check here in the previous version which I can't find. The
> previous stage bootloader is creating an EventLog.  Since it can't control
> the TPM the pcr banks that end up in the EventLog are defined at compile
> time.  This isn't ideal, but we have 2 options here:
> 1. Check the hardware active PCR banks and report an error if there's a
> mismatch (which is what the older version did)
> 2. Add the missing function of re-configuring the active banks to
> whatever the previous bootloader tells us.
>
> Obviously (2) is a better option, but I am fine if we just report an error
> for now.


Yes I found it, and I will add that.


>
> [...]
>
>> +	*((u8 *)ev + (event_size - 1)) = 0;
>> +	elog->log_position = log_size;
>> +
>> +	return 0;
>> +}
>> +
>> +static int tcg2_log_find_end(struct tcg2_event_log *elog, struct udevice *dev,
> Can we find a better name for this? This basically replays an eventlog we
> inherited from a previous stage boot loader into the TPM.
> So something like tcg2_replay_eventlog()?


Sure.


>
>> +			     struct tpml_digest_values *digest_list)
>> +{
>> +	const u32 offset = offsetof(struct tcg_pcr_event2, digests) +
>> +		offsetof(struct tpml_digest_values, digests);
>> +	u32 event_size;
>> +	u32 count;
>> +	u16 algo;
>> +	u32 pcr;
>> +	u32 pos;
>> +	u16 len;
>> +	u8 *log;
>> +	int rc;
>> +	u32 i;
>> +
>> +	while (elog->log_position + offset < elog->log_size) {
>> +		log = elog->log + elog->log_position;
>> +
>> +		pos = offsetof(struct tcg_pcr_event2, pcr_index);
>> +		pcr = get_unaligned_le32(log + pos);
>> +		pos = offsetof(struct tcg_pcr_event2, event_type);
>> +		if (!get_unaligned_le32(log + pos))
>> +			return 0;
> isn't this an actual error ?


Good point, and all below too. I will return errors there.


>
>> +
>> +		pos = offsetof(struct tcg_pcr_event2, digests) +
>> +			offsetof(struct tpml_digest_values, count);
>> +		count = get_unaligned_le32(log + pos);
>> +		if (count > ARRAY_SIZE(tpm2_supported_algorithms) ||
>> +		    (digest_list->count && digest_list->count != count))
>> +			return 0;
> ditto
>
>> +
>> +		pos = offsetof(struct tcg_pcr_event2, digests) +
>> +			offsetof(struct tpml_digest_values, digests);
>> +		for (i = 0; i < count; ++i) {
>> +			pos += offsetof(struct tpmt_ha, hash_alg);
>> +			if (elog->log_position + pos + sizeof(u16) >=
>> +			    elog->log_size)
>> +				return 0;
> ditto
>
>> +
>> +			algo = get_unaligned_le16(log + pos);
>> +			pos += offsetof(struct tpmt_ha, digest);
>> +			switch (algo) {
>> +			case TPM2_ALG_SHA1:
>> +			case TPM2_ALG_SHA256:
>> +			case TPM2_ALG_SHA384:
>> +			case TPM2_ALG_SHA512:
>> +				len = tpm2_algorithm_to_len(algo);
>> +				break;
>> +			default:
>> +				return 0;
> I think we should return errors in this case.  Otherwise we'll end up with
> an incomplete view of either the TPM or the extended PCRs
>
>> +			}
>> +
>> +			if (digest_list->count) {
>> +				if (algo != digest_list->digests[i].hash_alg ||
>> +				    elog->log_position + pos + len >=
>> +				    elog->log_size)
>> +					return 0;
>> +
>> +				memcpy(digest_list->digests[i].digest.sha512,
>> +				       log + pos, len);
>> +			}
>> +
>> +			pos += len;
>> +		}
>> +
>> +		if (elog->log_position + pos + sizeof(u32) >= elog->log_size)
>> +			return 0;
>> +
>> +		event_size = get_unaligned_le32(log + pos);
>> +		pos += event_size + sizeof(u32);
>> +		if (elog->log_position + pos >= elog->log_size)
> This is off by one and as a result you skip replaying the last
> event. This should be
> if (elog->log_position + pos > elog->log_size)


Nice catch, thank you! I'll fix it.


Thanks for your review! v8 coming soon.

Eddie


>      ....
>
>   [...]
>
>   Cheers
>   /Ilias