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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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?

> > +
> >   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.

[...]

cheerrs
/Ilias