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

[Ilias Apalodimas]

On Fri, 8 Jul 2022 at 13:37, Masahisa Kojima <masahisa.kojima at linaro.org> wrote:
>
> 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.

No need I just misread the code.

Regards
/Ilias
> > > +     /* 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