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

[Ilias Apalodimas]

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