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 >