Hi Eddie,

Thanks for the cleanup! Unfortunately this doesn't compile with EFI
selected, but in general it looks pretty good.

On Wed, Jan 25, 2023 at 11:18:06AM -0600, Eddie James 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 <eaja...@linux.ibm.com>
> ---
>  include/efi_tcg2.h        |  44 --
>  include/tpm-v2.h          | 254 ++++++++++
>  lib/efi_loader/efi_tcg2.c | 975 +++-----------------------------------
>  lib/tpm-v2.c              | 799 +++++++++++++++++++++++++++++++
>  4 files changed, 1129 insertions(+), 943 deletions(-)
>
>       HR_NV_INDEX             = TPM_HT_NV_INDEX << HR_SHIFT,
>  };
>

[...]

>       }
>
> +}
> +
> +int tcg2_measure_data(struct udevice *dev, struct tcg2_event_log *elog,
> +                   u32 pcr_index, u32 size, const u8 *data, u32 event_type,
> +                   u32 event_size, const u8 *event)
> +{
> +     struct tpml_digest_values digest_list;
> +     int rc;
> +
> +     rc = tcg2_create_digest(dev, data, size, &digest_list);
> +     if (rc)
> +             return rc;
> +
> +     rc = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> +     if (rc)
> +             return rc;
> +
> +     return tcg2_log_append_check(elog, pcr_index, event_type, &digest_list,
> +                                  event_size, event);
> +}
> +
> +int tcg2_measure_event(struct udevice *dev, struct tcg2_event_log *elog,
> +                    u32 pcr_index, u32 event_type, u32 size,
> +                    const u8 *event)
> +{
> +     struct tpml_digest_values digest_list;
> +     int rc;
> +
> +     rc = tcg2_create_digest(dev, event, size, &digest_list);
> +     if (rc)
> +             return rc;
> +
> +     rc = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> +     if (rc)
> +             return rc;
> +
> +     return tcg2_log_append_check(elog, pcr_index, event_type, &digest_list,
> +                                  size, event);
> +}

The only difference between these 2 is the measured data.  Can't we make
one function?  If data = NULL we could just measure the event

