[PATCH 1/2] tpm: Make response length of tpm2_get_capability() configurable

[Heinrich Schuchardt]

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