[PATCH 3/3 v2] efi: Add basic EFI_TCG2_PROTOCOL support

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Heinrich, 

[...]
> > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> > new file mode 100644
> > index 000000000000..9e7b85db058d
> > --- /dev/null
> > +++ b/include/efi_tcg2.h
> > @@ -0,0 +1,91 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2020, Linaro Limited
> 
> Please, add at least one line describing what this include is about.
> 

Ok 

> > + */
> > +
> > +#if !defined _EFI_TCG2_PROTOCOL_H_
> > +#define _EFI_TCG2_PROTOCOL_H_
> > +
> > +#include <tpm-v2.h>
> > +
> > +#define EFI_TCG2_PROTOCOL_GUID \
> > +	EFI_GUID(0x607f766c, 0x7455, 0x42be, 0x93, \
> > +		 0x0b, 0xe4, 0xd7, 0x6d, 0xb2, 0x72, 0x0f)
> > +
> > +/* TPMV2 only */
> > +#define TCG2_EVENT_LOG_FORMAT_TCG_2 0x00000002
> > +
> > +/* SHA1, SHA256, SHA384, SHA512, TPM_ALG_SM3_256 */
> > +#define MAX_HASH_COUNT 5
> > +/* Algorithm Registry */
> > +#define EFI_TCG2_BOOT_HASH_ALG_SHA1    0x00000001
> > +#define EFI_TCG2_BOOT_HASH_ALG_SHA256  0x00000002
> > +#define EFI_TCG2_BOOT_HASH_ALG_SHA384  0x00000004
> > +#define EFI_TCG2_BOOT_HASH_ALG_SHA512  0x00000008
> > +#define EFI_TCG2_BOOT_HASH_ALG_SM3_256 0x00000010
> > +
> > +typedef u32 efi_tcg_event_log_bitmap;
> > +typedef u32 efi_tcg_event_log_format;
> > +typedef u32 efi_tcg_event_algorithm_bitmap;
> > +
> > +struct efi_tcg2_version {
> > +	u8 major;
> > +	u8 minor;
> > +};
> > +
> > +struct efi_tcg2_event_header {
> > +	u32 header_size;
> > +	u16 header_version;
> > +	u32 pcr_index;
> > +	u32 event_type;
> > +} __packed;
> > +
> > +struct efi_tcg2_event {
> > +	u32 size;
> > +	struct efi_tcg2_event_header header;
> > +	u8 event[];
> > +} __packed;
> > +
> > +struct efi_tcg2_boot_service_capability {
> > +	u8 size;
> > +	struct efi_tcg2_version structure_version;
> > +	struct efi_tcg2_version protocol_version;
> > +	efi_tcg_event_algorithm_bitmap hash_algorithm_bitmap;
> > +	efi_tcg_event_log_bitmap supported_event_logs;
> > +	bool tpm_present_flag;
> 
> The UEFI spec defines BOOLEAN as "1-byte value".
> The C99 standard does not require that _Bool has size 1 byte.
> Wouldn't it be wiser to use u8?

Yes, good catch

> 
> Other structures in this patch are packed. Has this structure to be
> packed too? The EFI ProtocolSpecification has this sentence:
> "All structures defined in this specification are packed, except where
> explicitly otherwise defined."
> 

Nop this is expliticly required to be not packed 
https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
Section 6.4 

[...]
> >
> > +config EFI_TCG2_PROTOCOL
> > +	bool "EFI_TCG2_PROTOCOL support"
> > +	depends on TPM_V2
> > +	default n
> 
> default n is superfluous.
> 

Ok

> > +	help
> > +	  Provide a EFI_TCG2_PROTOCOL implementation using the TPM hardware
> > +	  of the platform.
> > +
> >  config EFI_LOAD_FILE2_INITRD
> >  	bool "EFI_FILE_LOAD2_PROTOCOL for Linux initial ramdisk"
> >  	default n
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index 8892fb01e125..cd4b252a417c 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -53,6 +53,7 @@ obj-$(CONFIG_NET) += efi_net.o
> >  obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
> >  obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
> >  obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
> > +obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
> >  obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
> >  obj-y += efi_signature.o
> >
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 45226c5c1a53..e206b60bb82c 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -156,6 +156,13 @@ efi_status_t efi_init_obj_list(void)
> >  		if (ret != EFI_SUCCESS)
> >  			goto out;
> >  	}
> > +
> > +	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > +		ret = efi_tcg2_register();
> > +		if (ret != EFI_SUCCESS)
> > +			goto out;
> > +	}
> > +
> >  	/* Initialize variable services */
> >  	ret = efi_init_variables();
> >  	if (ret != EFI_SUCCESS)
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > new file mode 100644
> > index 000000000000..b735f3f2a83d
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -0,0 +1,493 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2020, Linaro Limited
> 
> Please, add at least one line describing what this module is about.
> 

