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

[Ilias Apalodimas]

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