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

Ilias Apalodimas ilias.apalodimas at linaro.org
Thu Nov 5 07:53:47 CET 2020


On Thu, Nov 05, 2020 at 03:27:15AM +0100, Heinrich Schuchardt wrote:
> 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.
> 

Exactly

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

We can remove it. But since this is a patch for for an EFI protocol, 
I wanted to keep the non EFI changes to a minimum and that's what this patch does. 
Get the extra 4 bytes of the count when required, without having to rewrite 
anything else on the TPM libraries.

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

patch 2/2 uses it because the tpm2_get_capability() uses it. 
So since the latter is using to talk to the tpm and then copy that into the provided 
buffer, you'll never get more than 256 whatever you end up defining this in patch 2/2.

Regards
/Ilias
> 
> 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;
> >>>>>>>
> >>>>>>
> >>>>
> >>
> 


More information about the U-Boot mailing list