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

[Ilias Apalodimas]

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?

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

[...]

> +	*((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()?

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

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

 [...]

 Cheers
 /Ilias