[RFC PATCH 1/3] eficonfig: add UEFI Secure Boot Key enrollment interface

[Masahisa Kojima]

On Fri, 8 Jul 2022 at 18:14, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Sun, Jun 19, 2022 at 02:20:20PM +0900, Masahisa Kojima 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.kojima at linaro.org>
> > ---
> >  cmd/Makefile          |   3 +
> >  cmd/eficonfig.c       |   3 +
> >  cmd/eficonfig_sbkey.c | 202 ++++++++++++++++++++++++++++++++++++++++++
> >  include/efi_config.h  |   3 +
> >  4 files changed, 211 insertions(+)
> >  create mode 100644 cmd/eficonfig_sbkey.c
> >
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 0afa687e94..9d87b639fc 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -64,6 +64,9 @@ 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
> > +obj-$(CONFIG_EFI_SECURE_BOOT) += eficonfig_sbkey.o
> > +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 e62f5e41a4..e6d2cba9c5 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -1832,6 +1832,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))
> > +     {"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..a5c0dbe9b3
> > --- /dev/null
> > +++ b/cmd/eficonfig_sbkey.c
> > @@ -0,0 +1,202 @@
> > +// 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>
> > +
> > +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;
> > +}
> > +
> > +static efi_status_t eficonfig_process_enroll_key(void *data)
> > +{
> > +     u32 attr;
> > +     char *buf = NULL;
> > +     efi_uintn_t size;
> > +     efi_status_t ret;
> > +     struct efi_file_handle *f;
> > +     struct efi_file_handle *root;
> > +     struct eficonfig_select_file_info file_info;
> > +
> > +     file_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
> > +     if (!file_info.current_path)
> > +             goto out;
> > +
> > +     ret = eficonfig_select_file_handler(&file_info);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = efi_open_volume_int(file_info.current_volume, &root);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = efi_file_open_int(root, &f, file_info.current_path, EFI_FILE_MODE_READ, 0);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     size = 0;
> > +     ret = EFI_CALL(f->getinfo(f, &efi_file_info_guid, &size, NULL));
> > +     if (ret != EFI_BUFFER_TOO_SMALL)
> > +             goto out;
> > +
> > +     buf = calloc(1, size);
> > +     if (!buf) {
> > +             ret = EFI_OUT_OF_RESOURCES;
> > +             goto out;
> > +     }
> > +     ret = EFI_CALL(f->getinfo(f, &efi_file_info_guid, &size, buf));
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     size = ((struct efi_file_info *)buf)->file_size;
> > +     free(buf);
>
> You should set buf to NULL here.

Yes, thank you.

>
> > +
> > +     buf = calloc(1, size);
> > +     if (!buf)
> > +             goto out;
> > +
> > +     ret = efi_file_read_int(f, &size, buf);
> > +     if (ret != EFI_SUCCESS || size == 0)
> > +             goto out;
> > +
> > +     attr = EFI_VARIABLE_NON_VOLATILE |
> > +            EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +            EFI_VARIABLE_RUNTIME_ACCESS |
> > +            EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
> > +     /* PK can enroll only one certificate */
> > +     if (u16_strcmp(data, u"PK")) {
> > +             efi_uintn_t db_size = 0;
> > +
> > +             /* check the variable exists. If exists, add APPEND_WRITE attribute */
> > +             ret = efi_get_variable_int(data, efi_auth_var_get_guid(data), NULL,
> > +                                        &db_size, NULL,  NULL);
> > +             if (ret == EFI_BUFFER_TOO_SMALL)
> > +                     attr |= EFI_VARIABLE_APPEND_WRITE;
> > +     }
> > +
>
> Why are we appending? Shouldn't we always overwrite the platform key?

This is the case other than "PK", check the variable name above:

Anyway, the following comment might mislead, I will update the comment.
> > +     /* PK can enroll only one certificate */
> > +     if (u16_strcmp(data, u"PK")) {

Thanks,
Masahisa Kojima

>
> > +     ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
> > +                                attr, size, buf, false);
> > +     if (ret != EFI_SUCCESS) {
> > +             eficonfig_print_msg("ERROR! Fail to update signature database");
> > +             goto out;
> > +     }
> > +
> > +out:
> > +     free(file_info.current_path);
> > +     free(buf);
> > +
> >
> [...]
>
> Thanks
> /Ilias