| Message ID | 20220619052022.2694-2-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | eficonfig: add UEFI Secure Boot key maintenance interface | expand |
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@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. > + > + 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? > + 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
On Fri, 8 Jul 2022 at 18:14, Ilias Apalodimas <ilias.apalodimas@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@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
On Fri, 8 Jul 2022 at 13:37, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > On Fri, 8 Jul 2022 at 18:14, Ilias Apalodimas > <ilias.apalodimas@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@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
On 7/8/22 11:14, Ilias Apalodimas 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@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> >> + Please, provide function descriptions. >> +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. Assigning NULL would have no effect. The variable is reassigned in the next line. > >> + >> + 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? The UEFI specification says: "The PK variable contains *the* current Platform Key." So there should always be only one key in the variable. > >> + 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"); *%s/Fail/Failed/ Please, add unit tests for your patches. My expectation is that efi_set_variable_int() will only succeed if the variable change request is signed with the old PK or if PK does not exist. Under which circumstances shall a board owner be able to remove PK if he does not possess the private key? Best regards Heinrich >> + goto out; >> + } >> + >> +out: >> + free(file_info.current_path); >> + free(buf); >> + >> > [...] > > Thanks > /Ilias
Hi Heinrich, On Sun, 10 Jul 2022 at 18:37, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 7/8/22 11:14, Ilias Apalodimas 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@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> > >> + > > Please, provide function descriptions. OK. > > >> +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. > > Assigning NULL would have no effect. The variable is reassigned in the > next line. OK. > > > > >> + > >> + 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? > > The UEFI specification says: > > "The PK variable contains *the* current Platform Key." > > So there should always be only one key in the variable. Yes, my code always overwrites the platform key. > > > > >> + 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"); > > *%s/Fail/Failed/ > > Please, add unit tests for your patches. What kind of unit tests do you expect to be added? This patch series mainly adds the UI, do you want to add the unit tests to manipulate the UI and update the secure boot keys? Or do you want to enhance the efi_set_variable_int() test cases? > My expectation is that efi_set_variable_int() will only succeed if the > variable change request is signed with the old PK or if PK does not exist. For PK, the current implementation meets this expectation. Key configuration UI code I'm working on behaves like the wrapper of efi_set_variable_int(). efi_set_variable_int() checks if the variable change request is signed or not. > > Under which circumstances shall a board owner be able to remove PK if he > does not possess the private key? This is important, but I feel this is out of scope of my patch series. In current U-Boot implementation, there is no way to remove PK if the board owner does not possess the private key or signed NULL key. EDK2 implements the "Custom Mode" to update the PK, KEK, db and dbx with the non-signed signature list. To enter the Custom Mode, it requires that board owner is physically present at the board and it requires the platform specific implementation. I can't come up with other ideas for now. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > >> + goto out; > >> + } > >> + > >> +out: > >> + free(file_info.current_path); > >> + free(buf); > >> + > >> > > [...] > > > > Thanks > > /Ilias >
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); + + 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; + } + + 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); + + /* to stay the parent menu */ + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret; + + return ret; +} + +static struct eficonfig_item key_config_pk_menu_items[] = { + {"Enroll New Key", eficonfig_process_enroll_key}, + {"Quit", eficonfig_process_quit}, +}; + +static struct eficonfig_item key_config_menu_items[] = { + {"Enroll New Key", eficonfig_process_enroll_key}, + {"Quit", eficonfig_process_quit}, +}; + +static efi_status_t eficonfig_process_set_secure_boot_pk(void *data) +{ + u32 i; + efi_status_t ret; + + for (i = 0; i < ARRAY_SIZE(key_config_pk_menu_items); i++) + key_config_pk_menu_items[i].data = data; + + while (1) { + ret = eficonfig_process_common(key_config_pk_menu_items, + ARRAY_SIZE(key_config_pk_menu_items), + " ** Configure PK **"); + if (ret == EFI_ABORTED) + break; + } + + /* to stay the parent menu */ + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret; + + return ret; +} + +static efi_status_t eficonfig_process_set_secure_boot_key(void *data) +{ + u32 i; + efi_status_t ret; + char header_str[32]; + + for (i = 0; i < ARRAY_SIZE(key_config_menu_items); i++) + key_config_menu_items[i].data = data; + + snprintf(header_str, sizeof(header_str), " ** Configure %ls **", (u16 *)data); + + while (1) { + ret = eficonfig_process_common(key_config_menu_items, + ARRAY_SIZE(key_config_menu_items), + header_str); + if (ret == EFI_ABORTED) + break; + } + + /* to stay the parent menu */ + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret; + + return ret; +} + +static const struct eficonfig_item secure_boot_menu_items[] = { + {"PK", eficonfig_process_set_secure_boot_pk, u"PK"}, + {"KEK", eficonfig_process_set_secure_boot_key, u"KEK"}, + {"db", eficonfig_process_set_secure_boot_key, u"db"}, + {"dbx", eficonfig_process_set_secure_boot_key, u"dbx"}, + {"Quit", eficonfig_process_quit}, +}; + +efi_status_t eficonfig_process_secure_boot_config(void *data) +{ + efi_status_t ret; + + while (1) { + char header_str[64]; + + snprintf(header_str, sizeof(header_str), + " ** UEFI Secure Boot Key Configuration (SecureBoot : %s) **", + (is_secureboot_enabled() ? "ON" : "OFF")); + ret = eficonfig_process_common(secure_boot_menu_items, + ARRAY_SIZE(secure_boot_menu_items), + header_str); + if (ret == EFI_ABORTED) + break; + } + + /* to stay the parent menu */ + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret; + + return ret; +} diff --git a/include/efi_config.h b/include/efi_config.h index 1b48e47c48..c6c7a7ae6e 100644 --- a/include/efi_config.h +++ b/include/efi_config.h @@ -87,5 +87,8 @@ efi_status_t eficonfig_process_quit(void *data); efi_status_t eficonfig_process_common(const struct eficonfig_item *items, int count, char *menu_header); efi_status_t eficonfig_select_file_handler(void *data); +#ifdef CONFIG_EFI_SECURE_BOOT +efi_status_t eficonfig_process_secure_boot_config(void *data); +#endif #endif
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@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