[PATCH v2 1/2] efi_loader: expose efi_image_parse() even if UEFI Secure Boot is disabled
Masahisa Kojima
masahisa.kojima at linaro.org
Wed Apr 28 03:06:15 CEST 2021
On Tue, 27 Apr 2021 at 22:52, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 27.04.21 15:08, 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 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 | 9 ++++
> > 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, 79 insertions(+), 75 deletions(-)
> >
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 0b99d7c774..f012eb7718 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
>
> Select means that you cannot switch it off. Is this really what you
> want? If you want the user to decide it is enabled, just make
> EFI_SIGNATURE_SUPPORT default y.
EFI_SIGNATURE_SUPPORT is mandatory only when EFI_SECURE_BOOT or
EFI_CAPSULE_AUTHENTICATE is enabled, so I use "select" here.
>
> > 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
>
> see above
>
> > default n
> > help
> > Select this option to enable EFI secure boot support.
> > @@ -343,6 +345,13 @@ 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 "Enable signature verification support"
> > + depends on EFI_SECURE_BOOT || EFI_CAPSULE_AUTHENTICATE
> > + default n
>
> see above
EFI_SIGNATURE_SUPPORT is only required in the case that
EFI_SECURE_BOOT or EFI_CAPSULE_AUTHENTICATE is enabled.
>
> > + help
> > + Select this option to enable signature verification support.
> > +
> > 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)
>
> Why are you moving this function to a different C module?
The major goal of this patch is exposing efi_image_loader.c::efi_image_parse()
for PE/COFF image measurement.
Measured Boot itself does not require EFI_SIGNATURE_SUPPORT,
efi_signature.c will not be compiled. That is why I would like to move
efi_image_region_add() into efi_image_loader.c, efi_image_parse() calles
efi_image_region_add().
Thank you for your review.
Best regards,
Masahisa
>
> > +{
> > + 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 = ®s->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(®s->reg[j + 1], ®s->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 = ®s->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 = ®s->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(®s->reg[j + 1], ®s->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 = ®s->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;
> > +
>
> Yes, lib/efi_loader/efi_var_common.c is the best place for it.
>
> Best regards
>
> Heinrich
>
> > 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},
> >
>
More information about the U-Boot
mailing list