| Message ID | 20201104134748.810291-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] tpm: Make response length of tpm2_get_capability() configurable | expand |
On 04.11.20 14:47, Ilias Apalodimas wrote: > A following patch introduces EFI_TCG2_PROTOCOL. > One of the functions of that protocol is GetCapability(). > In order to parse device capabilities we need to access a u32 > before the properties which the current implementation ignores > while reading device properties. > > So let's make the response length configurable and prepare the > functions for EFI_TCG2_PROTOCOL. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/tpm-v2.c | 2 +- > include/tpm-v2.h | 12 +++++++----- > lib/tpm-v2.c | 10 +++++++--- > 3 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > index e6742656f578..c2df1c34043a 100644 > --- a/cmd/tpm-v2.c > +++ b/cmd/tpm-v2.c > @@ -183,7 +183,7 @@ static int do_tpm_get_capability(struct cmd_tbl *cmdtp, int flag, int argc, > data = map_sysmem(simple_strtoul(argv[3], NULL, 0), 0); > count = simple_strtoul(argv[4], NULL, 0); > > - rc = tpm2_get_capability(dev, capability, property, data, count); > + rc = tpm2_get_capability(dev, capability, property, data, count, false); > if (rc) > goto unmap_data; > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > index f6c045d35480..ee74028ca83b 100644 > --- a/include/tpm-v2.h > +++ b/include/tpm-v2.h > @@ -257,15 +257,17 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz, > * to query property index that is 4-byte wide. > * > * @dev TPM device > - * @capability Partition of capabilities > - * @property Further definition of capability, limited to be 4 bytes wide > - * @buf Output buffer for capability information > - * @prop_count Size of output buffer > + * @capability Partition of capabilities > + * @property Further definition of capability, limited to be 4 bytes > + * wide > + * @buf Output buffer for capability information > + * @prop_count Size of output buffer > + * @get_count Include tpmu property count > * > * @return code of the operation > */ > u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > - void *buf, size_t prop_count); > + void *buf, size_t prop_count, bool get_count); > > /** > * Issue a TPM2_DictionaryAttackLockReset command. > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > index a4c352e3ef75..b58c1057995b 100644 > --- a/lib/tpm-v2.c > +++ b/lib/tpm-v2.c > @@ -161,7 +161,7 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz, > } > > u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > - void *buf, size_t prop_count) > + void *buf, size_t prop_count, bool get_count) The implementation would be more stable if we would derive the offset from field property instead of adding get_count. > { > u8 command_v2[COMMAND_BUFFER_SIZE] = { Shouldn't COMMAND_BUFFER_SIZE be changed to something with TPM in the name, e.g TPM_COMMAND_BUFFER_SIZE? > tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */ > @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > if (ret) > return ret; > > + /* When reading PCR properties we need the count */ > + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > + sizeof(u8) + sizeof(u32); > /* > * In the response buffer, the properties are located after the: > * tag (u16), response size (u32), response code (u32), > * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32). > */ This comment should be above 'properties_off ='. 'get_count' related field should be mentioned. Best regards Heinrich > - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > - sizeof(u8) + sizeof(u32) + sizeof(u32); > + if (!get_count) > + properties_off += sizeof(u32); > + > memcpy(buf, &response[properties_off], response_len - properties_off); > > return 0; >
Hi Heinrich, [...] > > +++ b/lib/tpm-v2.c > > @@ -161,7 +161,7 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz, > > } > > > > u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > > - void *buf, size_t prop_count) > > + void *buf, size_t prop_count, bool get_count) > > The implementation would be more stable if we would derive the offset > from field property instead of adding get_count. > We are not defining the tpmv2 internal structures anywhere in U-Boot. That's why the code is doing static sizeof(uX) instead of using offsetof. In the EFI part of the patchset, I've done exaclty that. Working with offsets on well defined struct is better, but out of scope for this patchset imho. We can look into refactoring the generic tpmv2 code once I add the rest of the EFI protocol parts? > > { > > u8 command_v2[COMMAND_BUFFER_SIZE] = { > > Shouldn't COMMAND_BUFFER_SIZE be changed to something with TPM in the > name, e.g TPM_COMMAND_BUFFER_SIZE? > > > tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */ > > @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > > if (ret) > > return ret; > > > > + /* When reading PCR properties we need the count */ > > + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > > + sizeof(u8) + sizeof(u32); > > /* > > * In the response buffer, the properties are located after the: > > * tag (u16), response size (u32), response code (u32), > > * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32). > > */ > > This comment should be above 'properties_off ='. 'get_count' related > field should be mentioned. Sure, I'll fix this Regards /Ilias > > Best regards > > Heinrich > > > - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > > - sizeof(u8) + sizeof(u32) + sizeof(u32); > > + if (!get_count) > > + properties_off += sizeof(u32); > > + > > memcpy(buf, &response[properties_off], response_len - properties_off); > > > > return 0; > > >
On 04.11.20 16:56, Ilias Apalodimas wrote: > Hi Heinrich, > > [...] >>> +++ b/lib/tpm-v2.c >>> @@ -161,7 +161,7 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz, >>> } >>> >>> u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, >>> - void *buf, size_t prop_count) >>> + void *buf, size_t prop_count, bool get_count) >> >> The implementation would be more stable if we would derive the offset >> from field property instead of adding get_count. >> > > We are not defining the tpmv2 internal structures anywhere in U-Boot. > That's why the code is doing static sizeof(uX) instead of using offsetof. > In the EFI part of the patchset, I've done exaclty that. > Working with offsets on well defined struct is better, but out of scope for this > patchset imho. > We can look into refactoring the generic tpmv2 code once I add the rest of the EFI > protocol parts? Can't we add the structures that we need to tpm-v2.h and use their size here? Best regards Heinrich > >>> { >>> u8 command_v2[COMMAND_BUFFER_SIZE] = { >> >> Shouldn't COMMAND_BUFFER_SIZE be changed to something with TPM in the >> name, e.g TPM_COMMAND_BUFFER_SIZE? >> >>> tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */ >>> @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, >>> if (ret) >>> return ret; >>> >>> + /* When reading PCR properties we need the count */ >>> + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + >>> + sizeof(u8) + sizeof(u32); >>> /* >>> * In the response buffer, the properties are located after the: >>> * tag (u16), response size (u32), response code (u32), >>> * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32). >>> */ >> >> This comment should be above 'properties_off ='. 'get_count' related >> field should be mentioned. > > Sure, I'll fix this > > Regards > /Ilias >> >> Best regards >> >> Heinrich >> >>> - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + >>> - sizeof(u8) + sizeof(u32) + sizeof(u32); >>> + if (!get_count) >>> + properties_off += sizeof(u32); >>> + >>> memcpy(buf, &response[properties_off], response_len - properties_off); >>> >>> return 0; >>> >>
Hi Heinrich, On Wed, Nov 04, 2020 at 05:50:16PM +0100, Heinrich Schuchardt wrote: > On 04.11.20 16:56, Ilias Apalodimas wrote: > >>> > >>> u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > >>> - void *buf, size_t prop_count) > >>> + void *buf, size_t prop_count, bool get_count) > >> > >> The implementation would be more stable if we would derive the offset > >> from field property instead of adding get_count. > >> > > > > We are not defining the tpmv2 internal structures anywhere in U-Boot. > > That's why the code is doing static sizeof(uX) instead of using offsetof. > > In the EFI part of the patchset, I've done exaclty that. > > Working with offsets on well defined struct is better, but out of scope for this > > patchset imho. > > We can look into refactoring the generic tpmv2 code once I add the rest of the EFI > > protocol parts? > > Can't we add the structures that we need to tpm-v2.h and use their size > here? Yes we can, but the scope of the patch is adding EFI_TCG2_PROTOCOL, not re-factor the tpmv2 code along the way. Since the only change on the existing code to support the functionality needed for the EFI_TCG2_PROTOCOL protocol is just 4 bytes on an existing buffer, can we stick to that and think of refactoring the tpm stuff afterwards? Regards /Ilias > > Best regards > > Heinrich > > > > >>> { > >>> u8 command_v2[COMMAND_BUFFER_SIZE] = { > >> > >> Shouldn't COMMAND_BUFFER_SIZE be changed to something with TPM in the > >> name, e.g TPM_COMMAND_BUFFER_SIZE? > >> > >>> tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */ > >>> @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > >>> if (ret) > >>> return ret; > >>> > >>> + /* When reading PCR properties we need the count */ > >>> + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > >>> + sizeof(u8) + sizeof(u32); > >>> /* > >>> * In the response buffer, the properties are located after the: > >>> * tag (u16), response size (u32), response code (u32), > >>> * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32). > >>> */ > >> > >> This comment should be above 'properties_off ='. 'get_count' related > >> field should be mentioned. > > > > Sure, I'll fix this > > > > Regards > > /Ilias > >> > >> Best regards > >> > >> Heinrich > >> > >>> - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > >>> - sizeof(u8) + sizeof(u32) + sizeof(u32); > >>> + if (!get_count) > >>> + properties_off += sizeof(u32); > >>> + > >>> memcpy(buf, &response[properties_off], response_len - properties_off); > >>> > >>> return 0; > >>> > >> >
On 11/4/20 6:26 PM, Ilias Apalodimas wrote: > Hi Heinrich, > > On Wed, Nov 04, 2020 at 05:50:16PM +0100, Heinrich Schuchardt wrote: >> On 04.11.20 16:56, Ilias Apalodimas wrote: >>>>> >>>>> u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, >>>>> - void *buf, size_t prop_count) >>>>> + void *buf, size_t prop_count, bool get_count) >>>> >>>> The implementation would be more stable if we would derive the offset >>>> from field property instead of adding get_count. >>>> >>> >>> We are not defining the tpmv2 internal structures anywhere in U-Boot. >>> That's why the code is doing static sizeof(uX) instead of using offsetof. >>> In the EFI part of the patchset, I've done exaclty that. >>> Working with offsets on well defined struct is better, but out of scope for this >>> patchset imho. >>> We can look into refactoring the generic tpmv2 code once I add the rest of the EFI >>> protocol parts? >> >> Can't we add the structures that we need to tpm-v2.h and use their size >> here? > > Yes we can, but the scope of the patch is adding EFI_TCG2_PROTOCOL, not re-factor the tpmv2 > code along the way. > Since the only change on the existing code to support the functionality needed for the > EFI_TCG2_PROTOCOL protocol is just 4 bytes on an existing buffer, can we stick to that and > think of refactoring the tpm stuff afterwards? You expect a TPML_PCR_SELECTION when capabilities is TPM_CAP_PCRS. typedef struct { UINT32 count; TPMS_PCR_SELECTION pcrSelections[TPM2_NUM_PCR_BANKS]; } TPML_PCR_SELECTION; Why do you have to skip over the UINT32 here? You just have to define this one structure in preparation of patch 2 and then you can correctly parse the response in your implementation of the EFI protocol. I suggest not to change tpm2_get_capability() at all. Best regards Heinrich > > Regards > /Ilias >> >> Best regards >> >> Heinrich >> >>> >>>>> { >>>>> u8 command_v2[COMMAND_BUFFER_SIZE] = { >>>> >>>> Shouldn't COMMAND_BUFFER_SIZE be changed to something with TPM in the >>>> name, e.g TPM_COMMAND_BUFFER_SIZE? >>>> >>>>> tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */ >>>>> @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, >>>>> if (ret) >>>>> return ret; >>>>> >>>>> + /* When reading PCR properties we need the count */ >>>>> + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + >>>>> + sizeof(u8) + sizeof(u32); >>>>> /* >>>>> * In the response buffer, the properties are located after the: >>>>> * tag (u16), response size (u32), response code (u32), >>>>> * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32). >>>>> */ >>>> >>>> This comment should be above 'properties_off ='. 'get_count' related >>>> field should be mentioned. >>> >>> Sure, I'll fix this >>> >>> Regards >>> /Ilias >>>> >>>> Best regards >>>> >>>> Heinrich >>>> >>>>> - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + >>>>> - sizeof(u8) + sizeof(u32) + sizeof(u32); >>>>> + if (!get_count) >>>>> + properties_off += sizeof(u32); >>>>> + >>>>> memcpy(buf, &response[properties_off], response_len - properties_off); >>>>> >>>>> return 0; >>>>> >>>> >>
On Wed, Nov 04, 2020 at 10:59:03PM +0100, Heinrich Schuchardt wrote: > On 11/4/20 6:26 PM, Ilias Apalodimas wrote: > > Hi Heinrich, > > > > On Wed, Nov 04, 2020 at 05:50:16PM +0100, Heinrich Schuchardt wrote: > >> On 04.11.20 16:56, Ilias Apalodimas wrote: > >>>>> > >>>>> u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > >>>>> - void *buf, size_t prop_count) > >>>>> + void *buf, size_t prop_count, bool get_count) > >>>> > >>>> The implementation would be more stable if we would derive the offset > >>>> from field property instead of adding get_count. > >>>> > >>> > >>> We are not defining the tpmv2 internal structures anywhere in U-Boot. > >>> That's why the code is doing static sizeof(uX) instead of using offsetof. > >>> In the EFI part of the patchset, I've done exaclty that. > >>> Working with offsets on well defined struct is better, but out of scope for this > >>> patchset imho. > >>> We can look into refactoring the generic tpmv2 code once I add the rest of the EFI > >>> protocol parts? > >> > >> Can't we add the structures that we need to tpm-v2.h and use their size > >> here? > > > > Yes we can, but the scope of the patch is adding EFI_TCG2_PROTOCOL, not re-factor the tpmv2 > > code along the way. > > Since the only change on the existing code to support the functionality needed for the > > EFI_TCG2_PROTOCOL protocol is just 4 bytes on an existing buffer, can we stick to that and > > think of refactoring the tpm stuff afterwards? > > You expect a TPML_PCR_SELECTION when capabilities is TPM_CAP_PCRS. > > typedef struct { > UINT32 count; > TPMS_PCR_SELECTION pcrSelections[TPM2_NUM_PCR_BANKS]; > } TPML_PCR_SELECTION; > > Why do you have to skip over the UINT32 here? You *don't* have to skip it since you need to implement GetCapabilty() and specifically the HashAlgorithmBitmap and ActivePcrBanks. That's what this patch is actually doing, preserve the u32 (corresponding to count). > > You just have to define this one structure in preparation of patch 2 and > then you can correctly parse the response in your implementation of the > EFI protocol. The struct is defined in this patchset, it's part of patch 2 and used to implement the EFI protocol. > > I suggest not to change tpm2_get_capability() at all. You can't. The current code has this comment: " * In the response buffer, the properties are located after the: * tag (u16), response size (u32), response code (u32), * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32)." So unless I am missing something it's referring to: typedef struct { UINT32 count; TPMS_TAGGED_PROPERTY tpmProperty[MAX_TPM_PROPERTIES]; } TPML_TAGGED_TPM_PROPERTY; So the last u32 that's removed from the buffer is the 'count', which it removes to access the tpmProperty directly. Regards /Ilias > > Best regards > > Heinrich > > > > > Regards > > /Ilias > >> > >> Best regards > >> > >> Heinrich > >> > >>> > >>>>> { > >>>>> u8 command_v2[COMMAND_BUFFER_SIZE] = { > >>>> > >>>> Shouldn't COMMAND_BUFFER_SIZE be changed to something with TPM in the > >>>> name, e.g TPM_COMMAND_BUFFER_SIZE? > >>>> > >>>>> tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */ > >>>>> @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > >>>>> if (ret) > >>>>> return ret; > >>>>> > >>>>> + /* When reading PCR properties we need the count */ > >>>>> + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > >>>>> + sizeof(u8) + sizeof(u32); > >>>>> /* > >>>>> * In the response buffer, the properties are located after the: > >>>>> * tag (u16), response size (u32), response code (u32), > >>>>> * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32). > >>>>> */ > >>>> > >>>> This comment should be above 'properties_off ='. 'get_count' related > >>>> field should be mentioned. > >>> > >>> Sure, I'll fix this > >>> > >>> Regards > >>> /Ilias > >>>> > >>>> Best regards > >>>> > >>>> Heinrich > >>>> > >>>>> - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > >>>>> - sizeof(u8) + sizeof(u32) + sizeof(u32); > >>>>> + if (!get_count) > >>>>> + properties_off += sizeof(u32); > >>>>> + > >>>>> memcpy(buf, &response[properties_off], response_len - properties_off); > >>>>> > >>>>> return 0; > >>>>> > >>>> > >> >
On 11/5/20 1:22 AM, Ilias Apalodimas wrote: > On Wed, Nov 04, 2020 at 10:59:03PM +0100, Heinrich Schuchardt wrote: >> On 11/4/20 6:26 PM, Ilias Apalodimas wrote: >>> Hi Heinrich, >>> >>> On Wed, Nov 04, 2020 at 05:50:16PM +0100, Heinrich Schuchardt wrote: >>>> On 04.11.20 16:56, Ilias Apalodimas wrote: >>>>>>> >>>>>>> u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, >>>>>>> - void *buf, size_t prop_count) >>>>>>> + void *buf, size_t prop_count, bool get_count) >>>>>> >>>>>> The implementation would be more stable if we would derive the offset >>>>>> from field property instead of adding get_count. >>>>>> >>>>> >>>>> We are not defining the tpmv2 internal structures anywhere in U-Boot. >>>>> That's why the code is doing static sizeof(uX) instead of using offsetof. >>>>> In the EFI part of the patchset, I've done exaclty that. >>>>> Working with offsets on well defined struct is better, but out of scope for this >>>>> patchset imho. >>>>> We can look into refactoring the generic tpmv2 code once I add the rest of the EFI >>>>> protocol parts? >>>> >>>> Can't we add the structures that we need to tpm-v2.h and use their size >>>> here? >>> >>> Yes we can, but the scope of the patch is adding EFI_TCG2_PROTOCOL, not re-factor the tpmv2 >>> code along the way. >>> Since the only change on the existing code to support the functionality needed for the >>> EFI_TCG2_PROTOCOL protocol is just 4 bytes on an existing buffer, can we stick to that and >>> think of refactoring the tpm stuff afterwards? >> >> You expect a TPML_PCR_SELECTION when capabilities is TPM_CAP_PCRS. >> >> typedef struct { >> UINT32 count; >> TPMS_PCR_SELECTION pcrSelections[TPM2_NUM_PCR_BANKS]; >> } TPML_PCR_SELECTION; >> >> Why do you have to skip over the UINT32 here? > > You *don't* have to skip it since you need to implement GetCapabilty() and specifically > the HashAlgorithmBitmap and ActivePcrBanks. > That's what this patch is actually doing, preserve the u32 (corresponding to count). > >> >> You just have to define this one structure in preparation of patch 2 and >> then you can correctly parse the response in your implementation of the >> EFI protocol. > > The struct is defined in this patchset, it's part of patch 2 and used to implement > the EFI protocol. > >> >> I suggest not to change tpm2_get_capability() at all. > > You can't. > > The current code has this comment: > " * In the response buffer, the properties are located after the: > * tag (u16), response size (u32), response code (u32), > * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32)." > > So unless I am missing something it's referring to: > typedef struct { > UINT32 count; > TPMS_TAGGED_PROPERTY tpmProperty[MAX_TPM_PROPERTIES]; > } TPML_TAGGED_TPM_PROPERTY; > > So the last u32 that's removed from the buffer is the 'count', which it removes to access > the tpmProperty directly. > do_tpm_get_capability() can request any combination of capability and property. For most capabilities only a single value of property is allowable according to "Trusted Platform Module LibraryPart 3: Commands". The return type depends on the capability. All relevant return types start with a u32 count field followed by an array of structures. The type of the structure depends on the capability. The description of the 'tpm2 get_capabilities' command seems to be incorrect in this light: "get_capability <capability> <property> <addr> <count>\n" " Read and display <count> entries indexed by <capability>/<property>.\n" " Values are 4 bytes long and are written at <addr>.\n" " <capability>: capability\n" " <property>: property\n There are no 4 byte return values to be shown. Without the count field the hexdump cannot be interpreted as you will not know where random garbage in memory starts. So can't we simply remove 4 bytes from the offset. Then do_tpm_get_capability() will show the count as the first 4 bytes which is necessary anyway to interpret the output. And you get the output you need for the EFI protocol. do_tpm_get_capability() should better use print_hex_dump() for output instead of inventing its own formatting routine. In the long run the 'tpm2 get_capability' sub-command should be completely reworked to show structured output according to the capability being read. In the spec there is a constant #define TPM2_MAX_CAP_BUFFER 1024 which is used to define TPM2_MAX_TPM_PROPERTIES and other maximum array sizes. So using COMMAND_BUFFER_SIZE=256 for the length of the response buffer seems to be wrong in patch 2/2. Cf. TCG TSS2.0 Overview and Common Structures Specification, Version 0.90, Revision 03, January 4, 2018 Best regards Heinrich > Regards > /Ilias >> >> Best regards >> >> Heinrich >> >>> >>> Regards >>> /Ilias >>>> >>>> Best regards >>>> >>>> Heinrich >>>> >>>>> >>>>>>> { >>>>>>> u8 command_v2[COMMAND_BUFFER_SIZE] = { >>>>>> >>>>>> Shouldn't COMMAND_BUFFER_SIZE be changed to something with TPM in the >>>>>> name, e.g TPM_COMMAND_BUFFER_SIZE? >>>>>> >>>>>>> tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */ >>>>>>> @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, >>>>>>> if (ret) >>>>>>> return ret; >>>>>>> >>>>>>> + /* When reading PCR properties we need the count */ >>>>>>> + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + >>>>>>> + sizeof(u8) + sizeof(u32); >>>>>>> /* >>>>>>> * In the response buffer, the properties are located after the: >>>>>>> * tag (u16), response size (u32), response code (u32), >>>>>>> * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32). >>>>>>> */ >>>>>> >>>>>> This comment should be above 'properties_off ='. 'get_count' related >>>>>> field should be mentioned. >>>>> >>>>> Sure, I'll fix this >>>>> >>>>> Regards >>>>> /Ilias >>>>>> >>>>>> Best regards >>>>>> >>>>>> Heinrich >>>>>> >>>>>>> - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + >>>>>>> - sizeof(u8) + sizeof(u32) + sizeof(u32); >>>>>>> + if (!get_count) >>>>>>> + properties_off += sizeof(u32); >>>>>>> + >>>>>>> memcpy(buf, &response[properties_off], response_len - properties_off); >>>>>>> >>>>>>> return 0; >>>>>>> >>>>>> >>>> >>
On Thu, Nov 05, 2020 at 03:27:15AM +0100, Heinrich Schuchardt wrote: > On 11/5/20 1:22 AM, Ilias Apalodimas wrote: > > On Wed, Nov 04, 2020 at 10:59:03PM +0100, Heinrich Schuchardt wrote: > >> On 11/4/20 6:26 PM, Ilias Apalodimas wrote: > >>> Hi Heinrich, > >>> > >>> On Wed, Nov 04, 2020 at 05:50:16PM +0100, Heinrich Schuchardt wrote: > >>>> On 04.11.20 16:56, Ilias Apalodimas wrote: > >>>>>>> > >>>>>>> u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > >>>>>>> - void *buf, size_t prop_count) > >>>>>>> + void *buf, size_t prop_count, bool get_count) > >>>>>> > >>>>>> The implementation would be more stable if we would derive the offset > >>>>>> from field property instead of adding get_count. > >>>>>> > >>>>> > >>>>> We are not defining the tpmv2 internal structures anywhere in U-Boot. > >>>>> That's why the code is doing static sizeof(uX) instead of using offsetof. > >>>>> In the EFI part of the patchset, I've done exaclty that. > >>>>> Working with offsets on well defined struct is better, but out of scope for this > >>>>> patchset imho. > >>>>> We can look into refactoring the generic tpmv2 code once I add the rest of the EFI > >>>>> protocol parts? > >>>> > >>>> Can't we add the structures that we need to tpm-v2.h and use their size > >>>> here? > >>> > >>> Yes we can, but the scope of the patch is adding EFI_TCG2_PROTOCOL, not re-factor the tpmv2 > >>> code along the way. > >>> Since the only change on the existing code to support the functionality needed for the > >>> EFI_TCG2_PROTOCOL protocol is just 4 bytes on an existing buffer, can we stick to that and > >>> think of refactoring the tpm stuff afterwards? > >> > >> You expect a TPML_PCR_SELECTION when capabilities is TPM_CAP_PCRS. > >> > >> typedef struct { > >> UINT32 count; > >> TPMS_PCR_SELECTION pcrSelections[TPM2_NUM_PCR_BANKS]; > >> } TPML_PCR_SELECTION; > >> > >> Why do you have to skip over the UINT32 here? > > > > You *don't* have to skip it since you need to implement GetCapabilty() and specifically > > the HashAlgorithmBitmap and ActivePcrBanks. > > That's what this patch is actually doing, preserve the u32 (corresponding to count). > > > >> > >> You just have to define this one structure in preparation of patch 2 and > >> then you can correctly parse the response in your implementation of the > >> EFI protocol. > > > > The struct is defined in this patchset, it's part of patch 2 and used to implement > > the EFI protocol. > > > >> > >> I suggest not to change tpm2_get_capability() at all. > > > > You can't. > > > > The current code has this comment: > > " * In the response buffer, the properties are located after the: > > * tag (u16), response size (u32), response code (u32), > > * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32)." > > > > So unless I am missing something it's referring to: > > typedef struct { > > UINT32 count; > > TPMS_TAGGED_PROPERTY tpmProperty[MAX_TPM_PROPERTIES]; > > } TPML_TAGGED_TPM_PROPERTY; > > > > So the last u32 that's removed from the buffer is the 'count', which it removes to access > > the tpmProperty directly. > > > > do_tpm_get_capability() can request any combination of capability and > property. > > For most capabilities only a single value of property is allowable > according to "Trusted Platform Module LibraryPart 3: Commands". > > The return type depends on the capability. > All relevant return types start with a u32 count field followed by an > array of structures. The type of the structure depends on the capability. > Exactly > The description of the 'tpm2 get_capabilities' command seems to be > incorrect in this light: > > "get_capability <capability> <property> <addr> <count>\n" > " Read and display <count> entries indexed by <capability>/<property>.\n" > " Values are 4 bytes long and are written at <addr>.\n" > " <capability>: capability\n" > " <property>: property\n > > There are no 4 byte return values to be shown. Without the count field > the hexdump cannot be interpreted as you will not know where random > garbage in memory starts. > > So can't we simply remove 4 bytes from the offset. Then > do_tpm_get_capability() will show the count as the first 4 bytes which > is necessary anyway to interpret the output. And you get the output you > need for the EFI protocol. We can remove it. But since this is a patch for for an EFI protocol, I wanted to keep the non EFI changes to a minimum and that's what this patch does. Get the extra 4 bytes of the count when required, without having to rewrite anything else on the TPM libraries. > > do_tpm_get_capability() should better use print_hex_dump() for output > instead of inventing its own formatting routine. In the long run the > 'tpm2 get_capability' sub-command should be completely reworked to show > structured output according to the capability being read. > > In the spec there is a constant > #define TPM2_MAX_CAP_BUFFER 1024 > which is used to define TPM2_MAX_TPM_PROPERTIES and other maximum array > sizes. So using COMMAND_BUFFER_SIZE=256 for the length of the response > buffer seems to be wrong in patch 2/2. patch 2/2 uses it because the tpm2_get_capability() uses it. So since the latter is using to talk to the tpm and then copy that into the provided buffer, you'll never get more than 256 whatever you end up defining this in patch 2/2. Regards /Ilias > > Cf. TCG TSS2.0 Overview and Common Structures Specification, Version > 0.90, Revision 03, January 4, 2018 > > Best regards > > Heinrich > > > Regards > > /Ilias > >> > >> Best regards > >> > >> Heinrich > >> > >>> > >>> Regards > >>> /Ilias > >>>> > >>>> Best regards > >>>> > >>>> Heinrich > >>>> > >>>>> > >>>>>>> { > >>>>>>> u8 command_v2[COMMAND_BUFFER_SIZE] = { > >>>>>> > >>>>>> Shouldn't COMMAND_BUFFER_SIZE be changed to something with TPM in the > >>>>>> name, e.g TPM_COMMAND_BUFFER_SIZE? > >>>>>> > >>>>>>> tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */ > >>>>>>> @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, > >>>>>>> if (ret) > >>>>>>> return ret; > >>>>>>> > >>>>>>> + /* When reading PCR properties we need the count */ > >>>>>>> + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > >>>>>>> + sizeof(u8) + sizeof(u32); > >>>>>>> /* > >>>>>>> * In the response buffer, the properties are located after the: > >>>>>>> * tag (u16), response size (u32), response code (u32), > >>>>>>> * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32). > >>>>>>> */ > >>>>>> > >>>>>> This comment should be above 'properties_off ='. 'get_count' related > >>>>>> field should be mentioned. > >>>>> > >>>>> Sure, I'll fix this > >>>>> > >>>>> Regards > >>>>> /Ilias > >>>>>> > >>>>>> Best regards > >>>>>> > >>>>>> Heinrich > >>>>>> > >>>>>>> - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + > >>>>>>> - sizeof(u8) + sizeof(u32) + sizeof(u32); > >>>>>>> + if (!get_count) > >>>>>>> + properties_off += sizeof(u32); > >>>>>>> + > >>>>>>> memcpy(buf, &response[properties_off], response_len - properties_off); > >>>>>>> > >>>>>>> return 0; > >>>>>>> > >>>>>> > >>>> > >> >
diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c index e6742656f578..c2df1c34043a 100644 --- a/cmd/tpm-v2.c +++ b/cmd/tpm-v2.c @@ -183,7 +183,7 @@ static int do_tpm_get_capability(struct cmd_tbl *cmdtp, int flag, int argc, data = map_sysmem(simple_strtoul(argv[3], NULL, 0), 0); count = simple_strtoul(argv[4], NULL, 0); - rc = tpm2_get_capability(dev, capability, property, data, count); + rc = tpm2_get_capability(dev, capability, property, data, count, false); if (rc) goto unmap_data; diff --git a/include/tpm-v2.h b/include/tpm-v2.h index f6c045d35480..ee74028ca83b 100644 --- a/include/tpm-v2.h +++ b/include/tpm-v2.h @@ -257,15 +257,17 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz, * to query property index that is 4-byte wide. * * @dev TPM device - * @capability Partition of capabilities - * @property Further definition of capability, limited to be 4 bytes wide - * @buf Output buffer for capability information - * @prop_count Size of output buffer + * @capability Partition of capabilities + * @property Further definition of capability, limited to be 4 bytes + * wide + * @buf Output buffer for capability information + * @prop_count Size of output buffer + * @get_count Include tpmu property count * * @return code of the operation */ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, - void *buf, size_t prop_count); + void *buf, size_t prop_count, bool get_count); /** * Issue a TPM2_DictionaryAttackLockReset command. diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c index a4c352e3ef75..b58c1057995b 100644 --- a/lib/tpm-v2.c +++ b/lib/tpm-v2.c @@ -161,7 +161,7 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz, } u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, - void *buf, size_t prop_count) + void *buf, size_t prop_count, bool get_count) { u8 command_v2[COMMAND_BUFFER_SIZE] = { tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */ @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property, if (ret) return ret; + /* When reading PCR properties we need the count */ + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + + sizeof(u8) + sizeof(u32); /* * In the response buffer, the properties are located after the: * tag (u16), response size (u32), response code (u32), * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32). */ - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) + - sizeof(u8) + sizeof(u32) + sizeof(u32); + if (!get_count) + properties_off += sizeof(u32); + memcpy(buf, &response[properties_off], response_len - properties_off); return 0;
A following patch introduces EFI_TCG2_PROTOCOL. One of the functions of that protocol is GetCapability(). In order to parse device capabilities we need to access a u32 before the properties which the current implementation ignores while reading device properties. So let's make the response length configurable and prepare the functions for EFI_TCG2_PROTOCOL. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- cmd/tpm-v2.c | 2 +- include/tpm-v2.h | 12 +++++++----- lib/tpm-v2.c | 10 +++++++--- 3 files changed, 15 insertions(+), 9 deletions(-) -- 2.29.2