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

[Heinrich Schuchardt]

Am 30. Mai 2023 23:20:53 MESZ schrieb Stuart Yoder <stuart.yoder at arm.com>:
>
>
>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.

Typically clients will pass in 0 to retrieve the size.

Best regards

Heinrich

>
>Thanks,
>Stuart