[v3,1/2] efi_loader: expose efi_image_parse() even if UEFI Secure Boot is disabled

Message ID 20210428121945.12586-2-masahisa.kojima@linaro.org
State Superseded
Headers show
Series
  • PE/COFF measurement support
Related show

Commit Message

Masahisa Kojima April 28, 2021, 12:19 p.m.
This is preparation for PE/COFF measurement support.
PE/COFF image hash calculation is same in both
UEFI Secure Boot image verification and measurement in
measured boot. PE/COFF image parsing functions are
gathered into efi_image_loader.c, and exposed even if
UEFI Secure Boot is not enabled.

This commit also adds the EFI_SIGNATURE_SUPPORT option
to decide if efi_signature.c shall be compiled.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>

---

Changes in v3:
- hide EFI_SIGNATURE_SUPPORT option

Changes in v2:
- Remove all #ifdef from efi_image_loader.c and efi_signature.c
- Add EFI_SIGNATURE_SUPPORT option
- Explicitly include <u-boot/hash-checksum.h>
- Gather PE/COFF parsing functions into efi_image_loader.c
- Move efi_guid_t efi_guid_image_security_database in efi_var_common.c


 lib/efi_loader/Kconfig            |  6 +++
 lib/efi_loader/Makefile           |  2 +-
 lib/efi_loader/efi_image_loader.c | 73 +++++++++++++++++++++++++++----
 lib/efi_loader/efi_signature.c    | 67 +---------------------------
 lib/efi_loader/efi_var_common.c   |  3 ++
 5 files changed, 76 insertions(+), 75 deletions(-)

-- 
2.17.1

Comments

Heinrich Schuchardt April 28, 2021, 1:16 p.m. | #1
On 28.04.21 14:19, Masahisa Kojima wrote:
> This is preparation for PE/COFF measurement support.

> PE/COFF image hash calculation is same in both

> UEFI Secure Boot image verification and measurement in

> measured boot. PE/COFF image parsing functions are

> gathered into efi_image_loader.c, and exposed even if

> UEFI Secure Boot is not enabled.

>

> This commit also adds the EFI_SIGNATURE_SUPPORT option

> to decide if efi_signature.c shall be compiled.

>

> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>

> ---

>

> Changes in v3:

> - hide EFI_SIGNATURE_SUPPORT option

>

> Changes in v2:

> - Remove all #ifdef from efi_image_loader.c and efi_signature.c

> - Add EFI_SIGNATURE_SUPPORT option

> - Explicitly include <u-boot/hash-checksum.h>

> - Gather PE/COFF parsing functions into efi_image_loader.c

> - Move efi_guid_t efi_guid_image_security_database in efi_var_common.c

>

>

>  lib/efi_loader/Kconfig            |  6 +++

>  lib/efi_loader/Makefile           |  2 +-

>  lib/efi_loader/efi_image_loader.c | 73 +++++++++++++++++++++++++++----

>  lib/efi_loader/efi_signature.c    | 67 +---------------------------

>  lib/efi_loader/efi_var_common.c   |  3 ++

>  5 files changed, 76 insertions(+), 75 deletions(-)

>

> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig

> index 0b99d7c774..b76e77180e 100644

> --- a/lib/efi_loader/Kconfig

> +++ b/lib/efi_loader/Kconfig

> @@ -174,6 +174,7 @@ config EFI_CAPSULE_AUTHENTICATE

>  	select PKCS7_MESSAGE_PARSER

>  	select PKCS7_VERIFY

>  	select IMAGE_SIGN_INFO

> +	select EFI_SIGNATURE_SUPPORT

>  	default n

>  	help

>  	  Select this option if you want to enable capsule

> @@ -336,6 +337,7 @@ config EFI_SECURE_BOOT

>  	select X509_CERTIFICATE_PARSER

>  	select PKCS7_MESSAGE_PARSER

>  	select PKCS7_VERIFY

> +	select EFI_SIGNATURE_SUPPORT

>  	default n

>  	help

>  	  Select this option to enable EFI secure boot support.

