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

[Ilias Apalodimas]

On Sun, Nov 29, 2020 at 02:49:33PM +0100, Heinrich Schuchardt wrote:
> 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;

The native cpu endianess is irrelevant here. 
The eventlog data structures need to be LE if I understand the spec 
correctly [1].

Since this is not supposed to run on Arm only (or any platform that supports 
unaligned accesses) I'd prefer keeping the put_unaligned16/32 calls.
I can remove the access to u8 on v2.
Furthermore tcg2_agile_log_append() can be called outside UEFI, in case we want
to extend U-boot and measure events before initializing EFI. In that case 
we got no guarantee on alignment.

[1] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
chapter 3.1 "Data Structures"

Cheers
/Ilias

> ...
> spec_event->uintn_size = efi_uintn_t) / sizeof(u32);
> 
> Best regards
> 
> Heinrich
> 
> >
> > [...]
> >
> > cheerrs
> > /Ilias
> >
>