Hi Heinrich
[...]
> > +#define TPM2_NUM_PCR_BANKS 16
> > +
> > +/* Table 22 — Definition of (UINT32) TPM_CAP Constants */
> > +#define TPM_CAP_FIRST 0x00000000U
> > +#define TPM_CAP_ALGS 0x00000000U
> > +#define TPM_CAP_HANDLES 0x00000001U
> > +#define TPM_CAP_COMMANDS 0x00000002U
> > +#define TPM_CAP_PP_COMMANDS 0x00000003U
> > +#define TPM_CAP_AUDIT_COMMANDS 0x00000004U
> > +#define TPM_CAP_PCRS 0x00000005U
> > +#define TPM_CAP_TPM_PROPERTIES 0x00000006U
> > +#define TPM_CAP_PCR_PROPERTIES 0x00000007U
> > +#define TPM_CAP_ECC_CURVES 0x00000008U
> > +#define TPM_CAP_LAST 0x00000008U
> > +#define TPM_CAP_VENDOR_PROPERTY 0x00000100U
>
> Thanks a lot for implementing this protocol which may be consumed by
> GRUB for example.
>
There's still some way ahead for that, but at least this implements the basics
> The changes to tpm-v2.h should be in a separate patch as they are not
> tied to UEFI.
Ok I'll split them off in v2
[...]
> > +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) {
> > + log_warning("Unable to get tpm device\n");
> > + return EFI_DEVICE_ERROR;
> > + }
> > +
> > + *dev = devp;
> > +
> > + return EFI_SUCCESS;
> > +}
>
> We are talking to the DM driver. That DM driver should take care of any
> initialization needed to make the TPM usable. I do not understand why we
> need a weak function here.
I was defined as a __weak function in efi_rng.c, so I assumed that was the
common practice.
Shall I remove it?
>
> > +
> > +/**
> > + * 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[COMMAND_BUFFER_SIZE];
> > + u32 ret;
> > +
> > + ret = tpm2_get_capability(dev, TPM_CAP_TPM_PROPERTIES,
> > + TPM_PT_MAX_COMMAND_SIZE, response, 1, false);
> > + if (ret)
> > + return -1;
> > + /*
> > + * On the definition of TPMS_TAGGED_PROPERTY Structure
> > + * property: u32
> > + * value: u32
> > + * So advance the response for TPM_CAP_TPM_PROPERTIES by sizeof(u32)
> > + * to get the value
> > + */
> > + *max_command_size = (uint16_t)get_unaligned_be32(response +
> > sizeof(u32));
>
> Why are we using COMMAND_BUFFER_SIZE throughout the TPM code if the
> required buffer size for commands and responses can be read from the TPM
> device?
I think the logic is that 256b is enough for the basic commands we needed.
I can change that here. Get the TPM response during efi_tcg2_register() and use
that
for the rest of the code?
>
> > +
> > + 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[COMMAND_BUFFER_SIZE];
> > + u32 ret;
> > +
> > + ret = tpm2_get_capability(dev, TPM_CAP_TPM_PROPERTIES,
> > + TPM_PT_MAX_RESPONSE_SIZE, response, 1, false);
> > + if (ret)
> > + return -1;
> > + /*
> > + * On the definition of TPMS_TAGGED_PROPERTY Structure
> > + * property: u32
> > + * value: u32
> > + * So advance the response for TPM_CAP_TPM_PROPERTIES by sizeof(u32)
> > + * to get the value
> > + */
> > + *max_response_size = (uint16_t)get_unaligned_be32(response +
> > sizeof(u32));
> > +
> > + 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[COMMAND_BUFFER_SIZE];
> > + u32 ret;
> > +
> > + ret = tpm2_get_capability(dev, TPM_CAP_TPM_PROPERTIES,
> > + TPM_PT_MANUFACTURER, response, 1, false);
> > + if (ret)
> > + return -1;
> > + *manufacturer_id = get_unaligned_be32(response + sizeof(u32));
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * is_active_pcr() - Check if an supported algorithm is active
>
> %s/an supported/a supported/
Ok
>
> > + *
> > + * @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
> > + */
> > +static int tpm2_get_pcr_info(struct udevice *dev, u32 *supported_pcr, u32
> > *active_pcr,
> > + u32 *pcr_banks)
> > +{
> > + u8 response[COMMAND_BUFFER_SIZE];
> > + struct tpml_pcr_selection pcrs;
> > + u32 ret;
> > + int i;
> > +
> > + ret = tpm2_get_capability(dev, TPM_CAP_PCRS, 0, response, 1, true);
> > + if (ret)
> > + return -1;
> > +
> > + pcrs.count = get_unaligned_be32(response);
> > + if (pcrs.count > MAX_HASH_COUNT || pcrs.count < 1)
> > + 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
> > + */
> > + u32 hash_offset = sizeof(pcrs.count) + i *
> > sizeof(pcrs.selection[0]);
> > + 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:
> > + log_err("Unknown algorithm %x\n",
> > pcrs.selection[i].hash);
>
> You do not want to mess up the screen output of an EFI application.
>
> Please, use EFI_PRINT().
Will do
>
> > + 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;
> > + 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;
> > +
> > + ret = platform_get_tpm2_device(&dev);
>
> The return value is of type efi_uintn_t and not int.
>
Yep, missed that, thanks!
> > + if (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;
>
> Why don't you simply return EFI_DEVICE_ERROR if there is no device?
>
> So actually I would write:
>
> if (ret != EFI_SUCCESS)
> goto out;
>
> Where out: is the central exit point.
Because that's what the TPM TCG2 spec says I should do, if a device is not
present [1]
>
> out:
> EFI_EXIT(ret);
>
>
> > + .get_result_of_set_active_pcr_banks =
> > get_result_of_set_active_pcr_banks,
[...]
> > +};
> > +
> > +/**
> > + * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
> > + *
> > + * If a TPM2 device is available, the TPM TCG2 Protocol is registered
> > + *
> > + * Return: An error status is only returned if adding the protocol fails.
> > + */
> > +efi_status_t efi_tcg2_register(void)
> > +{
> > + int ret;
>
> efi_unintn_t ret;
Yep, I'll fix it for v2
>
> Best regards
>
> Heinrich
>
> > + struct udevice *dev;
> > + enum tpm_version tpm_ver;
> > +
> > + ret = platform_get_tpm2_device(&dev);
>
> The return value is of type efi_uintn_t (aka size_t).
>
> > + if (ret != EFI_SUCCESS)
> > + return EFI_SUCCESS;
> > +
> > + tpm_ver = tpm_get_version(dev);
> > + if (tpm_ver != TPM_V2) {
> > + log_warning("Only TPMv2 supported for EFI_TCG2_PROTOCOL");
> > + return EFI_SUCCESS;
> > + }
> > +
> > + ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
> > + (void *)&efi_tcg2_protocol);
> > + if (ret != EFI_SUCCESS)
> > + log_err("Cannot install EFI_TCG2_PROTOCOL");
> > +
> > + return ret;
> > +}
> >
>
[1]
https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
Section 6.4.4
Regards
/Ilias
