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

[Simon Glass]

Hi Eddie,

On Fri, 13 Jan 2023 at 07:46, Eddie James <eajames at linux.ibm.com> wrote:
>
>
> On 1/12/23 17:43, Simon Glass wrote:
> > Hi Eddie,
> >
> > On Thu, 12 Jan 2023 at 09:16, Eddie James <eajames at 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 at 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 at bootlin.com>
> >>    */
> >>
> >> +#include <asm/types.h>
> >> +#include <asm/io.h>
> > Please check header order:
> >
> > https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files
>
>
> Sure, however I did have a compile error with sandbox build due to
> missing phys_addr_t definition in asm/io.h...

Oh, perhaps that needs to be added to that header? But if it cannot be
fixed, you may need to split some part of an existing header into a
separate file, as was done with ofnode.h into ofnode_decl.h

>
>
> >
> >>   #include <common.h>
> >>   #include <dm.h>
> >> +#include <dm/of_access.h>
> >>   #include <tpm-common.h>
> >>   #include <tpm-v2.h>
> >>   #include <linux/bitops.h>
> >> +#include <linux/unaligned/be_byteshift.h>
> >> +#include <linux/unaligned/generic.h>
> >> +#include <linux/unaligned/le_byteshift.h>
> >> +#include <u-boot/sha1.h>
> >> +#include <u-boot/sha256.h>
> >> +#include <u-boot/sha512.h>
> >> +#include <version_string.h>
> >>   #include "tpm-utils.h"
> >>
> > [..]
> >
> >> +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.

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