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

Ilias Apalodimas
2023-01-13T07:08:02Z

Hi Simon

On Fri, 13 Jan 2023 at 01:43, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Eddie,
>
> On Thu, 12 Jan 2023 at 09:16, Eddie James <eajames@linux.ibm.com> wrote:
> >
> > Add TPM2 functions to support boot measurement. This includes
> > starting up the TPM, initializing/appending the event log, and
> > measuring the U-Boot version. Much of the code was used in the
> > EFI subsystem, so remove it there and use the common functions.
> >
> > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > ---
> > include/efi_tcg2.h | 44 ---
> > include/tpm-v2.h | 211 ++++++++++++
> > lib/efi_loader/efi_tcg2.c | 362 +------------------
> > lib/tpm-v2.c | 708 ++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 938 insertions(+), 387 deletions(-)
>
> [..]
>
> > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > index 697b982e07..00e1b04d74 100644
> > --- a/lib/tpm-v2.c
> > +++ b/lib/tpm-v2.c
> > @@ -4,13 +4,597 @@
> > * Author: Miquel Raynal <miquel.raynal@bootlin.com>
> > */
> >

[...]

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

Yes. The TCG eventlog and everything Eddie is trying to add are
defined by an extension to the EFI spec. I am unaware of any forms of
measurement (with a TPM). The eventlong is purely a software
construct. The TPM PCR extension involves taking measurements and
talking to the hardware. Nothing prevents you from doing this outside
EFI. What I am curious about is how these measurements are used by
the OS in Eddie's case.

When booting with EFI, the kernel calls the GetEventlog callback and
stores the event log in memory. What happens to bootm?

>
> Put another way, people using a TPM should not pull in EFI things just
> to do that.
>
> I'm just not quite sure of the best approach here...

The extensions to the EFI spec defines
1. What the eventlog format looks like
2. Functions to configure the TPM pcr banks, retrieve the eventlog
from memory etc.

Regards
/Ilias
>
> Regards,
> Simon