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

Simon Glass
2023-01-12T23:44:04Z

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>
> */
>
> +#include <asm/types.h>
> +#include <asm/io.h>

Please check header order:

https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files

> #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.

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

I'm just not quite sure of the best approach here...

Regards,
Simon