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

Simon Glass
2023-01-23T22:25:58Z

Hi Ilias,

On Mon, 16 Jan 2023 at 03:51, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> [...]
>
> > > >>
> > > > [..]
> > > >
> > > >> +static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog)
> > > >> +{
> > > >> + struct tcg_efi_spec_id_event *ev;
> > > > We cannot add EFI things to generic TPM code.
> > >
> > >
> > > Ah, this is NOT an EFI thing even though it is named as such. Please see
> > > https://trustedcomputinggroup.org/wp-content/uploads/TCG_ServerManagDomainFWProfile_r1p00_pub.pdf
> > > section 9 and
> > > https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_Specific_Platform_Profile_for_TPM_2p0_1p04_PUBLIC.pdf
> > > section 9
> > >
> > >
> > > Neither of these documents are specific to EFI.
> >
> > OK, then please drop efi_ from the identifiers. It is terribly confusing.
>
> Why? The terrible confusing part would be trying to read code not
> named after the spec because you don't like EFI. When looking at code
> that's documented by a spec it's far easier to keep the naming as
> close as possible, so I'd prefer leaving it as is.

Because this is not an EFI spec, is it? What am I missing?

Regards,
Simon

>
> Regards
> /Ilias
> >
> > >
> > >
> > > >
> > > >> + 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;
> > > >> + }
> > > >> +
> > > >> + log = (struct tcg_pcr_event *)elog->log;
> > > >> + put_unaligned_le32(0, &log->pcr_index);
> > > >> + put_unaligned_le32(EV_NO_ACTION, &log->event_type);
> > > >> + memset(&log->digest, 0, sizeof(log->digest));
> > > >> + put_unaligned_le32(event_size, &log->event_size);
> > > >> +
> > > >> + ev = (struct tcg_efi_spec_id_event *)log->event;
> > > >> + strlcpy((char *)ev->signature, TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03,
> > > > Same with all of this.
> > > >
> > > >> + sizeof(ev->signature));
> > > >> + put_unaligned_le32(0, &ev->platform_class);
> > > >> + ev->spec_version_minor = TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2;
> > > >> + ev->spec_version_major = TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2;
> > > >> + ev->spec_errata = TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_ERRATA_TPM2;
> > > > I'm not quite sure what is going on here...is this log in a format
> > > > defined by the EFI spec? What if we are not using EFI? How would a
> > > > different format be used?
> > > >
> > > > Put another way, people using a TPM should not pull in EFI things just
> > > > to do that.
> > >
> > >
> > > Agreed, however the EFI spec is not involved. These specifications and
> > > structures are general to any boot measurement. I would guess EFI was
> > > the first to do this and therefore defined some structures that the TCG
> > > re-used when writing the specs.
> >
> > Regards,
> > Simon