> @@ -343,6 +345,10 @@ config EFI_SECURE_BOOT

>  	  it is signed with a trusted key. To do that, you need to install,

>  	  at least, PK, KEK and db.

>

> +config EFI_SIGNATURE_SUPPORT

> +	bool

> +	depends on EFI_SECURE_BOOT || EFI_CAPSULE_AUTHENTICATE

> +

>  config EFI_ESRT

>  	bool "Enable the UEFI ESRT generation"

>  	depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT

> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile

> index 8bd343e258..fd344cea29 100644

> --- a/lib/efi_loader/Makefile

> +++ b/lib/efi_loader/Makefile

> @@ -63,7 +63,7 @@ obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o

>  obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o

>  obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o

>  obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o

> -obj-y += efi_signature.o

> +obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o

>

>  EFI_VAR_SEED_FILE := $(subst $\",,$(CONFIG_EFI_VAR_SEED_FILE))

>  $(obj)/efi_var_seed.o: $(srctree)/$(EFI_VAR_SEED_FILE)

> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c

> index f53ef367ec..b8a790bcb9 100644

> --- a/lib/efi_loader/efi_image_loader.c

> +++ b/lib/efi_loader/efi_image_loader.c

> @@ -213,7 +213,68 @@ static void efi_set_code_and_data_type(

>  	}

>  }

>

> -#ifdef CONFIG_EFI_SECURE_BOOT

> +/**

> + * efi_image_region_add() - add an entry of region

> + * @regs:	Pointer to array of regions

> + * @start:	Start address of region (included)

> + * @end:	End address of region (excluded)

> + * @nocheck:	flag against overlapped regions

> + *

> + * Take one entry of region [@start, @end[ and insert it into the list.

> + *

> + * * If @nocheck is false, the list will be sorted ascending by address.

> + *   Overlapping entries will not be allowed.

> + *

> + * * If @nocheck is true, the list will be sorted ascending by sequence

> + *   of adding the entries. Overlapping is allowed.

> + *

> + * Return:	status code

> + */

> +efi_status_t efi_image_region_add(struct efi_image_regions *regs,

> +				  const void *start, const void *end,

> +				  int nocheck)

> +{

> +	struct image_region *reg;

> +	int i, j;

> +

> +	if (regs->num >= regs->max) {

> +		EFI_PRINT("%s: no more room for regions\n", __func__);

> +		return EFI_OUT_OF_RESOURCES;

> +	}

> +

> +	if (end < start)

> +		return EFI_INVALID_PARAMETER;

> +

> +	for (i = 0; i < regs->num; i++) {

> +		reg = &regs->reg[i];

> +		if (nocheck)

> +			continue;

> +

> +		/* new data after registered region */

> +		if (start >= reg->data + reg->size)

> +			continue;

> +

> +		/* new data preceding registered region */

> +		if (end <= reg->data) {

> +			for (j = regs->num - 1; j >= i; j--)

> +				memcpy(&regs->reg[j + 1], &regs->reg[j],

> +				       sizeof(*reg));

> +			break;

> +		}

> +

> +		/* new data overlapping registered region */

> +		EFI_PRINT("%s: new region already part of another\n", __func__);

> +		return EFI_INVALID_PARAMETER;

> +	}

> +

> +	reg = &regs->reg[i];

> +	reg->data = start;

> +	reg->size = end - start;

> +	regs->num++;

> +

> +	return EFI_SUCCESS;

> +}

> +

>  /**

>   * cmp_pe_section() - compare virtual addresses of two PE image sections

>   * @arg1:	pointer to pointer to first section header

> @@ -504,6 +565,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)

>

>  	EFI_PRINT("%s: Enter, %d\n", __func__, ret);

>

> +	if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> +		return true;

> +


Why is this needed? Doesn't efi_secure_boot_enabled() return false in
this case?

Best regards

Heinrich

>  	if (!efi_secure_boot_enabled())

>  		return true;

>

> @@ -668,13 +732,6 @@ err:

>  	EFI_PRINT("%s: Exit, %d\n", __func__, ret);

>  	return ret;

>  }

> -#else

> -static bool efi_image_authenticate(void *efi, size_t efi_size)

> -{

> -	return true;

> -}

> -#endif /* CONFIG_EFI_SECURE_BOOT */

