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