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

Ilias Apalodimas ilias.apalodimas at linaro.org
Wed Nov 4 16:56:26 CET 2020


Hi Heinrich, 

[...]
> > +++ b/lib/tpm-v2.c
> > @@ -161,7 +161,7 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz,
> >  }
> >
> >  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?

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


More information about the U-Boot mailing list