> -

>

>  /**

>   * efi_check_pe() - check if a memory buffer contains a PE-COFF image

> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c

> index c7ec275414..bdd09881fc 100644

> --- a/lib/efi_loader/efi_signature.c

> +++ b/lib/efi_loader/efi_signature.c

> @@ -15,18 +15,16 @@

>  #include <crypto/public_key.h>

>  #include <linux/compat.h>

>  #include <linux/oid_registry.h>

> +#include <u-boot/hash-checksum.h>

>  #include <u-boot/rsa.h>

>  #include <u-boot/sha256.h>

>

> -const efi_guid_t efi_guid_image_security_database =

> -		EFI_IMAGE_SECURITY_DATABASE_GUID;

>  const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID;

>  const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID;

>  const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID;

>  const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;

>  const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;

>

> -#if defined(CONFIG_EFI_SECURE_BOOT) || defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)

>  static u8 pkcs7_hdr[] = {

>  	/* SEQUENCE */

>  	0x30, 0x82, 0x05, 0xc7,

> @@ -539,68 +537,6 @@ out:

>  	return !revoked;

>  }

>

> -/**

> - * efi_image_region_add() - add an entry of region

> - * @regs:	Pointer to array of regions

> - * @start:	Start address of region (included)

> - * @end:	End address of region (excluded)

> - * @nocheck:	flag against overlapped regions

> - *

> - * Take one entry of region [@start, @end[ and insert it into the list.

> - *

> - * * If @nocheck is false, the list will be sorted ascending by address.

> - *   Overlapping entries will not be allowed.

> - *

> - * * If @nocheck is true, the list will be sorted ascending by sequence

> - *   of adding the entries. Overlapping is allowed.

> - *

> - * Return:	status code

> - */

> -efi_status_t efi_image_region_add(struct efi_image_regions *regs,

> -				  const void *start, const void *end,

> -				  int nocheck)

> -{

> -	struct image_region *reg;

> -	int i, j;

> -

> -	if (regs->num >= regs->max) {

> -		EFI_PRINT("%s: no more room for regions\n", __func__);

> -		return EFI_OUT_OF_RESOURCES;

> -	}

> -

> -	if (end < start)

> -		return EFI_INVALID_PARAMETER;

> -

> -	for (i = 0; i < regs->num; i++) {

> -		reg = &regs->reg[i];

> -		if (nocheck)

> -			continue;

> -

> -		/* new data after registered region */

> -		if (start >= reg->data + reg->size)

> -			continue;

> -

> -		/* new data preceding registered region */

> -		if (end <= reg->data) {

> -			for (j = regs->num - 1; j >= i; j--)

> -				memcpy(&regs->reg[j + 1], &regs->reg[j],

> -				       sizeof(*reg));

> -			break;

> -		}

> -

> -		/* new data overlapping registered region */

> -		EFI_PRINT("%s: new region already part of another\n", __func__);

> -		return EFI_INVALID_PARAMETER;

> -	}

> -

> -	reg = &regs->reg[i];

> -	reg->data = start;

> -	reg->size = end - start;

> -	regs->num++;

> -

> -	return EFI_SUCCESS;

> -}

> -

>  /**

>   * efi_sigstore_free - free signature store

>   * @sigstore:	Pointer to signature store structure

> @@ -846,4 +782,3 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)

>

>  	return efi_build_signature_store(db, db_size);

>  }

> -#endif /* CONFIG_EFI_SECURE_BOOT || CONFIG_EFI_CAPSULE_AUTHENTICATE */

> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c

> index b11ed91a74..83479dd142 100644

> --- a/lib/efi_loader/efi_var_common.c

> +++ b/lib/efi_loader/efi_var_common.c

> @@ -24,6 +24,9 @@ struct efi_auth_var_name_type {

>  	const enum efi_auth_var_type type;

>  };

