[PATCH 2/3] efi_loader: Introduce eventlog support for TCG2_PROTOCOL

[Heinrich Schuchardt]

On 11/29/20 2:27 PM, Ilias Apalodimas wrote:
> On Sun, Nov 29, 2020 at 07:02:39AM +0100, Heinrich Schuchardt wrote:
>> On 11/27/20 5:29 PM, Ilias Apalodimas wrote:
>>> In the previous patches we only introduced a minimal subset of the
>>>
> [...]
>>> +#define TPM2_SHA1_DIGEST_SIZE 20
>>> +#define TPM2_SHA256_DIGEST_SIZE 32
>>> +#define TPM2_SHA384_DIGEST_SIZE 48
>>> +#define TPM2_SHA512_DIGEST_SIZE 64
>>
>> lib/efi_loader/efi_tcg2.c already includes tpm-v2.h.
>>
>> Why should we define the same constant twice?
>>
>
> We don't. That was probably a c/p I forgot to fix when I creted the
> patches.
> I'll only keep the declarations in tpm-v2.h.
>
>> Best regards
>>
>> Heinrich
>>
>>> +
>>> +#define EFI_TCG2_FINAL_EVENTS_TABLE_VERSION 1
>>> +
>>> +#define TPM2_EVENT_LOG_SIZE 4096
>>
>> What does this size derive from? A comment describing the constant could
>> help.
>>
>
> There's no guidance for this. This is the size of the eventlog and it's up to us
> to define something that makes sense. It obviously depends on:
> - Number of supported algoriths
> - Number of events
> We could move it to a Kconfig?

That is probably the best choice.

>
>>> +
>>>   typedef u32 efi_tcg_event_log_bitmap;
>>>   typedef u32 efi_tcg_event_log_format;
>>>   typedef u32 efi_tcg_event_algorithm_bitmap;
>>> @@ -65,6 +76,40 @@ struct efi_tcg2_boot_service_capability {
>>>   	sizeof(struct efi_tcg2_boot_service_capability) - \
>>>   	offsetof(struct efi_tcg2_boot_service_capability, number_of_pcr_banks)
>>>
>>> +#define tcg_efi_spec_id_event_signature_03 "Spec ID Event03"
>>> +
>>> +#define tcg_efi_spec_id_event_spec_version_major_tpm2 2
>>> +#define tcg_efi_spec_id_event_spec_version_minor_tpm2 0
>>> +#define tcg_efi_spec_id_event_spec_version_errata_tpm2 0
>>
>> Constants should be capitalized.
>>
>
> Ok
>
>>> +
>>
>> Please, provide Sphinx style comments for structures. Cf.
>> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
>>
>
> Ok
>
>>> +struct tcg_efi_spec_id_event_algorithm_size {
>>> +	u16      algorithm_id;
>>> +	u16      digest_size;
>>> +} __packed;
>>> +
>>> +struct tcg_efi_spec_id_event {
>>> +	u8 signature[16];
>>> +	u32 platform_class;
>>> +	u8 spec_version_minor;
>>> +	u8 spec_version_major;
>>> +	u8 spec_errata;
>>> +	u8 uintn_size;
>>> +	u32 number_of_algorithms;
>>> +	struct tcg_efi_spec_id_event_algorithm_size digest_sizes[TPM2_NUM_PCR_BANKS];
>>> +	u8 vendor_info_size;
>>> +	/*
>>> +	 * filled in with vendor_info_size
>>> +	 * currently vendor_info_size = 0
>>
>> %s/vendor_info_size = 0/U-Boot does not provide any vendor info/
>>
>
> Ok
>
> [...]
>>> +	/* Setup specID event data */
>>> +	spec_event = (struct tcg_efi_spec_id_event *)buffer;
>>> +	memcpy(spec_event->signature, tcg_efi_spec_id_event_signature_03,
>>> +	       sizeof(spec_event->signature));
>>> +	put_unaligned_le32(0, &spec_event->platform_class); /* type client */
>>> +	__put_unaligned_le(tcg_efi_spec_id_event_spec_version_minor_tpm2,
>>> +			   &spec_event->spec_version_minor);
>>> +	__put_unaligned_le(tcg_efi_spec_id_event_spec_version_major_tpm2,
>>> +			   &spec_event->spec_version_major);
>>> +	__put_unaligned_le(tcg_efi_spec_id_event_spec_version_errata_tpm2,
>>> +			   &spec_event->spec_errata);
>>> +	__put_unaligned_le(sizeof(efi_uintn_t) / sizeof(u32),
>>> +			   &spec_event->uintn_size);
>
> Any preference on this?
> The put_unaligned here is not strictly needed since it's u8 values.
> It just seemed 'easier' to read since all the other additions to the log
> are done with put_unaligned16/32.

U-Boot does not support unaligned access on some platforms before
allow_unaligned() is called. Furthermore endianness depends on the platform.

The current function is called after the initialization of the UEFI
sub-system (actually during the TCG2 protocol invocation). At this point
U-Boot supports unaligned access. And you know that your system is
little-endian. So you should use normal assignments which is much more
readable and reduces code size.

spec_event->platform_class = 0;
...
spec_event->uintn_size = efi_uintn_t) / sizeof(u32);

Best regards

Heinrich

>
> [...]
>
> cheerrs
> /Ilias
>