| Message ID | 20230530063932.310959-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: explicitly return EFI_UNSUPPORTED for TCG 1.0 compatibility | expand |
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@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. Thanks, Stuart
Hi Stuart, On Wed, 31 May 2023 at 00:20, Stuart Yoder <stuart.yoder@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@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
On 5/31/23 2:48 AM, Ilias Apalodimas wrote: > Hi Stuart, > > On Wed, 31 May 2023 at 00:20, Stuart Yoder <stuart.yoder@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@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
Am 30. Mai 2023 23:20:53 MESZ schrieb Stuart Yoder <stuart.yoder@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@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
Hi Stuart, [...] > >> 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? It does but that violates the spec once again. The spec says that the firmware should return BOOT_SERVICE_CAPABILITY_MIN if the size is less than the size of the EFI_TCG2_BOOT_SERVICE_CAPABILITY. The current logic is similar, the only thing that differs on your patch is the size of the struct we return. Thanks /Ilias > > Thanks, > Stuart
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@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@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
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]
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]
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
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
On 5/31/23 3:10 PM, Heinrich Schuchardt wrote: > 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/ I can't find any public spec with the 1.0 struct. >> >> 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. The spec is not clear. And the committee that owns the spec provided the clarifications I outlined. They were supposed to provide an errata update to publish those clarifications, but it seems somehow that didn't happen. I specifically defined the SCT test spec based on what the committee told me: https://github.com/stuyod01/edk2-test/blob/master/uefi-sct/Doc/UEFI-SCT-Case-Spec/30_Protocols_TCG2_Test.md The Arm created tests match what I've been told is the the _intent_ of the spec. What is missing is getting TCG to publish errata documenting that. Thanks, Stuart
Am 31. Mai 2023 22:40:23 MESZ schrieb Stuart Yoder <stuart.yoder@arm.com>: > > >On 5/31/23 3:10 PM, Heinrich Schuchardt wrote: >> 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/ > >I can't find any public spec with the 1.0 struct. If it does not exist in a specification, why care about it? > >>> >>> 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. > >The spec is not clear. And the committee that owns the spec provided >the clarifications I outlined. They were supposed to provide an errata >update to publish those clarifications, but it seems somehow that >didn't happen. > >I specifically defined the SCT test spec based on what the committee >told me: >https://github.com/stuyod01/edk2-test/blob/master/uefi-sct/Doc/UEFI-SCT-Case-Spec/30_Protocols_TCG2_Test.md > >The Arm created tests match what I've been told is the the _intent_ of >the spec. What is missing is getting TCG to publish errata documenting >that. As you wrote above the tests don't relate to a known specification. Best regards Heinrich > >Thanks, >Stuart
On 5/31/23 5:09 PM, Heinrich Schuchardt wrote: > > > Am 31. Mai 2023 22:40:23 MESZ schrieb Stuart Yoder <stuart.yoder@arm.com>: >> >> >> On 5/31/23 3:10 PM, Heinrich Schuchardt wrote: >>> 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/ >> >> I can't find any public spec with the 1.0 struct. > > If it does not exist in a specification, why care about it? In theory there could be old clients from 6+ years ago that were built to support the 1.0 struct. But, this seems unlikely given how much time has passed. This is exactly why Ilias doesn't want to put support for the 1.0 struct in u-boot. We don't care about 1.0 clients. >> >>>> >>>> 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. >> >> The spec is not clear. And the committee that owns the spec provided >> the clarifications I outlined. They were supposed to provide an errata >> update to publish those clarifications, but it seems somehow that >> didn't happen. >> >> I specifically defined the SCT test spec based on what the committee >> told me: >> https://github.com/stuyod01/edk2-test/blob/master/uefi-sct/Doc/UEFI-SCT-Case-Spec/30_Protocols_TCG2_Test.md >> >> The Arm created tests match what I've been told is the the _intent_ of >> the spec. What is missing is getting TCG to publish errata documenting >> that. > > As you wrote above the tests don't relate to a known specification. I'm going to push TCG to publish the errata clarifying this. Once that is published the tests will match the spec. Thanks, Stuart
Am 1. Juni 2023 01:35:01 MESZ schrieb Stuart Yoder <stuart.yoder@arm.com>: > > >On 5/31/23 5:09 PM, Heinrich Schuchardt wrote: >> >> >> Am 31. Mai 2023 22:40:23 MESZ schrieb Stuart Yoder <stuart.yoder@arm.com>: >>> >>> >>> On 5/31/23 3:10 PM, Heinrich Schuchardt wrote: >>>> 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/ >>> >>> I can't find any public spec with the 1.0 struct. >> >> If it does not exist in a specification, why care about it? > >In theory there could be old clients from 6+ years ago that were >built to support the 1.0 struct. But, this seems unlikely >given how much time has passed. > >This is exactly why Ilias doesn't want to put support for the 1.0 >struct in u-boot. We don't care about 1.0 clients. > >>> >>>>> >>>>> 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. >>> >>> The spec is not clear. And the committee that owns the spec provided >>> the clarifications I outlined. They were supposed to provide an errata >>> update to publish those clarifications, but it seems somehow that >>> didn't happen. >>> >>> I specifically defined the SCT test spec based on what the committee >>> told me: >>> https://github.com/stuyod01/edk2-test/blob/master/uefi-sct/Doc/UEFI-SCT-Case-Spec/30_Protocols_TCG2_Test.md >>> >>> The Arm created tests match what I've been told is the the _intent_ of >>> the spec. What is missing is getting TCG to publish errata documenting >>> that. >> >> As you wrote above the tests don't relate to a known specification. > >I'm going to push TCG to publish the errata clarifying this. Once that >is published the tests will match the spec. > >Thanks, >Stuart https://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev9-150513_Public-Review.pdf is a draft version using ProtocolVersion = 1.3. I would assume 1.0 relates to an earlier draft not to a published specification. Best regards Heinrich
On 6/1/23 1:30 AM, Heinrich Schuchardt wrote: > > > Am 1. Juni 2023 01:35:01 MESZ schrieb Stuart Yoder <stuart.yoder@arm.com>: >> >> >> On 5/31/23 5:09 PM, Heinrich Schuchardt wrote: >>> >>> >>> Am 31. Mai 2023 22:40:23 MESZ schrieb Stuart Yoder <stuart.yoder@arm.com>: >>>> >>>> >>>> On 5/31/23 3:10 PM, Heinrich Schuchardt wrote: >>>>> 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/ >>>> >>>> I can't find any public spec with the 1.0 struct. >>> >>> If it does not exist in a specification, why care about it? >> >> In theory there could be old clients from 6+ years ago that were >> built to support the 1.0 struct. But, this seems unlikely >> given how much time has passed. >> >> This is exactly why Ilias doesn't want to put support for the 1.0 >> struct in u-boot. We don't care about 1.0 clients. >> >>>> >>>>>> >>>>>> 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. >>>> >>>> The spec is not clear. And the committee that owns the spec provided >>>> the clarifications I outlined. They were supposed to provide an errata >>>> update to publish those clarifications, but it seems somehow that >>>> didn't happen. >>>> >>>> I specifically defined the SCT test spec based on what the committee >>>> told me: >>>> https://github.com/stuyod01/edk2-test/blob/master/uefi-sct/Doc/UEFI-SCT-Case-Spec/30_Protocols_TCG2_Test.md >>>> >>>> The Arm created tests match what I've been told is the the _intent_ of >>>> the spec. What is missing is getting TCG to publish errata documenting >>>> that. >>> >>> As you wrote above the tests don't relate to a known specification. >> >> I'm going to push TCG to publish the errata clarifying this. Once that >> is published the tests will match the spec. >> >> Thanks, >> Stuart > > > https://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev9-150513_Public-Review.pdf > > is a draft version using ProtocolVersion = 1.3. I would assume 1.0 relates to an earlier draft not to a published specification. In 6.1 the spec says: A user of this protocol should call the EFI_TCG2_PROTOCOL.GetCapabilities operation (Section 6.4) to determine the functionality implemented by this interface. There are earlier implementations of this protocol that implement a subset of the functions and capabilities defined here. I don't know anything about the earlier implementations, but they seem to have existed at some point. The structure version is 1.1: 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. Thanks, Stuart
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; }
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@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(-) -- 2.39.2