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

[Eddie James <eajames@linux.ibm.com>]

On 1/26/23 01:51, Ilias Apalodimas wrote:
> Hi Eddie,
>
> Thanks for the cleanup! Unfortunately this doesn't compile with EFI
> selected, but in general it looks pretty good.


Thanks, yes I forgot to remove tcg2_pcr_read


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


Ok, yes that should work. I'll probably add a macro for 
tcg2_measure_event that fills in NULL and 0 for data and size for 
convenience.


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


Ack.


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


Yes I'd be happy to use it. I'll just base my series on yours.


> Also we should be bailing out on selftest errors?'


Maybe so, yes. Whatever behavior your auto startup function implements 
will be fine, I'm sure.


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


Yes. 
https://trustedcomputinggroup.org/wp-content/uploads/TCG_ServerManagDomainFWProfile_r1p00_pub.pdf


See section 7.1.1


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