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

[Stuart Yoder]

On 5/31/23 2:48 AM, Ilias Apalodimas wrote:
> Hi Stuart,
> 
> On Wed, 31 May 2023 at 00:20, Stuart Yoder <stuart.yoder at arm.com> wrote:
>>
>>
>>
>> On 5/30/23 1:39 AM, Ilias Apalodimas wrote:
>>> In the EFI TCG spec EFI_TCG2_BOOT_SERVICE_CAPABILITY struct is
>>> versioned -- there are 1.0 and 1.1 versions of that struct.
>>> The spec [0] describes:
>>> "Version of the EFI_TCG2_BOOT_SERVICE_CAPABILITY
>>> structure itself. For this version of the protocol, the Major version
>>> SHALL be set to 1 and the Minor version SHALL be set to 1."
>>> which is what we currently support.
>>>
>>> The SCT tests perfromed By Arms SIE(Security interface extensions) [1]
>>> perform a check for clients supporting the older 1.0 version of the
>>> spec (Test 30.1.1.4). Given than this spec is 7 years old,  there should
>>> be no need for the older 1.0 version support.  Instead of returning
>>> EFI_BUFFER_TOO_SMALLL switch to EFI_UNSUPPORTED which is more
>>> appropriate.  It's worth noting that the spec doesn't explicitly
>>> describe the return value at the moment.
>>>
>>> [0] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
>>> [1] https://github.com/stuyod01/edk2-test/blob/master/uefi-sct/Doc/UEFI-SCT-Case-Spec/30_Protocols_TCG2_Test.md
>>>
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>>> ---
>>>
>>> Heinrich,  Stuart is investigating the chance of the spec getting updated
>>> adding EFI_UNSUPPORTED.  In any case I think the patch should be aplied since
>>> the new return code makes more sense.  If for some reason the spec change is
>>> rejected, I can go back and add support for 1.0 structure versions.
>>>
>>>    lib/efi_loader/efi_tcg2.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
>>> index a83ae7a46cf3..220c442bdf93 100644
>>> --- a/lib/efi_loader/efi_tcg2.c
>>> +++ b/lib/efi_loader/efi_tcg2.c
>>> @@ -750,7 +750,7 @@ efi_tcg2_get_capability(struct efi_tcg2_protocol *this,
>>>
>>>        if (capability->size < sizeof(*capability)) {
>>>                capability->size = sizeof(*capability);
>>> -             efi_ret = EFI_BUFFER_TOO_SMALL;
>>> +             efi_ret = EFI_UNSUPPORTED;
>>>                goto out;
>>>        }
>>
>> Hi Ilias,
>>
>> I don't think this is the right fix.
>>
>> The struct looks like this:
>>
>> struct tdEFI_TCG2_BOOT_SERVICE_CAPABILITY {
>>     UINT8 Size;
>>     EFI_TCG2_VERSION StructureVersion;
>>     EFI_TCG2_VERSION ProtocolVersion;
>>     EFI_TCG2_EVENT_ALGORITHM_BITMAP HashAlgorithmBitmap
>>     EFI_TCG2_EVENT_LOG_BITMAP SupportedEventLogs
>>     BOOLEAN TPMPresentFlag
>>     UINT16 MaxCommandSize
>>     UINT16 MaxResponseSize
>>     UINT32 ManufacturerID
>>     UINT32 NumberOfPcrBanks
>>     EFI_TCG2_EVENT_ALGORITHM_BITMAP ActivePcrBanks
>> };
>>
>> The intent in the TCG spec is for the caller to be able to
>> determine the size of the struct by passing in a small
>> buffer (e.g. 1 byte buffer), and then the firmware
>> should return sizeof(EFI_TCG2_BOOT_SERVICE_CAPABILITY) in the
>> 1-byte "Size" field.  And the return value should be
>> EFI_BUFFER_TOO_SMALL as per the spec.
>>
>> In order to detect a 1.0 client, which you don't want to support,
>> I think you need a _new_ check that is something like this:
>>
>> // detect 1.0 client
>> if (capability->size < sizeof(*capability) &&
>>       capability->size >= offsetof(EFI_TCG2_BOOT_SERVICE_CAPABILITY,
>> NumberOfPcrBanks)) {
>>       efi_ret = EFI_UNSUPPORTED;
>>       goto out;
>> }
>>
>> The last field in the 1.0 struct was ManufacturerID, so you should
>> be able to detect 1.0 clients that expect that based on the size they
>> pass in.
> 
> The patch doesn't show the entire u-boot code there and is a bit misleading.
> There's another check that precedes the one I am patching, which if I
> am reading it correctly does exactly what you describe
> 
> #define BOOT_SERVICE_CAPABILITY_MIN \
>            offsetof(struct efi_tcg2_boot_service_capability, number_of_pcr_banks)
> if (capability->size < BOOT_SERVICE_CAPABILITY_MIN) {
>            capability->size = BOOT_SERVICE_CAPABILITY_MIN;
>            efi_ret = EFI_BUFFER_TOO_SMALL;
>            goto out;
> }

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?

Thanks,
Stuart