>

> +const efi_guid_t efi_guid_image_security_database =

> +		EFI_IMAGE_SECURITY_DATABASE_GUID;

> +

>  static const struct efi_auth_var_name_type name_type[] = {

>  	{u"PK", &efi_global_variable_guid, EFI_AUTH_VAR_PK},

>  	{u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK},

>
Heinrich Schuchardt May 8, 2021, 2:08 p.m. | #2
On 4/28/21 3:16 PM, Heinrich Schuchardt wrote:
> On 28.04.21 14:19, Masahisa Kojima wrote:

<snip />
>>   /**

>>    * cmp_pe_section() - compare virtual addresses of two PE image sections

>>    * @arg1:	pointer to pointer to first section header

>> @@ -504,6 +565,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)

>>

>>   	EFI_PRINT("%s: Enter, %d\n", __func__, ret);

>>

>> +	if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

>> +		return true;

>> +

>

> Why is this needed? Doesn't efi_secure_boot_enabled() return false in

> this case?


Hello Masahisa,

I did not see any reply yet. Was a mail lost?

Best regards

Heinrich
Masahisa Kojima May 10, 2021, 12:49 a.m. | #3
Hi Heinrich,

Sorry for the late reply.

On Sat, 8 May 2021 at 23:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>

> On 4/28/21 3:16 PM, Heinrich Schuchardt wrote:

> > On 28.04.21 14:19, Masahisa Kojima wrote:

> <snip />

> >>   /**

> >>    * cmp_pe_section() - compare virtual addresses of two PE image sections

> >>    * @arg1:  pointer to pointer to first section header

> >> @@ -504,6 +565,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)

> >>

> >>      EFI_PRINT("%s: Enter, %d\n", __func__, ret);

> >>

> >> +    if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> >> +            return true;

> >> +

> >

> > Why is this needed? Doesn't efi_secure_boot_enabled() return false in

> > this case?


The original code is as follows.

#ifdef CONFIG_EFI_SECURE_BOOT
static bool efi_image_authenticate(void *efi, size_t efi_size) {

  < snip >

 }
#else
static bool efi_image_authenticate(void *efi, size_t efi_size)
{
       return true;
}
#endif /* CONFIG_EFI_SECURE_BOOT */

The purpose of this commit is removing #if compilation switch,
so I keep the original implementation, always return true
if CONFIG_EFI_SECURE_BOOT is disabled.

Thanks,
Masahisa

>

> Hello Masahisa,

>

> I did not see any reply yet. Was a mail lost?

>

> Best regards

>

> Heinrich
AKASHI Takahiro May 10, 2021, 2:07 a.m. | #4
On Mon, May 10, 2021 at 09:49:03AM +0900, Masahisa Kojima wrote:
> Hi Heinrich,

> 

> Sorry for the late reply.

> 

> On Sat, 8 May 2021 at 23:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> >

> > On 4/28/21 3:16 PM, Heinrich Schuchardt wrote:

> > > On 28.04.21 14:19, Masahisa Kojima wrote:

> > <snip />

> > >>   /**

> > >>    * cmp_pe_section() - compare virtual addresses of two PE image sections

> > >>    * @arg1:  pointer to pointer to first section header

> > >> @@ -504,6 +565,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)

> > >>

> > >>      EFI_PRINT("%s: Enter, %d\n", __func__, ret);

> > >>

> > >> +    if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> > >> +            return true;

> > >> +

> > >

> > > Why is this needed? Doesn't efi_secure_boot_enabled() return false in

> > > this case?

> 

> The original code is as follows.


Heinrich's concern was, I guess, that

> > >> +    if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> > >> +            return true;


and the succeeding check,

        if (!efi_secure_boot_enabled())
                        return true;

are somehow redundant.
But in the latter case, I'm afraid that a compiler cannot optimize out
the rest of the logic in efi_image_authenticate().

-Takahiro Akashi


> #ifdef CONFIG_EFI_SECURE_BOOT

> static bool efi_image_authenticate(void *efi, size_t efi_size) {

> 

>   < snip >

> 

>  }

> #else

> static bool efi_image_authenticate(void *efi, size_t efi_size)

> {

>        return true;

> }

> #endif /* CONFIG_EFI_SECURE_BOOT */

> 

> The purpose of this commit is removing #if compilation switch,

> so I keep the original implementation, always return true

> if CONFIG_EFI_SECURE_BOOT is disabled.

> 

> Thanks,

> Masahisa

> 

> >

> > Hello Masahisa,

> >

> > I did not see any reply yet. Was a mail lost?

> >

> > Best regards

> >

> > Heinrich
Masahisa Kojima May 10, 2021, 10:06 p.m. | #5
On Mon, 10 May 2021 at 11:07, Takahiro Akashi
<takahiro.akashi@linaro.org> wrote:
>

> On Mon, May 10, 2021 at 09:49:03AM +0900, Masahisa Kojima wrote:

> > Hi Heinrich,

> >

> > Sorry for the late reply.

> >

> > On Sat, 8 May 2021 at 23:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> > >

> > > On 4/28/21 3:16 PM, Heinrich Schuchardt wrote:

> > > > On 28.04.21 14:19, Masahisa Kojima wrote:

> > > <snip />

> > > >>   /**

> > > >>    * cmp_pe_section() - compare virtual addresses of two PE image sections

> > > >>    * @arg1:  pointer to pointer to first section header

> > > >> @@ -504,6 +565,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)

> > > >>

> > > >>      EFI_PRINT("%s: Enter, %d\n", __func__, ret);

> > > >>

> > > >> +    if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> > > >> +            return true;

> > > >> +

> > > >

> > > > Why is this needed? Doesn't efi_secure_boot_enabled() return false in

> > > > this case?

> >

> > The original code is as follows.

>

> Heinrich's concern was, I guess, that

>

> > > >> +    if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> > > >> +            return true;

>

> and the succeeding check,

>

>         if (!efi_secure_boot_enabled())

>                         return true;

>

> are somehow redundant.

> But in the latter case, I'm afraid that a compiler cannot optimize out

> the rest of the logic in efi_image_authenticate().


Hi Heinrich, Takahiro,

Sorry for the late reply.
I now understand Takahiro's concern.
If I remove following check,

> +    if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> +            return true;


compiler optimization does not work and link error occurs.

lib/built-in.o: In function `efi_image_authenticate':
/home/ubuntu/SynQuacer/ledge/u-boot/lib/efi_loader/efi_image_loader.c:601:
undefined reference to `efi_sigstore_parse_sigdb'
/home/ubuntu/SynQuacer/ledge/u-boot/lib/efi_loader/efi_image_loader.c:607:
undefined reference to `efi_sigstore_parse_sigdb'
/home/ubuntu/SynQuacer/ledge/u-boot/lib/efi_loader/efi_image_loader.c:613:
undefined reference to `efi_signature_lookup_digest'

I would like to propose two resolution.

1) keep if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition
2) remove if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition and
    always include efi_signature.c as compilation target.

Please advise.

Thanks,
Masahisa


>

> -Takahiro Akashi

>

>

> > #ifdef CONFIG_EFI_SECURE_BOOT

> > static bool efi_image_authenticate(void *efi, size_t efi_size) {

> >

> >   < snip >

> >

> >  }

> > #else

> > static bool efi_image_authenticate(void *efi, size_t efi_size)

> > {

> >        return true;

> > }

> > #endif /* CONFIG_EFI_SECURE_BOOT */

> >

> > The purpose of this commit is removing #if compilation switch,

> > so I keep the original implementation, always return true

> > if CONFIG_EFI_SECURE_BOOT is disabled.

> >

> > Thanks,

> > Masahisa

> >

> > >

> > > Hello Masahisa,

> > >

> > > I did not see any reply yet. Was a mail lost?

> > >

> > > Best regards

> > >

> > > Heinrich
Masahisa Kojima May 12, 2021, 6:57 a.m. | #6
Hi Heinrich,

I'm about to send v4 patch series.

> 1) keep if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition


