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

[Heinrich Schuchardt]

On 5/31/23 18:41, Stuart Yoder wrote:
>
>
> 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.

Please, open a bug report for the SCT. It should check the value of
ProtocolCapability.ProtocolVersion returned by GetCapability() and
provide code paths for 1.0 and 1.1.

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

This is contradicts the standard. You MUST return EFI_BUFFER_TOO_SMALL.
It is the clients obligation to check StructureVersion and ProtocolVersion.

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

This does not conform to the TCG standard. You MUST return
EFI_BUFFER_TO_SMALL in this case.

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

No, you cannot detect 1.0 clients this way.

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

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.

It is the client's obligation to check the values of fields
StructureVersion and ProtocolVersion.

Best regards

Heinrich

>
> Thanks,
> Stuart