Ok

> > + */
> > +
> > +#define LOG_CATEGORY LOGC_EFI
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <efi_loader.h>
> > +#include <efi_tcg2.h>
> > +#include <log.h>
> > +#include <tpm-v2.h>
> > +#include <linux/unaligned/access_ok.h>
> > +#include <linux/unaligned/generic.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/*
> > + * When requesting TPM2_CAP_TPM_PROPERTIES the value is on a standard offset.
> > + * Since the current tpm2_get_capability() response buffers starts at
> > + * 'union tpmu_capabilities data' of 'struct tpms_capability_data', calculate the
> > + * response size and offset once for all consumers
> 
> Please, restrict your code to 80 characters per line.
> 

Ok

> > + */
> > +#define TPM2_RESPONSE_BUFFER_SIZE (sizeof(struct tpms_capability_data) - \
> > +				   offsetof(struct tpms_capability_data, data))
> > +#define properties_offset (offsetof(struct tpml_tagged_tpm_property, tpm_property) + \
> > +			   offsetof(struct tpms_tagged_property, value))
> > +
> > +const efi_guid_t efi_guid_tcg2_protocol = EFI_TCG2_PROTOCOL_GUID;
> > +
> > +/**
> > + * platform_get_tpm_device() - retrieve TPM device
> > + *
> > + * This function retrieves the udevice implementing a TPM
> > + *
> > + * This function may be overridden if special initialization is needed.
> > + *
> > + * @dev:	udevice
> > + * Return:	status code
> > + */
> > +__weak efi_status_t platform_get_tpm2_device(struct udevice **dev)
> > +{
> > +	int ret;
> > +	struct udevice *devp;
> > +
> > +	ret = uclass_get_device(UCLASS_TPM, 0, &devp);
> > +	if (ret)
> > +		return EFI_DEVICE_ERROR;
> > +
> > +	*dev = devp;
> > +
> > +	return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * tpm2_get_max_command_size() - TPM2 command to get the supported max command size
> > + *
> > + * @dev:		TPM device
> > + * @max_command_size:	output buffer for the size
> > + *
> > + * Return: 0 on success -1 on error
> > + */
> > +static int tpm2_get_max_command_size(struct udevice *dev, u16 *max_command_size)
> > +{
> > +	u8 response[TPM2_RESPONSE_BUFFER_SIZE];
> > +	u32 ret;
> > +
> > +	memset(response, 0, sizeof(response));
> 
> Why is memset() required here?
> Do you want avoid the TPM chip to be able to see your bytes from the stack?
> Wouldn't it preferable that tpm2_get_capability() takes care of zeroing
> out buffers?

I generally zero out stack initialized arrays for various reasons. 
tpm2_get_capability only gets one argument (void *buf) not the length. 
Can we change that when specific tpm2_get_capability() patches are sent?

> 
> > +	ret = tpm2_get_capability(dev, TPM2_CAP_TPM_PROPERTIES,
> > +				  TPM2_PT_MAX_COMMAND_SIZE, response, 1);
> 
> Here you pass TPM2_PT_MAX_COMMAND_SIZE but the buffer is of size
> TPM2_RESPONSE_BUFFER_SIZE. How should tpm2_get_capability() guess the
> size of the response?

I am not sure I am following. This asks the TPMv2 to respond (in a response buffer), of
it's maximum allowed command size. The buffer size is defined from the standard.

#define TPM2_RESPONSE_BUFFER_SIZE (sizeof(struct tpms_capability_data) - \
                                   offsetof(struct tpms_capability_data, data))

> 
> > +	if (ret)
> > +		return -1;
> > +
> > +	*max_command_size = (uint16_t)get_unaligned_be32(response + properties_offset);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * tpm2_get_max_response_size() - TPM2 command to get the supported max response size
> > + *
> > + * @dev:		TPM device
> > + * @max_response_size:	output buffer for the size
> > + *
> > + * Return: 0 on success -1 on error
> > + */
> > +static int tpm2_get_max_response_size(struct udevice *dev, u16 *max_response_size)
> > +{
> > +	u8 response[TPM2_RESPONSE_BUFFER_SIZE];
> > +	u32 ret;
> > +
> > +	memset(response, 0, sizeof(response));
> 
> Same here. Moving memset to tpm2_get_capability already would save code
> size.
> 
> > +	ret = tpm2_get_capability(dev, TPM2_CAP_TPM_PROPERTIES,
> > +				  TPM2_PT_MAX_RESPONSE_SIZE, response, 1);
> > +	if (ret)
> > +		return -1;
> > +
> > +	*max_response_size = (uint16_t)get_unaligned_be32(response + properties_offset);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * tpm2_get_manufacturer_id() - Issue a TPM2 command to get the manufacturer ID
> > + *
> > + * @dev:		TPM device
> > + * @manufacturer_id:	output buffer for the id
> > + *
> > + * Return: 0 on success -1 on error
> > + */
> > +static int tpm2_get_manufacturer_id(struct udevice *dev, u32 *manufacturer_id)
> > +{
> > +	u8 response[TPM2_RESPONSE_BUFFER_SIZE];
> > +	u32 ret;
> > +
> > +	memset(response, 0, sizeof(response));
> 
> Again
> 
> > +	ret = tpm2_get_capability(dev, TPM2_CAP_TPM_PROPERTIES,
> > +				  TPM2_PT_MANUFACTURER, response, 1);
> > +	if (ret)
> > +		return -1;
> > +
> > +	*manufacturer_id = get_unaligned_be32(response + properties_offset);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * tpm2_get_num_pcr() - Issue a TPM2 command to get the number of PCRs
> > + *
> > + * @dev:		TPM device
> > + * @manufacturer_id:	output buffer for the number
> > + *
> > + * Return: 0 on success -1 on error
> > + */
> > +static int tpm2_get_num_pcr(struct udevice *dev, u32 *num_pcr)
> > +{
> > +	u8 response[TPM2_RESPONSE_BUFFER_SIZE];
> > +	u32 ret;
> > +
> > +	memset(response, 0, sizeof(response));
> 
> Again
> 
> > +	ret = tpm2_get_capability(dev, TPM2_CAP_TPM_PROPERTIES,
> > +				  TPM2_PT_PCR_COUNT, response, 1);
> > +	if (ret)
> > +		return -1;
> > +
> > +	*num_pcr = get_unaligned_be32(response + properties_offset);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * is_active_pcr() - Check if a supported algorithm is active
> > + *
> > + * @dev:		TPM device
> > + * @selection:		struct of PCR information
> > + *
> > + * Return: true if PCR is active
> > + */
> > +bool is_active_pcr(struct tpms_pcr_selection *selection)
> > +{
> > +	int i;
> > +	/*
> > +	 * check the pcr_select. If at least one of the PCRs supports the algorithm
> > +	 * add it on the active ones
> > +	 */
> > +	for (i = 0; i < selection->size_of_select; i++) {
> > +		if (selection->pcr_select[i])
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +/**
> > + * tpm2_get_pcr_info() - Issue a TPM2 command to get the supported, active PCRs and number of banks
> > + *
> > + * @dev:		TPM device
> > + * @supported_pcr:	bitmask with the algorithms supported
> > + * @active_pcr:		bitmask with the active algorithms
> > + * @pcr_banks:		number of PCR banks
> > + *
> > + * Return: 0 on success -1 on error
> 
> %s/ -1/, -1/
> 

Ok 

> > + */
> > +static int tpm2_get_pcr_info(struct udevice *dev, u32 *supported_pcr, u32 *active_pcr,
> > +			     u32 *pcr_banks)
> > +{
> > +	u8 response[TPM2_RESPONSE_BUFFER_SIZE];
> > +	struct tpml_pcr_selection pcrs;
> > +	u32 ret, num_pcr;
> > +	int i, tpm_ret;
> > +
> > +	memset(response, 0, sizeof(response));
> 
> Again
> 
> > +	ret = tpm2_get_capability(dev, TPM2_CAP_PCRS, 0, response, 1);
> > +	if (ret)
> > +		return -1;
> > +
> > +	pcrs.count = get_unaligned_be32(response);
> > +	if (pcrs.count > MAX_HASH_COUNT || pcrs.count < 1)
> > +		return -1;
> > +
> > +	tpm_ret = tpm2_get_num_pcr(dev, &num_pcr);
> > +	if (tpm_ret)
> > +		return -1;
> > +	for (i = 0; i < pcrs.count; i++) {
> > +		/*
> > +		 * Definition of TPMS_PCR_SELECTION Structure
> > +		 * hash: u16
> > +		 * size_of_select: u8
> > +		 * pcr_select: u8 array
> > +		 *
> > +		 * The offsets depend on the number of the device PCRs
> > +		 * so we have to calculate them based on that
> > +		 */
> > +		u32 hash_offset = offsetof(struct tpml_pcr_selection, selection) +
> > +			i * offsetof(struct tpms_pcr_selection, pcr_select) +
> > +			i * ((num_pcr + 7) / 8);
> > +		u32 size_select_offset =
> > +			hash_offset + offsetof(struct tpms_pcr_selection, size_of_select);
> > +		u32 pcr_select_offset =
> > +			hash_offset + offsetof(struct tpms_pcr_selection, pcr_select);
> > +
> > +		pcrs.selection[i].hash = get_unaligned_be16(response + hash_offset);
> > +		pcrs.selection[i].size_of_select =
> > +			__get_unaligned_be(response + size_select_offset);
> > +		/* copy the array of pcr_select */
> > +		memcpy(pcrs.selection[i].pcr_select, response + pcr_select_offset,
> > +		       pcrs.selection[i].size_of_select);
> > +	}
> > +
> > +	for (i = 0; i < pcrs.count; i++) {
> > +		switch (pcrs.selection[i].hash) {
> > +		case TPM2_ALG_SHA1:
> > +			 *supported_pcr |= EFI_TCG2_BOOT_HASH_ALG_SHA1;
> > +			if (is_active_pcr(&pcrs.selection[i]))
> > +				*active_pcr |= EFI_TCG2_BOOT_HASH_ALG_SHA1;
> > +			break;
> > +		case TPM2_ALG_SHA256:
> > +			*supported_pcr |= EFI_TCG2_BOOT_HASH_ALG_SHA256;
> > +			if (is_active_pcr(&pcrs.selection[i]))
> > +				*active_pcr |= EFI_TCG2_BOOT_HASH_ALG_SHA256;
> > +			break;
> > +		case TPM2_ALG_SHA384:
> > +			*supported_pcr |= EFI_TCG2_BOOT_HASH_ALG_SHA384;
> > +			if (is_active_pcr(&pcrs.selection[i]))
> > +				*active_pcr |= EFI_TCG2_BOOT_HASH_ALG_SHA384;
> > +			break;
> > +		case TPM2_ALG_SHA512:
> > +			*supported_pcr |= EFI_TCG2_BOOT_HASH_ALG_SHA512;
> > +			if (is_active_pcr(&pcrs.selection[i]))
> > +				*active_pcr |= EFI_TCG2_BOOT_HASH_ALG_SHA512;
> > +			break;
> > +		case TPM2_ALG_SM3_256:
> > +			*supported_pcr |= EFI_TCG2_BOOT_HASH_ALG_SM3_256;
> > +			if (is_active_pcr(&pcrs.selection[i]))
> > +				*active_pcr |= EFI_TCG2_BOOT_HASH_ALG_SM3_256;
> > +			break;
> > +		default:
> > +			EFI_PRINT("Unknown algorithm %x\n", pcrs.selection[i].hash);
> > +			break;
> > +		}
> > +	}
> > +
> > +	*pcr_banks = pcrs.count;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * get_capability() - provide protocol capability information and state information
> > + *
> > + * @this:		TCG2 protocol instance
> > + * @capability:		caller allocated memory with size field to the size of the
> > + *			structure allocated
> > +
> > + * Return:	status code
> > + */
> > +static efi_status_t EFIAPI
> > +get_capability(struct efi_tcg2_protocol *this,
> > +	       struct efi_tcg2_boot_service_capability *capability)
> > +{
> > +	struct udevice *dev;
> > +	efi_status_t efi_ret;
> > +	int ret;
> > +
> > +	EFI_ENTRY("%p, %p", this, capability);
> > +
> > +	if (!this || !capability)
> > +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> > +
> > +	if (capability->size < boot_service_capability_min) {
> > +		capability->size = boot_service_capability_min;
> > +		return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
> > +	}
> > +
> > +	if (capability->size < sizeof(*capability)) {
> > +		capability->size = sizeof(*capability);
> > +		return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
> > +	}
> > +
> > +	capability->structure_version.major = 1;
> > +	capability->structure_version.minor = 1;
> > +	capability->protocol_version.major = 1;
> > +	capability->protocol_version.minor = 1;
> > +
> > +	efi_ret = platform_get_tpm2_device(&dev);
> > +	if (efi_ret != EFI_SUCCESS) {
> > +		capability->supported_event_logs = 0;
> > +		capability->hash_algorithm_bitmap = 0;
> > +		capability->tpm_present_flag = false;
> > +		capability->max_command_size = 0;
> > +		capability->max_response_size = 0;
> > +		capability->manufacturer_id = 0;
> > +		capability->number_of_pcr_banks = 0;
> > +		capability->active_pcr_banks = 0;
> > +
> > +		return EFI_EXIT(EFI_SUCCESS);
> 
> Instead of hoping that the compiler reduces the code size I would prefer
> to use a common exit point where you call EFI_EXIT().

Sure I'll change this on v3

> 
> Best regards
> 
> Heinrich
> 
[...]

Regards
/Ilias