[PATCH 0/3] Miscellaneous fixes of efi_tcg2

[PATCH 0/3] Miscellaneous fixes of efi_tcg2

[Masahisa Kojima]

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

[PATCH 1/3] efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api

[Masahisa Kojima]

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

[PATCH 2/3] efi_loader: fix boot_service_capability_min calculation

[Masahisa Kojima]

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

[PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check

[Masahisa Kojima]

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

Re: [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation

[Ilias Apalodimas]

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
>

Re: [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check

[Ilias Apalodimas]

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
>

Re: [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check

[Heinrich Schuchardt]

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

Re: [PATCH 1/3] efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api

[Ilias Apalodimas]

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
>

Re: [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check

[Ilias Apalodimas]

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

Re: [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation

[Heinrich Schuchardt]

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"
>

Re: [PATCH 3/3] efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check

[Heinrich Schuchardt]

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
>

Re: [PATCH 2/3] efi_loader: fix boot_service_capability_min calculation

[Masahisa Kojima]

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"
> >