I chose this option, but I reverted #ifdef statement instead of using
"if (IS_ENABLED)" because I think it is better not to rely on compiler
optimization.

> 2) remove if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition and

>     always include efi_signature.c as compilation target.


In this option, CONFIG_PKCS7_VERIFY is required for EFI_TCG2_PROTOCOL
just for successful build.
To minimize dependency, I did not proceed with 2).

Please kindly review v4.

Thanks,
Masahisa

On Tue, 11 May 2021 at 07:06, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>

> On Mon, 10 May 2021 at 11:07, Takahiro Akashi

> <takahiro.akashi@linaro.org> wrote:

> >

> > On Mon, May 10, 2021 at 09:49:03AM +0900, Masahisa Kojima wrote:

> > > Hi Heinrich,

> > >

> > > Sorry for the late reply.

> > >

> > > On Sat, 8 May 2021 at 23:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> > > >

> > > > On 4/28/21 3:16 PM, Heinrich Schuchardt wrote:

> > > > > On 28.04.21 14:19, Masahisa Kojima wrote:

> > > > <snip />

> > > > >>   /**

> > > > >>    * cmp_pe_section() - compare virtual addresses of two PE image sections

> > > > >>    * @arg1:  pointer to pointer to first section header

> > > > >> @@ -504,6 +565,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)

> > > > >>

> > > > >>      EFI_PRINT("%s: Enter, %d\n", __func__, ret);

> > > > >>

> > > > >> +    if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> > > > >> +            return true;

> > > > >> +

> > > > >

> > > > > Why is this needed? Doesn't efi_secure_boot_enabled() return false in

> > > > > this case?

> > >

> > > The original code is as follows.

> >

> > Heinrich's concern was, I guess, that

> >

> > > > >> +    if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> > > > >> +            return true;

> >

> > and the succeeding check,

> >

> >         if (!efi_secure_boot_enabled())

> >                         return true;

> >

> > are somehow redundant.

> > But in the latter case, I'm afraid that a compiler cannot optimize out

> > the rest of the logic in efi_image_authenticate().

>

> Hi Heinrich, Takahiro,

>

> Sorry for the late reply.

> I now understand Takahiro's concern.

> If I remove following check,

>

> > +    if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))

> > +            return true;

>

> compiler optimization does not work and link error occurs.

>

> lib/built-in.o: In function `efi_image_authenticate':

> /home/ubuntu/SynQuacer/ledge/u-boot/lib/efi_loader/efi_image_loader.c:601:

> undefined reference to `efi_sigstore_parse_sigdb'

> /home/ubuntu/SynQuacer/ledge/u-boot/lib/efi_loader/efi_image_loader.c:607:

> undefined reference to `efi_sigstore_parse_sigdb'

> /home/ubuntu/SynQuacer/ledge/u-boot/lib/efi_loader/efi_image_loader.c:613:

> undefined reference to `efi_signature_lookup_digest'

>

> I would like to propose two resolution.

>

> 1) keep if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition

> 2) remove if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition and

>     always include efi_signature.c as compilation target.

>

> Please advise.

>

> Thanks,

> Masahisa

>

>

> >

> > -Takahiro Akashi

> >

> >

> > > #ifdef CONFIG_EFI_SECURE_BOOT

> > > static bool efi_image_authenticate(void *efi, size_t efi_size) {

> > >

> > >   < snip >

> > >

> > >  }

> > > #else

> > > static bool efi_image_authenticate(void *efi, size_t efi_size)

> > > {

> > >        return true;

> > > }

> > > #endif /* CONFIG_EFI_SECURE_BOOT */

