[PATCH] efi_loader: explicitly return EFI_UNSUPPORTED for TCG 1.0 compatibility

[Heinrich Schuchardt]

On 5/31/23 21:37, Stuart Yoder wrote:
>
> Unfortunately, the TCG spec is very confusing in section 6.4.4 #2 and
> #3.  They attempted to clarify in an errata:
> https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-Errata-v.5.pdf
>
> ...but it is still confusing.
>
> Ilias and I had discussed the ambiguities, and back in March 2022 I
> requested clarification from the TCG workgroup.  In cases of
> ambiguity TCG frequently will defer to how EDK2 has implemented
> a point in the spec.
>
> Here are my notes following the call with TCG about the intent
> of #2 and #3, which was based on their review of the EDK2
> implementation:
>
> a. If a client passes in a Size that is the full size including all
>     fields including ActivePcrBanks, the return code is SUCCESS and
>     all fields are populated. [This is a 1.1 client scenario]
>
> b. If a client passes in a Size that includes all fields up to
>     and including the vendor ID, the return code is SUCCESS and all
>     fields up to including the vendor ID are populated. [This is a
>     1.0 client scenario, so a populated 1.0 struct is returned]

This contradicts the TCG EFI Protocol Specifiction which knows of no 1.0
structure but requires:

If the input ProtocolCapability.Size <
sizeof(EFI_TCG2_BOOT_SERVICE_CAPABILITY) the function will initialize
the fields included in ProtocolCapability.Size. The values of the
remaining fields will be undefined.

We should stick with what is specified.

The above requirement is not yet implemented in U-Boot.

Could you, please, indicated where the 1.0 structure was ever defined. I
could not find any a document linked on
https://trustedcomputinggroup.org/resource/tcg-efi-protocol-specification/

>
> c. If a client passes in a Size that is less than the size up to
>     and including the vendor ID, the return code is BUFFER_TOO_SMALL
>     and the Size field is populated with the full size of the struct
>     supported by the firmware. [This allows a client to determine
>     whether it is talking to 1.0 or 1.1 firmware]

Yes, it is the client's task to check the protocol version and not the
firmware's task to guess what the client has in mind.

ARM should fix their tests that don't comply with the TCG EFI Protocol
Specification and then upstream them to edk-test. U-Boot should not try
to work around incorrect vendor tests.

Best regards

Heinrich

>
> What Ilias wants to avoid is supporting #b above in u-boot, and
> I agree that having u-boot support clients based on struct 1.0
> seems unnecessary.
>
>>> I took a look at the u-boot code.  What I think we want is this:
>>>
>>> diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
>>> index b1c3abd097..6a46d9e51c 100644
>>> --- a/include/efi_tcg2.h
>>> +++ b/include/efi_tcg2.h
>>> @@ -126,7 +126,7 @@ struct efi_tcg2_boot_service_capability {
>>>   };
>>>
>>>   /* up to and including the vendor ID (manufacturer_id) field */
>>> -#define BOOT_SERVICE_CAPABILITY_MIN \
>>> +#define BOOT_SERVICE_CAPABILITY_1_0 \
>>>          offsetof(struct efi_tcg2_boot_service_capability,
>>> number_of_pcr_banks)
>>>
>>>   #define TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03 "Spec ID Event03"
>>> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
>>> index a83ae7a46c..d37b896b7e 100644
>>> --- a/lib/efi_loader/efi_tcg2.c
>>> +++ b/lib/efi_loader/efi_tcg2.c
>>> @@ -742,15 +742,15 @@ efi_tcg2_get_capability(struct efi_tcg2_protocol
>>> *this,
>>>                  goto out;
>>>          }
>>>
>>> -       if (capability->size < BOOT_SERVICE_CAPABILITY_MIN) {
>>> -               capability->size = BOOT_SERVICE_CAPABILITY_MIN;
>>> +       if (capability->size < BOOT_SERVICE_CAPABILITY_1_0) {
>>> +               capability->size = sizeof(*capability);
>>>                  efi_ret = EFI_BUFFER_TOO_SMALL;
>>>                  goto out;
>>>          }
>>>
>>>          if (capability->size < sizeof(*capability)) {
>>>                  capability->size = sizeof(*capability);
>>> -               efi_ret = EFI_BUFFER_TOO_SMALL;
>>> +               efi_ret = EFI_UNSUPPORTED;
>>>                  goto out;
>>>          }
>>>
>>> That is:
>>>
>>> -if the passed in size is less than the 1.0 struct size
>>>   then return sizeof() the full struct.  This allows
>>>   client to query and determine the supported struct
>>>   size
>>>
>>> -to detect and reject 1.0 clients, if the passed in
>>>   size is >= 1.0 struct size AND less than sizeof(*capability)
>>>   then return EFI_UNSUPPORTED
>>>
>>> Does that make sense?
>>
>> No, this will not work. A 1.0 client may call with size = 0 and next
>> will call with whatever size was returned accompanied by
>> EFI_BUFFER_TOO_SMALL.
>
> The spec defines the Size field as "Allocated size of the
> structure.", so I don't see how a value of 0 makes sense.
>
> If a client wants to just get the Size field populated, pass in
> a buffer of Size=1 and firmware will return just the Size field
> populated with the correct struct size.
>
>> It is the client's obligation to check the values of fields
>> StructureVersion and ProtocolVersion.
>
> Yes, but what if the client fails to do that? What if a client
> passes in a struct with a size corresponding to a 1.0 struct?
> What should firmware do?
>
> According to point #b above, firmware should support the 1.0
> client and populate only the 1.0 fields.  But, what we are
> proposing is not supporting that in u-boot.  Instead return
> EFI_UNSUPPORTED.  I can discuss this with the TCG WG.
>
> Thanks,
> Stuart