> +
> +int tcg2_init_log(struct udevice *dev, struct tcg2_event_log *elog)
> +{
> +     int rc;
> +
> +     rc = tcg2_platform_get_log(dev, (void **)&elog->log, &elog->log_size);
> +     if (rc)
> +             return rc;
> +
> +     elog->log_position = 0;
> +     elog->found = false;
> +     rc = tcg2_log_parse(dev, elog);
> +     if (rc)
> +             return rc;
> +
> +     if (!elog->found) {
> +             rc = tcg2_log_init(dev, elog);
> +             if (rc)
> +                     return rc;

You can get rid of this if and just return rc on the function

> +     }
> +
> +     return 0;
> +}
> +
> +int tcg2_measurement_init(struct udevice **dev, struct tcg2_event_log *elog)
> +{
> +     int rc;
> +
> +     rc = tcg2_platform_get_tpm2(dev);
> +     if (rc)
> +             return rc;
> +
> +     rc = tpm_init(*dev);
> +     if (rc)
> +             return rc;
> +
> +     rc = tpm2_startup(*dev, TPM2_SU_CLEAR);
> +     if (rc) {
> +             tcg2_platform_startup_error(*dev, rc);
> +             return rc;
> +     }
> +
> +     rc = tpm2_self_test(*dev, TPMI_YES);
> +     if (rc)
> +             printf("%s: self test error, continuing.\n", __func__);

This is the correct init sequence of the TPM.  I was trying to solve
something similar a few days ago [0].  I haven't merged that patch yet,
but I will send a new version that also calls tpm_init() and doesn't exit
on -EBUSY.  Can you use that patch instead of open coding the init
sequence?
Also we should be bailing out on selftest errors?

The problem here is that with U-Boot's lazy binding we might need to init
the TPM in other subsystems and at the very least we can have a function
doing that for us reliably.

> +
> +     rc = tcg2_init_log(*dev, elog);
> +     if (rc) {
> +             tcg2_measurement_term(*dev, elog, true);
> +             return rc;
> +     }
> +
> +     rc = tcg2_measure_event(*dev, elog, 0, EV_S_CRTM_VERSION,
> +                             strlen(version_string) + 1,
> +                             (u8 *)version_string);
> +     if (rc) {
> +             tcg2_measurement_term(*dev, elog, true);
> +             return rc;
> +     }
> +
> +     return 0;
> +}
> +
> +void tcg2_measurement_term(struct udevice *dev, struct tcg2_event_log *elog,
> +                        bool error)
> +{
> +     u32 event = error ? 0x1 : 0xffffffff;
> +     int i;
> +
> +     for (i = 0; i < 8; ++i)
> +             tcg2_measure_event(dev, elog, i, EV_SEPARATOR, sizeof(event),
> +                                (const u8 *)&event);

Is the separator event at the end of the eventlog described in the spec?

> +
> +     if (elog->log)
> +             unmap_physmem(elog->log, MAP_NOCACHE);
> +}
> +
> +__weak int tcg2_platform_get_log(struct udevice *dev, void **addr, u32 *size)
> +{
> +     const __be32 *addr_prop;
> +     const __be32 *size_prop;
> +     int asize;
> +     int ssize;
> +
> +     *addr = NULL;
> +     *size = 0;
> +
> +     addr_prop = dev_read_prop(dev, "linux,sml-base", &asize);
> +     if (!addr_prop)
> +             addr_prop = dev_read_prop(dev, "tpm_event_log_addr", &asize);
> +     size_prop = dev_read_prop(dev, "linux,sml-size", &ssize);
> +     if (!size_prop)
> +             size_prop = dev_read_prop(dev, "tpm_event_log_size", &ssize);
> +     if (addr_prop && size_prop) {
> +             u64 a = of_read_number(addr_prop, asize / sizeof(__be32));
> +             u64 s = of_read_number(size_prop, ssize / sizeof(__be32));
> +
> +             *addr = map_physmem(a, s, MAP_NOCACHE);
> +             *size = (u32)s;
> +     } else {
> +             struct ofnode_phandle_args args;
> +             phys_addr_t a;
> +             phys_size_t s;
> +
> +             if (dev_read_phandle_with_args(dev, "memory-region", NULL, 0,
> +                                            0, &args))
> +                     return -ENODEV;
> +
> +             a = ofnode_get_addr_size(args.node, "reg", &s);
> +             if (a == FDT_ADDR_T_NONE)
> +                     return -ENOMEM;
> +
> +             *addr = map_physmem(a, s, MAP_NOCACHE);
> +             *size = (u32)s;
> +     }
> +
> +     return 0;
> +}
> +
> +__weak int tcg2_platform_get_tpm2(struct udevice **dev)
> +{
> +     for_each_tpm_device(*dev) {
> +             if (tpm_get_version(*dev) == TPM_V2)
> +                     return 0;
> +     }
> +
> +     return -ENODEV;
> +}
> +
> +__weak void tcg2_platform_startup_error(struct udevice *dev, int rc) {}
> +
>  u32 tpm2_startup(struct udevice *dev, enum tpm2_startup_types mode)
>  {
>       const u8 command_v2[12] = {
> @@ -342,6 +1016,131 @@ u32 tpm2_get_capability(struct udevice *dev, u32 
> capability, u32 property,
>       return 0;
>  }
>
> +static int tpm2_get_num_pcr(struct udevice *dev, u32 *num_pcr)
> +{
> +     u8 response[(sizeof(struct tpms_capability_data) -
> +             offsetof(struct tpms_capability_data, data))];
> +     u32 properties_offset =
> +             offsetof(struct tpml_tagged_tpm_property, tpm_property) +
> +             offsetof(struct tpms_tagged_property, value);
> +     u32 ret;
> +
> +     memset(response, 0, sizeof(response));
> +     ret = tpm2_get_capability(dev, TPM2_CAP_TPM_PROPERTIES,
> +                               TPM2_PT_PCR_COUNT, response, 1);
> +     if (ret)
> +             return ret;
> +
> +     *num_pcr = get_unaligned_be32(response + properties_offset);
> +     if (*num_pcr > TPM2_MAX_PCRS) {
> +             printf("%s: too many pcrs: %u\n", __func__, *num_pcr);
> +             return -E2BIG;
> +     }
> +
> +     return 0;
> +}
> +

[...]

[0] 
https://lore.kernel.org/u-boot/20230125144851.532154-1-ilias.apalodi...@linaro.org/

Thanks
/Ilias

Reply via email to