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

[Simon Glass]

Hi Ilias,

On Mon, 16 Jan 2023 at 03:51, Ilias Apalodimas
<ilias.apalodimas at 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