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

Ilias Apalodimas
2023-01-26T07:51:34Z

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 <eajames@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.apalodimas@linaro.org/

Thanks
/Ilias