> > >

> > > The purpose of this commit is removing #if compilation switch,

> > > so I keep the original implementation, always return true

> > > if CONFIG_EFI_SECURE_BOOT is disabled.

> > >

> > > Thanks,

> > > Masahisa

> > >

> > > >

> > > > Hello Masahisa,

> > > >

> > > > I did not see any reply yet. Was a mail lost?

> > > >

> > > > Best regards

> > > >

> > > > Heinrich

Patch

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 0b99d7c774..b76e77180e 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -174,6 +174,7 @@  config EFI_CAPSULE_AUTHENTICATE
 	select PKCS7_MESSAGE_PARSER
 	select PKCS7_VERIFY
 	select IMAGE_SIGN_INFO
+	select EFI_SIGNATURE_SUPPORT
 	default n
 	help
 	  Select this option if you want to enable capsule
@@ -336,6 +337,7 @@  config EFI_SECURE_BOOT
 	select X509_CERTIFICATE_PARSER
 	select PKCS7_MESSAGE_PARSER
 	select PKCS7_VERIFY
+	select EFI_SIGNATURE_SUPPORT
 	default n
 	help
 	  Select this option to enable EFI secure boot support.
@@ -343,6 +345,10 @@  config EFI_SECURE_BOOT
 	  it is signed with a trusted key. To do that, you need to install,
 	  at least, PK, KEK and db.
 
