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

[Heinrich Schuchardt]

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 at 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},
>