* [PATCH 0/3] Miscellaneous fixes of efi_tcg2
@ 2021-09-03 1:55 Masahisa Kojima
2021-09-03 1:55 ` [PATCH 1/3] efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api Masahisa Kojima
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Masahisa Kojima @ 2021-09-03 1:55 UTC (permalink / raw)
To: Heinrich Schuchardt, Alexander Graf, Ilias Apalodimas,
Simon Glass, AKASHI Takahiro, u-boot
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(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api
2021-09-03 1:55 [PATCH 0/3] Miscellaneous fixes of efi_tcg2 Masahisa Kojima
@ 2021-09-03 1:55 ` Masahisa Kojima
2021-09-03 6:25 ` Ilias Apalodimas
2021-09-03 1:55 ` [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation Masahisa Kojima
2021-09-03 1:55 ` [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check Masahisa Kojima
2 siblings, 1 reply; 12+ messages in thread
From: Masahisa Kojima @ 2021-09-03 1:55 UTC (permalink / raw)
To: Heinrich Schuchardt, Alexander Graf, Ilias Apalodimas,
Simon Glass, AKASHI Takahiro, u-boot
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation
2021-09-03 1:55 [PATCH 0/3] Miscellaneous fixes of efi_tcg2 Masahisa Kojima
2021-09-03 1:55 ` [PATCH 1/3] efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api Masahisa Kojima
@ 2021-09-03 1:55 ` Masahisa Kojima
2021-09-03 6:19 ` Ilias Apalodimas
2021-09-03 7:01 ` Heinrich Schuchardt
2021-09-03 1:55 ` [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check Masahisa Kojima
2 siblings, 2 replies; 12+ messages in thread
From: Masahisa Kojima @ 2021-09-03 1:55 UTC (permalink / raw)
To: Heinrich Schuchardt, Alexander Graf, Ilias Apalodimas,
Simon Glass, AKASHI Takahiro, u-boot
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
2021-09-03 1:55 [PATCH 0/3] Miscellaneous fixes of efi_tcg2 Masahisa Kojima
2021-09-03 1:55 ` [PATCH 1/3] efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api Masahisa Kojima
2021-09-03 1:55 ` [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation Masahisa Kojima
@ 2021-09-03 1:55 ` Masahisa Kojima
2021-09-03 6:20 ` Ilias Apalodimas
2021-09-03 6:22 ` Heinrich Schuchardt
2 siblings, 2 replies; 12+ messages in thread
From: Masahisa Kojima @ 2021-09-03 1:55 UTC (permalink / raw)
To: Heinrich Schuchardt, Alexander Graf, Ilias Apalodimas,
Simon Glass, AKASHI Takahiro, u-boot
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation
2021-09-03 1:55 ` [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation Masahisa Kojima
@ 2021-09-03 6:19 ` Ilias Apalodimas
2021-09-03 7:01 ` Heinrich Schuchardt
1 sibling, 0 replies; 12+ messages in thread
From: Ilias Apalodimas @ 2021-09-03 6:19 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Heinrich Schuchardt, Alexander Graf, Simon Glass,
AKASHI Takahiro, U-Boot Mailing List
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
2021-09-03 1:55 ` [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check Masahisa Kojima
@ 2021-09-03 6:20 ` Ilias Apalodimas
2021-09-03 6:22 ` Heinrich Schuchardt
1 sibling, 0 replies; 12+ messages in thread
From: Ilias Apalodimas @ 2021-09-03 6:20 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Heinrich Schuchardt, Alexander Graf, Simon Glass,
AKASHI Takahiro, U-Boot Mailing List
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
2021-09-03 1:55 ` [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check Masahisa Kojima
2021-09-03 6:20 ` Ilias Apalodimas
@ 2021-09-03 6:22 ` Heinrich Schuchardt
2021-09-03 6:51 ` Ilias Apalodimas
1 sibling, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2021-09-03 6:22 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Alexander Graf, Ilias Apalodimas, Simon Glass, u-boot, AKASHI Takahiro
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;
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api
2021-09-03 1:55 ` [PATCH 1/3] efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api Masahisa Kojima
@ 2021-09-03 6:25 ` Ilias Apalodimas
0 siblings, 0 replies; 12+ messages in thread
From: Ilias Apalodimas @ 2021-09-03 6:25 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Heinrich Schuchardt, Alexander Graf, Simon Glass,
AKASHI Takahiro, U-Boot Mailing List
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
2021-09-03 6:22 ` Heinrich Schuchardt
@ 2021-09-03 6:51 ` Ilias Apalodimas
2021-09-03 7:11 ` Heinrich Schuchardt
0 siblings, 1 reply; 12+ messages in thread
From: Ilias Apalodimas @ 2021-09-03 6:51 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Masahisa Kojima, Alexander Graf, Simon Glass, u-boot, AKASHI Takahiro
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_v0p9_r10_12june2021.pdf
>
> >
> > 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_v0p9_r10_12june2021.pdf
Regards
/Ilias
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation
2021-09-03 1:55 ` [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation Masahisa Kojima
2021-09-03 6:19 ` Ilias Apalodimas
@ 2021-09-03 7:01 ` Heinrich Schuchardt
2021-09-03 7:24 ` Masahisa Kojima
1 sibling, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2021-09-03 7:01 UTC (permalink / raw)
To: Masahisa Kojima
Cc: AKASHI Takahiro, Alexander Graf, Ilias Apalodimas, Simon Glass, u-boot
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"
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
2021-09-03 6:51 ` Ilias Apalodimas
@ 2021-09-03 7:11 ` Heinrich Schuchardt
0 siblings, 0 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2021-09-03 7:11 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Masahisa Kojima, Alexander Graf, Simon Glass, u-boot, AKASHI Takahiro
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_v0p9_r10_12june2021.pdf
>
>>
>>>
>>> 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_v0p9_r10_12june2021.pdf
>
> Regards
> /Ilias
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation
2021-09-03 7:01 ` Heinrich Schuchardt
@ 2021-09-03 7:24 ` Masahisa Kojima
0 siblings, 0 replies; 12+ messages in thread
From: Masahisa Kojima @ 2021-09-03 7:24 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: AKASHI Takahiro, Alexander Graf, Ilias Apalodimas, Simon Glass,
U-Boot Mailing List
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"
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-09-03 7:24 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-03 1:55 [PATCH 0/3] Miscellaneous fixes of efi_tcg2 Masahisa Kojima
2021-09-03 1:55 ` [PATCH 1/3] efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api Masahisa Kojima
2021-09-03 6:25 ` Ilias Apalodimas
2021-09-03 1:55 ` [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation Masahisa Kojima
2021-09-03 6:19 ` Ilias Apalodimas
2021-09-03 7:01 ` Heinrich Schuchardt
2021-09-03 7:24 ` Masahisa Kojima
2021-09-03 1:55 ` [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check Masahisa Kojima
2021-09-03 6:20 ` Ilias Apalodimas
2021-09-03 6:22 ` Heinrich Schuchardt
2021-09-03 6:51 ` Ilias Apalodimas
2021-09-03 7:11 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.