+config EFI_SIGNATURE_SUPPORT
+	bool
+	depends on EFI_SECURE_BOOT || EFI_CAPSULE_AUTHENTICATE
+
 config EFI_ESRT
 	bool "Enable the UEFI ESRT generation"
 	depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 8bd343e258..fd344cea29 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -63,7 +63,7 @@  obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
 obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
 obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
 obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
-obj-y += efi_signature.o
+obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o
 
 EFI_VAR_SEED_FILE := $(subst $\",,$(CONFIG_EFI_VAR_SEED_FILE))
 $(obj)/efi_var_seed.o: $(srctree)/$(EFI_VAR_SEED_FILE)
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index f53ef367ec..b8a790bcb9 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -213,7 +213,68 @@  static void efi_set_code_and_data_type(
 	}
 }
 
-#ifdef CONFIG_EFI_SECURE_BOOT
+/**
+ * efi_image_region_add() - add an entry of region
+ * @regs:	Pointer to array of regions
+ * @start:	Start address of region (included)
+ * @end:	End address of region (excluded)
+ * @nocheck:	flag against overlapped regions
+ *
+ * Take one entry of region [@start, @end[ and insert it into the list.
+ *
+ * * If @nocheck is false, the list will be sorted ascending by address.
+ *   Overlapping entries will not be allowed.
+ *
+ * * If @nocheck is true, the list will be sorted ascending by sequence
+ *   of adding the entries. Overlapping is allowed.
+ *
+ * Return:	status code
+ */
+efi_status_t efi_image_region_add(struct efi_image_regions *regs,
+				  const void *start, const void *end,
+				  int nocheck)
+{
+	struct image_region *reg;
+	int i, j;
+
+	if (regs->num >= regs->max) {
+		EFI_PRINT("%s: no more room for regions\n", __func__);
+		return EFI_OUT_OF_RESOURCES;
+	}
+
+	if (end < start)
+		return EFI_INVALID_PARAMETER;
+
+	for (i = 0; i < regs->num; i++) {
+		reg = &regs->reg[i];
+		if (nocheck)
+			continue;
+
+		/* new data after registered region */
+		if (start >= reg->data + reg->size)
+			continue;
+
+		/* new data preceding registered region */
+		if (end <= reg->data) {
+			for (j = regs->num - 1; j >= i; j--)
+				memcpy(&regs->reg[j + 1], &regs->reg[j],
+				       sizeof(*reg));
+			break;
+		}
+
+		/* new data overlapping registered region */
+		EFI_PRINT("%s: new region already part of another\n", __func__);
+		return EFI_INVALID_PARAMETER;
+	}
+
+	reg = &regs->reg[i];
+	reg->data = start;
+	reg->size = end - start;
+	regs->num++;
+
+	return EFI_SUCCESS;
+}
+
 /**
  * cmp_pe_section() - compare virtual addresses of two PE image sections
  * @arg1:	pointer to pointer to first section header
@@ -504,6 +565,9 @@  static bool efi_image_authenticate(void *efi, size_t efi_size)
 
 	EFI_PRINT("%s: Enter, %d\n", __func__, ret);
 
+	if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT))
+		return true;
+
 	if (!efi_secure_boot_enabled())
 		return true;
 
@@ -668,13 +732,6 @@  err:
 	EFI_PRINT("%s: Exit, %d\n", __func__, ret);
 	return ret;
 }
-#else
-static bool efi_image_authenticate(void *efi, size_t efi_size)
-{
-	return true;
-}
-#endif /* CONFIG_EFI_SECURE_BOOT */
-
 
 /**
  * efi_check_pe() - check if a memory buffer contains a PE-COFF image
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index c7ec275414..bdd09881fc 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -15,18 +15,16 @@ 
 #include <crypto/public_key.h>
 #include <linux/compat.h>
 #include <linux/oid_registry.h>
+#include <u-boot/hash-checksum.h>
 #include <u-boot/rsa.h>
 #include <u-boot/sha256.h>
 
-const efi_guid_t efi_guid_image_security_database =
-		EFI_IMAGE_SECURITY_DATABASE_GUID;
 const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID;
 const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID;
 const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID;
 const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
 const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
 
-#if defined(CONFIG_EFI_SECURE_BOOT) || defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
 static u8 pkcs7_hdr[] = {
 	/* SEQUENCE */
 	0x30, 0x82, 0x05, 0xc7,
