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