On Sat, Nov 07, 2020 at 09:33:35PM +0100, Heinrich Schuchardt wrote: > On 11/5/20 10:58 PM, Ilias Apalodimas wrote: > > A following patch introduces EFI_TCG2_PROTOCOL. > > Add the required TPMv2 headers to support that and remove the (now) > > redundant definitions from tpm2_tis_sandbox > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org> > > --- > > include/tpm-v2.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 69 insertions(+) > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > index f6c045d35480..b62f2c5b0fb8 100644 > > --- a/include/tpm-v2.h > > +++ b/include/tpm-v2.h > > @@ -11,6 +11,73 @@ > > > > #define TPM2_DIGEST_LEN 32 > > Miquel, the constant name and its usage seems not to reflect the TCG spec: > > The spec has the following alternative digest length constants: > > #define TPM2_SHA_DIGEST_SIZE 20 > #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 > > Can't we use the same names as in the spec? Why did you assume in your > code that SHA256 is the only digest being used? > > > > > +#define TPM2_MAX_PCRS 32 > > +#define TPM2_PCR_SELECT_MAX ((TPM2_MAX_PCRS + 7) / 8) > > +#define TPM2_MAX_CAP_BUFFER 1024 > > +#define TPM2_MAX_TPM_PROPERTIES ((TPM2_MAX_CAP_BUFFER - sizeof(u32) /* > > TPM2_CAP */ - \ > > + sizeof(u32)) / sizeof(struct > > tpms_tagged_property)) > > + > > +/* > > + * We deviate from this draft of the specification by increasing the > > value of TPM2_NUM_PCR_BANKS > > + * from 3 to 16 to ensure compatibility with TPM2 implementations that > > have enabled a larger than > > + * typical number of PCR banks. This larger value for TPM2_NUM_PCR_BANKS > > is expected to be included > > + * in a future revision of the specification. > > Ilias, please, restrict your lines to 80 characters where possible. >
Sure > Do you have a reference for the usage of that larger number on current > hardware? We should make the deviation from the standard easily verifiable. > Yes this was copied from this: https://tpm2-tss.readthedocs.io/en/latest/ > > + */ > > +#define TPM2_NUM_PCR_BANKS 16 > > + > > +/* Definition of (UINT32) TPM2_CAP Constants */ > > +#define TPM2_CAP_PCRS 0x00000005U > > +#define TPM2_CAP_TPM_PROPERTIES 0x00000006U > > + > > +/* Definition of (UINT32) TPM2_PT Constants */ > > +#define PT_GROUP (u32)(0x00000100) > > +#define PT_FIXED (u32)(PT_GROUP * 1) > > +#define TPM2_PT_MANUFACTURER (u32)(PT_FIXED + 5) > > +#define TPM2_PT_PCR_COUNT (u32)(PT_FIXED + 18) > > +#define TPM2_PT_MAX_COMMAND_SIZE (u32)(PT_FIXED + 30) > > +#define TPM2_PT_MAX_RESPONSE_SIZE (u32)(PT_FIXED + 31) > > All these definitions are all copied from the "TCG TSS2.0 Overview and > Common Structures Specification". I am missing a reference to the > copyright notice of the spec. I think the best thing to do would be > placing the TCG copyrighted code into a separate include that is > included in tpm_v2.h. Please, check with Tom if the license contradicts > GPL. Especially the following sentence seems problematic: > > "THE COPYRIGHT LICENSES SET FORTH ABOVE DO NOT REPRESENT ANY FORM OF > LICENSE OR WAIVER, EXPRESS OR IMPLIED, BY ESTOPPEL OR OTHERWISE, WITH > RESPECT TO PATENT RIGHTS HELD BY TCG MEMBERS (OR OTHER THIRD PARTIES) > THAT MAY BE NECESSARY TO IMPLEMENT THIS SPECIFICATION OR OTHERWISE." > > Cf. https://fedoraproject.org/wiki/Licensing/TCGL > Ok will do > > + > > +/* TPMS_TAGGED_PROPERTY Structure */ > > +struct tpms_tagged_property { > > + u32 property; > > + u32 value; > > +} __packed; > > + > > +/* TPMS_PCR_SELECTION Structure */ > > +struct tpms_pcr_selection { > > + u16 hash; > (This is a TPM2_ALG_ID.) > > + u8 size_of_select; > > + u8 pcr_select[TPM2_PCR_SELECT_MAX]; > > +} __packed; > > + > > +/* TPML_PCR_SELECTION Structure */ > > +struct tpml_pcr_selection { > > + u32 count; > > + struct tpms_pcr_selection selection[TPM2_NUM_PCR_BANKS]; > > +} __packed; > > + > > +/* TPML_TAGGED_TPM_PROPERTY Structure */ > > +struct tpml_tagged_tpm_property { > > + u32 count; > > + struct tpms_tagged_property tpm_property[TPM2_MAX_TPM_PROPERTIES]; > > +} __packed; > > + > > +/* TPMU_CAPABILITIES Union */ > > +union tpmu_capabilities { > > + /* > > + * Non exhaustive. Only added the structs needed for our > > + * current code > > + */ > > + struct tpml_pcr_selection assigned_pcr; > > + struct tpml_tagged_tpm_property tpm_properties; > > +} __packed; > > + > > +/* TPMS_CAPABILITY_DATA Structure */ > > +struct tpms_capability_data { > > + u32 capability; > > + union tpmu_capabilities data; > > +} __packed; > > + > > /** > > * TPM2 Structure Tags for command/response buffers. > > * > > @@ -123,11 +190,13 @@ enum tpm2_return_codes { > > * TPM2 algorithms. > > */ > > enum tpm2_algorithms { > > + TPM2_ALG_SHA1 = 0x04, > > TPM2_ALG_XOR = 0x0A, > > TPM2_ALG_SHA256 = 0x0B, > > TPM2_ALG_SHA384 = 0x0C, > > TPM2_ALG_SHA512 = 0x0D, > > TPM2_ALG_NULL = 0x10, > > + TPM2_ALG_SM3_256 = 0x12, > > }; > > In the spec these values are not an enum (i.e. 32 bit values if you do > not use short enums) but explicitly u16/TPM2_ALG_ID. But probably that > does not make a difference. I just extended what was already there. I agree we are better off changing it, not on this series though. Regards /Ilias > > Best regards > > Heinrich > > > > > /* NV index attributes */ > > >