[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 04:43:56 CEST 2021
On Wed, 28 Apr 2021 at 11:35, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am 28. April 2021 03:06:15 MESZ schrieb Masahisa Kojima <masahisa.kojima at linaro.org>:
> >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.
> >
>
> The line 'default n' is superfluous as this is default behavior.
>
> If it does not make sense to select this option manually, you would remove help and short text to hide the symbol.
Hi Heinrich,
You are correct, I will update and send v3.
Thanks,
Masahisa
>
> Best regards
>
> Heinrich
>
> >>
> >> > + 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