[PATCH 0/3] Miscellaneous fixes of efi_tcg2
Arm is preparing the BBSR(Base Boot Security Requirement) compiance test tool. This tool covers Measured Boot, it checks TCG EFI Protocol compliance.
This patch series fix the failure items detected by this BBSR compliance test tool.
Masahisa Kojima (3): efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api efi_loader: fix boot_service_capability_min calculation efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
include/efi_tcg2.h | 4 +++- lib/efi_loader/efi_tcg2.c | 21 ++++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-)
TCG EFI Protocol Specification defines the required parameter checking and return value for each API. This commit adds the missing parameter check and fixes the wrong return value to comply the specification.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- lib/efi_loader/efi_tcg2.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index 35e69b9112..c4e9f61fd6 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -708,6 +708,18 @@ efi_tcg2_get_eventlog(struct efi_tcg2_protocol *this, EFI_ENTRY("%p, %u, %p, %p, %p", this, log_format, event_log_location, event_log_last_entry, event_log_truncated);
+ if (!this || !event_log_location || !event_log_last_entry || + !event_log_truncated) { + ret = EFI_INVALID_PARAMETER; + goto out; + } + + /* Only support TPMV2 */ + if (log_format != TCG2_EVENT_LOG_FORMAT_TCG_2) { + ret = EFI_INVALID_PARAMETER; + goto out; + } + ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) { event_log_location = NULL; @@ -965,6 +977,7 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, data_to_hash_len, (void **)&nt); if (ret != EFI_SUCCESS) { log_err("Not a valid PE-COFF file\n"); + ret = EFI_UNSUPPORTED; goto out; } ret = tcg2_hash_pe_image((void *)(uintptr_t)data_to_hash, @@ -1038,9 +1051,15 @@ efi_tcg2_get_active_pcr_banks(struct efi_tcg2_protocol *this, { efi_status_t ret;
+ if (!this || !active_pcr_banks) { + ret = EFI_INVALID_PARAMETER; + goto out; + } + EFI_ENTRY("%p, %p", this, active_pcr_banks); ret = __get_active_pcr_banks(active_pcr_banks);
+out: return EFI_EXIT(ret); }
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
On Fri, 3 Sept 2021 at 04:54, Masahisa Kojima masahisa.kojima@linaro.org wrote:
TCG EFI Protocol Specification defines the required parameter checking and return value for each API. This commit adds the missing parameter check and fixes the wrong return value to comply the specification.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
lib/efi_loader/efi_tcg2.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index 35e69b9112..c4e9f61fd6 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -708,6 +708,18 @@ efi_tcg2_get_eventlog(struct efi_tcg2_protocol *this, EFI_ENTRY("%p, %u, %p, %p, %p", this, log_format, event_log_location, event_log_last_entry, event_log_truncated);
if (!this || !event_log_location || !event_log_last_entry ||!event_log_truncated) {ret = EFI_INVALID_PARAMETER;goto out;}/* Only support TPMV2 */if (log_format != TCG2_EVENT_LOG_FORMAT_TCG_2) {ret = EFI_INVALID_PARAMETER;goto out;}ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) { event_log_location = NULL;@@ -965,6 +977,7 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, data_to_hash_len, (void **)&nt); if (ret != EFI_SUCCESS) { log_err("Not a valid PE-COFF file\n");
ret = EFI_UNSUPPORTED; goto out; } ret = tcg2_hash_pe_image((void *)(uintptr_t)data_to_hash,@@ -1038,9 +1051,15 @@ efi_tcg2_get_active_pcr_banks(struct efi_tcg2_protocol *this, { efi_status_t ret;
if (!this || !active_pcr_banks) {ret = EFI_INVALID_PARAMETER;goto out;}EFI_ENTRY("%p, %p", this, active_pcr_banks); ret = __get_active_pcr_banks(active_pcr_banks);+out: return EFI_EXIT(ret); }
-- 2.17.1
TCG EFI Protocol Specification requires to the input ProtocolCapability.Size < size of the EFI_TCG2_BOOT_SERVICE_CAPABILITY up to and including the vendor ID field. Current implementation does different calculation, let's fix it.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- include/efi_tcg2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index b6b958da51..45788d55d5 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -127,8 +127,8 @@ struct efi_tcg2_boot_service_capability { efi_tcg_event_algorithm_bitmap active_pcr_banks; };
+/* up to and including the vendor ID(manufacture_id) field */ #define boot_service_capability_min \ - sizeof(struct efi_tcg2_boot_service_capability) - \ offsetof(struct efi_tcg2_boot_service_capability, number_of_pcr_banks)
#define TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03 "Spec ID Event03"
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
On Fri, 3 Sept 2021 at 04:54, Masahisa Kojima masahisa.kojima@linaro.org wrote:
TCG EFI Protocol Specification requires to the input ProtocolCapability.Size < size of the EFI_TCG2_BOOT_SERVICE_CAPABILITY up to and including the vendor ID field. Current implementation does different calculation, let's fix it.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
include/efi_tcg2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index b6b958da51..45788d55d5 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -127,8 +127,8 @@ struct efi_tcg2_boot_service_capability { efi_tcg_event_algorithm_bitmap active_pcr_banks; };
+/* up to and including the vendor ID(manufacture_id) field */ #define boot_service_capability_min \
sizeof(struct efi_tcg2_boot_service_capability) - \ offsetof(struct efi_tcg2_boot_service_capability, number_of_pcr_banks)#define TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03 "Spec ID Event03"
2.17.1
On 9/3/21 3:55 AM, Masahisa Kojima wrote:
TCG EFI Protocol Specification requires to the input ProtocolCapability.Size < size of the EFI_TCG2_BOOT_SERVICE_CAPABILITY up to and including the vendor ID field.
Current implementation does different calculation, let's fix it.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
include/efi_tcg2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index b6b958da51..45788d55d5 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -127,8 +127,8 @@ struct efi_tcg2_boot_service_capability { efi_tcg_event_algorithm_bitmap active_pcr_banks; };
+/* up to and including the vendor ID(manufacture_id) field */
Thank you for fixing the issue. There is just a typo above:
%s/manufacture_id/manufacturer_id/
I can fix that when merging
#define boot_service_capability_min \
Constants should be capitalized. This can be corrected in a future patch.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
sizeof(struct efi_tcg2_boot_service_capability) - \ offsetof(struct efi_tcg2_boot_service_capability, number_of_pcr_banks)
#define TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03 "Spec ID Event03"
On Fri, 3 Sept 2021 at 16:01, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 9/3/21 3:55 AM, Masahisa Kojima wrote:
TCG EFI Protocol Specification requires to the input ProtocolCapability.Size < size of the EFI_TCG2_BOOT_SERVICE_CAPABILITY up to and including the vendor ID field.
Current implementation does different calculation, let's fix it.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
include/efi_tcg2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index b6b958da51..45788d55d5 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -127,8 +127,8 @@ struct efi_tcg2_boot_service_capability { efi_tcg_event_algorithm_bitmap active_pcr_banks; };
+/* up to and including the vendor ID(manufacture_id) field */
Thank you for fixing the issue. There is just a typo above:
%s/manufacture_id/manufacturer_id/
I can fix that when merging
Sorry for my typo, thank you.
#define boot_service_capability_min \
Constants should be capitalized. This can be corrected in a future patch.
Thank you, I will fix it after this patch merged.
Thanks, Masahisa Kojima
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
sizeof(struct efi_tcg2_boot_service_capability) - \ offsetof(struct efi_tcg2_boot_service_capability, number_of_pcr_banks)#define TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03 "Spec ID Event03"
TCG EFI Protocol Specification defines that PCRIndex parameter passed from caller must be 0 to 23. TPM2_MAX_PCRS is currently used to check the range of PCRIndex, but TPM2_MAX_PCRS is tpm2 device dependent and may have larger value. This commit newly adds EFI_TCG2_MAX_PCR_INDEX macro, it is used to check the range of PCRIndex parameter.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- include/efi_tcg2.h | 2 ++ lib/efi_loader/efi_tcg2.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index 45788d55d5..b647361d44 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -28,6 +28,8 @@ #define EFI_TCG2_EXTEND_ONLY 0x0000000000000001 #define PE_COFF_IMAGE 0x0000000000000010
+#define EFI_TCG2_MAX_PCR_INDEX 23 + /* Algorithm Registry */ #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001 #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002 diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index c4e9f61fd6..b268a02976 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -958,7 +958,7 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, goto out; }
- if (efi_tcg_event->header.pcr_index > TPM2_MAX_PCRS) { + if (efi_tcg_event->header.pcr_index > EFI_TCG2_MAX_PCR_INDEX) { ret = EFI_INVALID_PARAMETER; goto out; }
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
On Fri, 3 Sept 2021 at 04:54, Masahisa Kojima masahisa.kojima@linaro.org wrote:
TCG EFI Protocol Specification defines that PCRIndex parameter passed from caller must be 0 to 23. TPM2_MAX_PCRS is currently used to check the range of PCRIndex, but TPM2_MAX_PCRS is tpm2 device dependent and may have larger value. This commit newly adds EFI_TCG2_MAX_PCR_INDEX macro, it is used to check the range of PCRIndex parameter.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
include/efi_tcg2.h | 2 ++ lib/efi_loader/efi_tcg2.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index 45788d55d5..b647361d44 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -28,6 +28,8 @@ #define EFI_TCG2_EXTEND_ONLY 0x0000000000000001 #define PE_COFF_IMAGE 0x0000000000000010
+#define EFI_TCG2_MAX_PCR_INDEX 23
/* Algorithm Registry */ #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001 #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002 diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index c4e9f61fd6..b268a02976 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -958,7 +958,7 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, goto out; }
if (efi_tcg_event->header.pcr_index > TPM2_MAX_PCRS) {
if (efi_tcg_event->header.pcr_index > EFI_TCG2_MAX_PCR_INDEX) { ret = EFI_INVALID_PARAMETER; goto out; }-- 2.17.1
On 9/3/21 3:55 AM, Masahisa Kojima wrote:
TCG EFI Protocol Specification defines that PCRIndex parameter passed from caller must be 0 to 23. TPM2_MAX_PCRS is currently used to check the range of PCRIndex, but TPM2_MAX_PCRS is tpm2 device dependent and may have larger value. This commit newly adds EFI_TCG2_MAX_PCR_INDEX macro, it is used to check the range of PCRIndex parameter.
In the U-Boot code we have TPM2_MAX_PCRS hard coded as 32. Can the value be less?
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
include/efi_tcg2.h | 2 ++ lib/efi_loader/efi_tcg2.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index 45788d55d5..b647361d44 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -28,6 +28,8 @@ #define EFI_TCG2_EXTEND_ONLY 0x0000000000000001 #define PE_COFF_IMAGE 0x0000000000000010
+#define EFI_TCG2_MAX_PCR_INDEX 23
- /* Algorithm Registry */ #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001 #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index c4e9f61fd6..b268a02976 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -958,7 +958,7 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, goto out; }
- if (efi_tcg_event->header.pcr_index > TPM2_MAX_PCRS) {
If TPM2_MAX_PCRS were device dependent and could be less then 23 we would need a check against both constants.
Could you, please, clarify the issue. If TPM2_MAX_PCRS is always greater then 23, please, mention this in the commit message and perhaps add a comment in the code here.
Best regards
Heinrich
- if (efi_tcg_event->header.pcr_index > EFI_TCG2_MAX_PCR_INDEX) { ret = EFI_INVALID_PARAMETER; goto out; }
Hi Heinrich,
On Fri, Sep 03, 2021 at 08:22:30AM +0200, Heinrich Schuchardt wrote:
On 9/3/21 3:55 AM, Masahisa Kojima wrote:
TCG EFI Protocol Specification defines that PCRIndex parameter passed from caller must be 0 to 23. TPM2_MAX_PCRS is currently used to check the range of PCRIndex, but TPM2_MAX_PCRS is tpm2 device dependent and may have larger value. This commit newly adds EFI_TCG2_MAX_PCR_INDEX macro, it is used to check the range of PCRIndex parameter.
In the U-Boot code we have TPM2_MAX_PCRS hard coded as 32. Can the value be less?
This is defined in [1] [1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_Overview_Common_v0p...
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
include/efi_tcg2.h | 2 ++ lib/efi_loader/efi_tcg2.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index 45788d55d5..b647361d44 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -28,6 +28,8 @@ #define EFI_TCG2_EXTEND_ONLY 0x0000000000000001 #define PE_COFF_IMAGE 0x0000000000000010
+#define EFI_TCG2_MAX_PCR_INDEX 23
- /* Algorithm Registry */ #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001 #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index c4e9f61fd6..b268a02976 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -958,7 +958,7 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, goto out; }
- if (efi_tcg_event->header.pcr_index > TPM2_MAX_PCRS) {
If TPM2_MAX_PCRS were device dependent and could be less then 23 we would need a check against both constants.
Could you, please, clarify the issue. If TPM2_MAX_PCRS is always greater then 23, please, mention this in the commit message and perhaps add a comment in the code here.
You don't need that (I think). If the spec says you have to check against 23, then that's what Kojima-san fixes here. If the device supports less than 23, then the current code as-is will return EFI_DEVICE_ERROR, in case the device has less than 23 PCRs.
We could ofc just check against the value we get from GetCapability, which would be correct as well?
Best regards
Heinrich
- if (efi_tcg_event->header.pcr_index > EFI_TCG2_MAX_PCR_INDEX) { ret = EFI_INVALID_PARAMETER; goto out; }
[1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_Overview_Common_v0p...
Regards /Ilias
On 9/3/21 8:51 AM, Ilias Apalodimas wrote:
Hi Heinrich,
On Fri, Sep 03, 2021 at 08:22:30AM +0200, Heinrich Schuchardt wrote:
On 9/3/21 3:55 AM, Masahisa Kojima wrote:
TCG EFI Protocol Specification defines that PCRIndex parameter passed from caller must be 0 to 23. TPM2_MAX_PCRS is currently used to check the range of PCRIndex, but TPM2_MAX_PCRS is tpm2 device dependent and may have larger value. This commit newly adds EFI_TCG2_MAX_PCR_INDEX macro, it is used to check the range of PCRIndex parameter.
In the U-Boot code we have TPM2_MAX_PCRS hard coded as 32. Can the value be less?
This is defined in [1] [1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_Overview_Common_v0p...
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
include/efi_tcg2.h | 2 ++ lib/efi_loader/efi_tcg2.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index 45788d55d5..b647361d44 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -28,6 +28,8 @@ #define EFI_TCG2_EXTEND_ONLY 0x0000000000000001 #define PE_COFF_IMAGE 0x0000000000000010
+#define EFI_TCG2_MAX_PCR_INDEX 23
- /* Algorithm Registry */ #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001 #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index c4e9f61fd6..b268a02976 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -958,7 +958,7 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, goto out; }
- if (efi_tcg_event->header.pcr_index > TPM2_MAX_PCRS) {
If TPM2_MAX_PCRS were device dependent and could be less then 23 we would need a check against both constants.
Could you, please, clarify the issue. If TPM2_MAX_PCRS is always greater then 23, please, mention this in the commit message and perhaps add a comment in the code here.
You don't need that (I think). If the spec says you have to check against 23, then that's what Kojima-san fixes here. If the device supports less than 23, then the current code as-is will return EFI_DEVICE_ERROR, in case the device has less than 23 PCRs.
We could ofc just check against the value we get from GetCapability, which would be correct as well?
Acked-by: Heinrich Schuchardt xypron.glpk@gmx.de
Best regards
Heinrich
- if (efi_tcg_event->header.pcr_index > EFI_TCG2_MAX_PCR_INDEX) { ret = EFI_INVALID_PARAMETER; goto out; }
[1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_Overview_Common_v0p...
Regards /Ilias
participants (3)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Masahisa Kojima