Hi Etienne,
On Tue, 25 Oct 2022 at 17:15, Etienne Carriere
<etienne.carri...@linaro.org> wrote:
>
> Hello Kojima-san,
>
> On Tue, 25 Oct 2022 at 05:17, Masahisa Kojima
> <masahisa.koj...@linaro.org> wrote:
> >
> > This commit adds the menu-driven UEFI Secure Boot Key
> > enrollment interface. User can enroll the PK, KEK, db
> > and dbx by selecting EFI Signature Lists file.
> > After the PK is enrolled, UEFI Secure Boot is enabled and
> > EFI Signature Lists file must be signed by KEK or PK.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org>
> > ---
> > No change since v4
> >
> > Changes in v4:
> > - add CONFIG_EFI_MM_COMM_TEE dependency
> > - fix error handling
> >
> > Changes in v3:
> > - fix error handling
> >
> > Changes in v2:
> > - allow to enroll .esl file
> > - fix typos
> > - add function comments
> >
> > cmd/Makefile | 5 +
> > cmd/eficonfig.c | 3 +
> > cmd/eficonfig_sbkey.c | 357 ++++++++++++++++++++++++++++++++++++++++++
> > include/efi_config.h | 5 +
> > 4 files changed, 370 insertions(+)
> > create mode 100644 cmd/eficonfig_sbkey.c
> >
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index c95e09d058..e43ef22e98 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -66,6 +66,11 @@ obj-$(CONFIG_CMD_EEPROM) += eeprom.o
> > obj-$(CONFIG_EFI) += efi.o
> > obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o
> > obj-$(CONFIG_CMD_EFICONFIG) += eficonfig.o
> > +ifdef CONFIG_CMD_EFICONFIG
> > +ifdef CONFIG_EFI_MM_COMM_TEE
> > +obj-$(CONFIG_EFI_SECURE_BOOT) += eficonfig_sbkey.o
> > +endif
> > +endif
> > obj-$(CONFIG_CMD_ELF) += elf.o
> > obj-$(CONFIG_CMD_EROFS) += erofs.o
> > obj-$(CONFIG_HUSH_PARSER) += exit.o
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index c765b795d0..0b643a046c 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -2447,6 +2447,9 @@ static const struct eficonfig_item
> > maintenance_menu_items[] = {
> > {"Edit Boot Option", eficonfig_process_edit_boot_option},
> > {"Change Boot Order", eficonfig_process_change_boot_order},
> > {"Delete Boot Option", eficonfig_process_delete_boot_option},
> > +#if (CONFIG_IS_ENABLED(EFI_SECURE_BOOT) &&
> > CONFIG_IS_ENABLED(EFI_MM_COMM_TEE))
> > + {"Secure Boot Configuration", eficonfig_process_secure_boot_config},
> > +#endif
> > {"Quit", eficonfig_process_quit},
> > };
> >
> > diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> > new file mode 100644
> > index 0000000000..32a39eb7ba
> > --- /dev/null
> > +++ b/cmd/eficonfig_sbkey.c
> > @@ -0,0 +1,357 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Menu-driven UEFI Secure Boot Key Maintenance
> > + *
> > + * Copyright (c) 2022 Masahisa Kojima, Linaro Limited
> > + */
> > +
> > +#include <ansi.h>
> > +#include <common.h>
> > +#include <charset.h>
> > +#include <hexdump.h>
> > +#include <log.h>
> > +#include <malloc.h>
> > +#include <menu.h>
> > +#include <efi_loader.h>
> > +#include <efi_config.h>
> > +#include <efi_variable.h>
> > +#include <crypto/pkcs7_parser.h>
> > +
> > +enum efi_sbkey_signature_type {
> > + SIG_TYPE_X509 = 0,
> > + SIG_TYPE_HASH,
> > + SIG_TYPE_CRL,
> > + SIG_TYPE_RSA2048,
> > +};
> > +
> > +struct eficonfig_sigtype_to_str {
> > + efi_guid_t sig_type;
> > + char *str;
> > + enum efi_sbkey_signature_type type;
> > +};
> > +
> > +static const struct eficonfig_sigtype_to_str sigtype_to_str[] = {
> > + {EFI_CERT_X509_GUID, "X509",
> > SIG_TYPE_X509},
> > + {EFI_CERT_SHA256_GUID, "SHA256",
> > SIG_TYPE_HASH},
> > + {EFI_CERT_X509_SHA256_GUID, "X509_SHA256 CRL",
> > SIG_TYPE_CRL},
> > + {EFI_CERT_X509_SHA384_GUID, "X509_SHA384 CRL",
> > SIG_TYPE_CRL},
> > + {EFI_CERT_X509_SHA512_GUID, "X509_SHA512 CRL",
> > SIG_TYPE_CRL},
> > + /* U-Boot does not support the following signature types */
> > +/* {EFI_CERT_RSA2048_GUID, "RSA2048",
> > SIG_TYPE_RSA2048}, */
> > +/* {EFI_CERT_RSA2048_SHA256_GUID, "RSA2048_SHA256",
> > SIG_TYPE_RSA2048}, */
> > +/* {EFI_CERT_SHA1_GUID, "SHA1",
> > SIG_TYPE_HASH}, */
> > +/* {EFI_CERT_RSA2048_SHA_GUID, "RSA2048_SHA",
> > SIG_TYPE_RSA2048 }, */
> > +/* {EFI_CERT_SHA224_GUID, "SHA224",
> > SIG_TYPE_HASH}, */
> > +/* {EFI_CERT_SHA384_GUID, "SHA384",
> > SIG_TYPE_HASH}, */
> > +/* {EFI_CERT_SHA512_GUID, "SHA512",
> > SIG_TYPE_HASH}, */
> > +};
> > +
> > +/**
> > + * is_secureboot_enabled() - check UEFI Secure Boot is enabled
> > + *
> > + * Return: true when UEFI Secure Boot is enabled, false otherwise
> > + */
> > +static bool is_secureboot_enabled(void)
> > +{
> > + efi_status_t ret;
> > + u8 secure_boot;
> > + efi_uintn_t size;
> > +
> > + size = sizeof(secure_boot);
> > + ret = efi_get_variable_int(u"SecureBoot", &efi_global_variable_guid,
> > + NULL, &size, &secure_boot, NULL);
> > +
> > + return secure_boot == 1;
>
> This should test 'ret{ against EFI_SUCCESS and EFI_NOT_FOUND.
> I think the logic should be:
> if (ret == EFI_SUCCESS) { return secure_boot == 1; }
> else if (ret == EFI_NOT_FOUND) { return false; }
> else { /* enforce secure bool policy on error */ return true; }
>
> That said, there is the exported efi_secure_boot_enabled() that should
> be reliable, i think.
You are correct, I overlooked this exported function.
>
>
> > +}
> > +
> > +/**
> > + * create_time_based_payload() - create payload for time based
> > authenticate variable
> > + *
> > + * @db: pointer to the original signature database
> > + * @new_db: pointer to the authenticated variable payload
> > + * @size: pointer to payload size
> > + * Return: status code
> > + */
> > +static efi_status_t create_time_based_payload(void *db, void **new_db,
> > efi_uintn_t *size)
> > +{
> > + efi_status_t ret;
> > + struct efi_time time;
> > + efi_uintn_t total_size;
> > + struct efi_variable_authentication_2 *auth;
> > +
> > + *new_db = NULL;
> > +
> > + /*
> > + * SetVariable() call with
> > EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS
> > + * attribute requires EFI_VARIABLE_AUTHENTICATED_2 descriptor,
> > prepare it
> > + * without certificate data in it.
> > + */
> > + total_size = sizeof(struct efi_variable_authentication_2) + *size;
> > +
> > + auth = calloc(1, total_size);
> > + if (!auth)
> > + return EFI_OUT_OF_RESOURCES;
> > +
> > + ret = EFI_CALL((*efi_runtime_services.get_time)(&time, NULL));
> > + if (ret != EFI_SUCCESS) {
> > + free(auth);
> > + return EFI_OUT_OF_RESOURCES;
> > + }
> > + time.pad1 = 0;
> > + time.nanosecond = 0;
> > + time.timezone = 0;
> > + time.daylight = 0;
> > + time.pad2 = 0;
> > + memcpy(&auth->time_stamp, &time, sizeof(time));
> > + auth->auth_info.hdr.dwLength = sizeof(struct
> > win_certificate_uefi_guid);
> > + auth->auth_info.hdr.wRevision = 0x0200;
>
> Can use WIN_CERT_REVISION_2_0 from pe.h.
Yes, I will fix it.
Thank you for your comment!
Regards,
Masahisa Kojima
>
> Regards,
> etienne
>
> > + auth->auth_info.hdr.wCertificateType = WIN_CERT_TYPE_EFI_GUID;
> > + guidcpy(&auth->auth_info.cert_type, &efi_guid_cert_type_pkcs7);
> > + if (db)
> > + memcpy((u8 *)auth + sizeof(struct
> > efi_variable_authentication_2), db, *size);
> > +
> > + *new_db = auth;
> > + *size = total_size;
> > +
> > + return EFI_SUCCESS;
> > +}
> > +
> > (snip)
