| Message ID | 20201105215846.1017178-2-ilias.apalodimas@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3,v2] tpm: Change response length of tpm2_get_capability() | expand |
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.apalodimas@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. 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. > + */ > +#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 > + > +/* 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. Best regards Heinrich > > /* NV index attributes */ >
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.apalodimas@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 */ > > >
Hi Heinrich, [...] > > > > + */ > > > +#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 So I talked to Tom and he suggested we have a look at linux, or any other project that uses those. I can't find any copyright claims in include/linux/tpm.h, apart from a pointer to the spec. I don't think splitting the changes to a new file is a good idea. Most of the existing definitions of the file are part of the same document. Maybe just updating the copyright properly is the right thing to do? [...] Regards /Ilias
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 +#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. + */ +#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) + +/* TPMS_TAGGED_PROPERTY Structure */ +struct tpms_tagged_property { + u32 property; + u32 value; +} __packed; + +/* TPMS_PCR_SELECTION Structure */ +struct tpms_pcr_selection { + u16 hash; + 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, }; /* NV index attributes */
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.apalodimas@linaro.org> --- include/tpm-v2.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) -- 2.29.2