@@ -539,68 +537,6 @@  out:
 	return !revoked;
 }
 
-/**
- * efi_image_region_add() - add an entry of region
- * @regs:	Pointer to array of regions
- * @start:	Start address of region (included)
- * @end:	End address of region (excluded)
- * @nocheck:	flag against overlapped regions
- *
- * Take one entry of region [@start, @end[ and insert it into the list.
- *
- * * If @nocheck is false, the list will be sorted ascending by address.
- *   Overlapping entries will not be allowed.
- *
- * * If @nocheck is true, the list will be sorted ascending by sequence
- *   of adding the entries. Overlapping is allowed.
- *
- * Return:	status code
- */
-efi_status_t efi_image_region_add(struct efi_image_regions *regs,
-				  const void *start, const void *end,
-				  int nocheck)
-{
-	struct image_region *reg;
-	int i, j;
-
-	if (regs->num >= regs->max) {
-		EFI_PRINT("%s: no more room for regions\n", __func__);
-		return EFI_OUT_OF_RESOURCES;
-	}
-
-	if (end < start)
-		return EFI_INVALID_PARAMETER;
-
-	for (i = 0; i < regs->num; i++) {
-		reg = &regs->reg[i];
-		if (nocheck)
-			continue;
-
-		/* new data after registered region */
-		if (start >= reg->data + reg->size)
-			continue;
-
-		/* new data preceding registered region */
-		if (end <= reg->data) {
-			for (j = regs->num - 1; j >= i; j--)
-				memcpy(&regs->reg[j + 1], &regs->reg[j],
-				       sizeof(*reg));
-			break;
-		}
-
-		/* new data overlapping registered region */
-		EFI_PRINT("%s: new region already part of another\n", __func__);
-		return EFI_INVALID_PARAMETER;
-	}
-
-	reg = &regs->reg[i];
-	reg->data = start;
-	reg->size = end - start;
-	regs->num++;
-
-	return EFI_SUCCESS;
-}
-
 /**
  * efi_sigstore_free - free signature store
  * @sigstore:	Pointer to signature store structure
@@ -846,4 +782,3 @@  struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
 
 	return efi_build_signature_store(db, db_size);
 }
-#endif /* CONFIG_EFI_SECURE_BOOT || CONFIG_EFI_CAPSULE_AUTHENTICATE */
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index b11ed91a74..83479dd142 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -24,6 +24,9 @@  struct efi_auth_var_name_type {
 	const enum efi_auth_var_type type;
 };
 
+const efi_guid_t efi_guid_image_security_database =
+		EFI_IMAGE_SECURITY_DATABASE_GUID;
+
 static const struct efi_auth_var_name_type name_type[] = {
 	{u"PK", &efi_global_variable_guid, EFI_AUTH_VAR_PK},
 	{u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK},