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.yo...@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.apalodi...@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
