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

Eddie James
2023-03-03T19:17:56Z

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