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
