Hi Heinrich, On Sun, Nov 29, 2020 at 06:28:39AM +0100, Heinrich Schuchardt wrote: > On 11/27/20 5:29 PM, Ilias Apalodimas wrote: > > A following patch introduces support for the EFI_TCG2_PROTOCOL > > evenlog management. > > %s/evenlog/eventlog/ > > > Introduce the necessary tpm related headers > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org> > > --- > > include/tpm-v2.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > index d8c9feda28dc..9637f9be998e 100644 > > --- a/include/tpm-v2.h > > +++ b/include/tpm-v2.h > > @@ -18,6 +18,12 @@ > > > > #define TPM2_DIGEST_LEN 32 > > Shouldn't TPM2_DIGEST_LEN be renamed to TPM2_SHA256_DIGEST_SIZE in all > of our code?
Ideally yes. The current tpm2 pre-existing code (apart from tpm2_pcr_extend() which I changed) uses a hardware SHA256 algorithm. Should we do it in this patchset though? > > > > > +#define TPM2_SHA1_DIGEST_SIZE 20 > > +#define TPM2_SHA256_DIGEST_SIZE 32 > > +#define TPM2_SHA384_DIGEST_SIZE 48 > > +#define TPM2_SHA512_DIGEST_SIZE 64 > > +#define TPM2_SM3_256_DIGEST_SIZE 32 > > + > > #define TPM2_MAX_PCRS 32 > > #define TPM2_PCR_SELECT_MAX ((TPM2_MAX_PCRS + 7) / 8) > > #define TPM2_MAX_CAP_BUFFER 1024 > > @@ -45,6 +51,15 @@ > > #define TPM2_PT_MAX_COMMAND_SIZE (u32)(TPM2_PT_FIXED + 30) > > #define TPM2_PT_MAX_RESPONSE_SIZE (u32)(TPM2_PT_FIXED + 31) > > > > +/* event types */ > > +#define EV_POST_CODE ((u32)0x00000001) > > +#define EV_NO_ACTION ((u32)0x00000003) > > +#define EV_SEPARATOR ((u32)0x00000004) > > +#define EV_S_CRTM_CONTENTS ((u32)0x00000007) > > +#define EV_S_CRTM_VERSION ((u32)0x00000008) > > +#define EV_CPU_MICROCODE ((u32)0x00000009) > > +#define EV_TABLE_OF_DEVICES ((u32)0x0000000B) > > + > > /* TPMS_TAGGED_PROPERTY Structure */ > > struct tpms_tagged_property { > > u32 property; > > @@ -86,6 +101,50 @@ struct tpms_capability_data { > > union tpmu_capabilities data; > > } __packed; > > > > +/* defined as TPM_SHA1_160_HASH_LEN in spec */ > > +struct tpm_digest { > > + u8 digest[TPM2_SHA1_DIGEST_SIZE]; > > +} __packed; > > + > > +/* SHA1 Event Log Entry Format */ > > Please, use Sphinx style comments for structures. This allows us to add > them to the HTML documentation. See > > https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation > > I would appreciate member descriptions, too. > Ok I assumed refering to the spec would be fine. I'll add description on v2 > > +struct tcg_pcr_event { > > + u32 pcr_index; > > + u32 event_type; > > + struct tpm_digest digest; > > struct tpm_digest is only used here in your patch series. > > The standard has > > typedef UINT8 TCG_DIGEST[TPM2_SHA1_DIGEST_SIZE]; > > Can't we simply write > > u8 digest[20]; > > here and get rid of struct tpm_digest? Yea we can. since it's a complex spec though I am trying to adhere to it as much as possible to make review and future extentions easier. I'd prefer keeping this as is tbh. > > Otherwise looks ok to me. > Thanks for the review!. I'll wait a few more days, fix your remarks and post a v2 Cheers /Ilias > Best regards > > Heinrich > > > + u32 event_size; > > + u8 event[]; > > +} __packed; > > + > > +/* Definition of TPMU_HA Union */ > > +union tmpu_ha { > > + u8 sha1[TPM2_SHA1_DIGEST_SIZE]; > > + u8 sha256[TPM2_SHA256_DIGEST_SIZE]; > > + u8 sm3_256[TPM2_SM3_256_DIGEST_SIZE]; > > + u8 sha384[TPM2_SHA384_DIGEST_SIZE]; > > + u8 sha512[TPM2_SHA512_DIGEST_SIZE]; > > +} __packed; > > + > > +/* Definition of TPMT_HA Structure */ > > +struct tpmt_ha { > > + u16 hash_alg; > > + union tmpu_ha digest; > > +} __packed; > > + > > +/* Definition of TPML_DIGEST_VALUES Structure */ > > +struct tpml_digest_values { > > + u32 count; > > + struct tpmt_ha digests[TPM2_NUM_PCR_BANKS]; > > +} __packed; > > + > > +/* Crypto Agile Log Entry Format */ > > +struct tcg_pcr_event2 { > > + u32 pcr_index; > > + u32 event_type; > > + struct tpml_digest_values digests; > > + u32 event_size; > > + u8 event[]; > > +} __packed; > > + > > /** > > * TPM2 Structure Tags for command/response buffers. > > * > > >