On Fri, 8 Jul 2022 at 13:37, Masahisa Kojima <masahisa.koj...@linaro.org> wrote: > > On Fri, 8 Jul 2022 at 18:14, Ilias Apalodimas > <ilias.apalodi...@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.koj...@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