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

[Ilias Apalodimas]

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

Thanks
/Ilias
>
> Thanks,
> Stuart