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; > >>>>> > >>>> > >> >