[RFC PATCH 0/3] eficonfig: add UEFI Secure Boot key maintenance interface

[RFC PATCH 0/3] eficonfig: add UEFI Secure Boot key maintenance interface

[Masahisa Kojima]

This series adds the UEFI Secure Boot key maintenance interface
to the eficonfig command.
User can enroll and delete the PK, KEK, db and dbx.

Note that this series is RFC since this series is implemented
on the top of the "enable menu-driven UEFI variable maintenance"
patch series still under review[1].

[1]https://lore.kernel.org/u-boot/20220619045607.1669-1-masahisa.kojima@linaro.org/T/#m7fe16b6044fbb2947b49c26051563c7cbb696fb3

Source code can be cloned with:
$ git clone https://git.linaro.org/people/masahisa.kojima/u-boot.git -b kojima/kojima/efi_seckey_menu_upstream_v1_0619

Masahisa Kojima (3):
  eficonfig: add UEFI Secure Boot Key enrollment interface
  eficonfig: add "Show Signature Database" menu entry
  eficonfig: add "Delete Key" menu entry

 cmd/Makefile          |   3 +
 cmd/eficonfig.c       |   3 +
 cmd/eficonfig_sbkey.c | 701 ++++++++++++++++++++++++++++++++++++++++++
 include/efi_config.h  |   3 +
 4 files changed, 710 insertions(+)
 create mode 100644 cmd/eficonfig_sbkey.c

-- 
2.17.1

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

[Masahisa Kojima]

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);
+
+	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
-- 
2.17.1

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

[Ilias Apalodimas]

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

Re: [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@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

Re: [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@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

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

[Heinrich Schuchardt]

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

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

[Masahisa Kojima]

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
>

[RFC PATCH 2/3] eficonfig: add "Show Signature Database" menu entry

[Masahisa Kojima]

This commit adds the menu-driven interface to show the
signature database.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 cmd/eficonfig_sbkey.c | 283 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 283 insertions(+)

diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
index a5c0dbe9b3..02ab8f8218 100644
--- a/cmd/eficonfig_sbkey.c
+++ b/cmd/eficonfig_sbkey.c
@@ -17,6 +17,64 @@
 #include <efi_variable.h>
 #include <crypto/pkcs7_parser.h>
 
+struct eficonfig_sig_data {
+	struct efi_signature_list *esl;
+	struct efi_signature_data *esd;
+	struct list_head list;
+	struct eficonfig_sig_data **selected;
+	u16 *varname;
+};
+
+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}, */
+};
+
+static void eficonfig_console_wait_enter(void)
+{
+	int esc = 0;
+	enum bootmenu_key key = KEY_NONE;
+
+	while (1) {
+		bootmenu_loop(NULL, &key, &esc);
+
+		switch (key) {
+		case KEY_SELECT:
+			return;
+		default:
+			break;
+		}
+	}
+
+	/* never happens */
+	debug("eficonfig: this should not happen");
+	return;
+}
+
 static bool is_secureboot_enabled(void)
 {
 	efi_status_t ret;
@@ -113,13 +171,238 @@ out:
 	return ret;
 }
 
+static void display_sigdata_info(struct eficonfig_sig_data *sg)
+{
+	u32 i;
+
+	for (i = 0; i < ARRAY_SIZE(sigtype_to_str); i++) {
+		if (!guidcmp(&sg->esl->signature_type, &sigtype_to_str[i].sig_type)) {
+			printf("    Signature Type:\n"
+			       "      %s\n", sigtype_to_str[i].str);
+
+			switch (sigtype_to_str[i].type) {
+			case SIG_TYPE_X509:
+			{
+				struct x509_certificate *cert_tmp;
+
+				cert_tmp = x509_cert_parse(sg->esd->signature_data,
+							   sg->esl->signature_size);
+				printf("    Subject:\n"
+				       "      %s\n"
+				       "    Issuer:\n"
+				       "      %s\n",
+				       cert_tmp->subject, cert_tmp->issuer);
+				break;
+			}
+			case SIG_TYPE_CRL:
+			{
+				u32 hash_size = sg->esl->signature_size - sizeof(efi_guid_t) -
+						sizeof(struct efi_time);
+				struct efi_time *time =
+					(struct efi_time *)((u8 *)sg->esd->signature_data +
+					hash_size);
+
+				printf("    ToBeSignedHash:\n");
+				print_hex_dump("      ", DUMP_PREFIX_NONE, 16, 1,
+					       sg->esd->signature_data, hash_size, false);
+				printf("    TimeOfRevocation:\n"
+				       "      %d-%d-%d %02d:%02d:%02d\n",
+				       time->year, time->month, time->day,
+				       time->hour, time->minute, time->second);
+				break;
+			}
+			case SIG_TYPE_HASH:
+			{
+				u32 hash_size = sg->esl->signature_size - sizeof(efi_guid_t);
+
+				printf("    Hash:\n");
+				print_hex_dump("      ", DUMP_PREFIX_NONE, 16, 1,
+					       sg->esd->signature_data, hash_size, false);
+				break;
+			}
+			default:
+				eficonfig_print_msg("ERROR! Unsupported format.");
+				break;
+			}
+		}
+	}
+}
+
+static void display_sigdata_header(struct eficonfig_sig_data *sg, char *str)
+{
+	puts(ANSI_CURSOR_HIDE);
+	puts(ANSI_CLEAR_CONSOLE);
+	printf(ANSI_CURSOR_POSITION, 1, 1);
+
+	*sg->selected = sg;
+	printf("\n  *** U-Boot Signature Database (%s %ls) ***\n\n"
+	       "    Owner GUID:\n"
+	       "      %pUL\n",
+	       str, sg->varname, sg->esd->signature_owner.b);
+}
+
+static efi_status_t eficonfig_process_sigdata_show(void *data)
+{
+	struct eficonfig_sig_data *sg = data;
+
+	display_sigdata_header(sg, "Show");
+	display_sigdata_info(sg);
+
+	printf("\n\n  Press ENTER to continue");
+	eficonfig_console_wait_enter();
+
+	return EFI_SUCCESS;
+}
+
+static efi_status_t prepare_signature_db_list(struct eficonfig_item **output, void *varname,
+					      void *db, efi_uintn_t db_size,
+					      eficonfig_entry_func func,
+					      struct eficonfig_sig_data **selected,
+					      struct list_head *siglist_list,
+					      u32 *count)
+{
+	u32 num = 0;
+	efi_uintn_t size;
+	struct list_head *pos, *n;
+	struct efi_signature_list *esl;
+	struct efi_signature_data *esd;
+	struct eficonfig_item *menu_item, *iter;
+	struct eficonfig_sig_data *sg;
+
+	INIT_LIST_HEAD(siglist_list);
+	esl = db;
+	size = db_size;
+
+	/*
+	 * parse the signature database and save the pointers to
+	 * efi_signature_list and efi_signature_data.
+	 * We expect the signature list is saved in correct format.
+	 */
+	while (size > 0) {
+		u32 remain;
+
+		esd = (struct efi_signature_data *)((u8 *)esl +
+						    (sizeof(struct efi_signature_list) +
+						    esl->signature_header_size));
+		remain = esl->signature_list_size - (sizeof(struct efi_signature_list) +
+						     esl->signature_header_size);
+		for (; remain > 0; remain -= esl->signature_size) {
+			sg = calloc(1, sizeof(struct eficonfig_sig_data));
+			if (!sg)
+				return EFI_OUT_OF_RESOURCES;
+
+			sg->esl = esl;
+			sg->esd = esd;
+			list_add_tail(&sg->list, siglist_list);
+			esd = (struct efi_signature_data *)((u8 *)esd + esl->signature_size);
+			num++;
+		}
+
+		size -= esl->signature_list_size;
+		esl = (struct efi_signature_list *)((u8 *)esl + esl->signature_list_size);
+	}
+
+	menu_item = calloc(num + 1, sizeof(struct eficonfig_item));
+	if (!menu_item)
+		return EFI_OUT_OF_RESOURCES;
+
+	iter = menu_item;
+	list_for_each_safe(pos, n, siglist_list) {
+		char buf[40] = {0};
+		char *title;
+
+		sg = list_entry(pos, struct eficonfig_sig_data, list);
+
+		snprintf(buf, sizeof(buf), "%pUL", &sg->esd->signature_owner);
+		title = calloc(1, (strlen(buf) + 1));
+		if (!title)
+			return EFI_OUT_OF_RESOURCES;
+
+		strlcpy(title, buf, strlen(buf) + 1);
+		iter->title = title;
+		sg->selected = selected;
+		sg->varname = varname;
+		iter->func = func;
+		iter->data = sg;
+		iter++;
+	}
+
+	/* add "Quit" entry */
+	iter->title = "Quit";
+	iter->func = eficonfig_process_quit;
+	iter->data = NULL;
+	num += 1;
+
+	*count = num;
+	*output = menu_item;
+
+	return EFI_SUCCESS;
+}
+
+static efi_status_t process_show_signature_db(void *varname)
+{
+	u32 i, count = 0;
+	efi_status_t ret;
+	struct eficonfig_item *menu_item = NULL, *iter;
+	void *db = NULL;
+	efi_uintn_t db_size;
+	struct list_head siglist_list;
+	struct eficonfig_sig_data *selected;
+
+	db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
+	if (!db) {
+		eficonfig_print_msg("There is no entry in the signature database.");
+		return EFI_NOT_FOUND;
+	}
+
+	ret = prepare_signature_db_list(&menu_item, varname, db, db_size,
+					eficonfig_process_sigdata_show, &selected,
+					&siglist_list, &count);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = eficonfig_process_common(menu_item, count, "  ** Show Signature Database **");
+
+out:
+	if (menu_item) {
+		iter = menu_item;
+		for (i = 0; i < count - 1; iter++, i++) {
+			free(iter->title);
+			free(iter->data);
+		}
+	}
+
+	free(menu_item);
+	free(db);
+
+	return ret;
+}
+
+static efi_status_t eficonfig_process_show_signature_db(void *data)
+{
+	efi_status_t ret;
+
+	while (1) {
+		ret = process_show_signature_db(data);
+		if (ret != EFI_SUCCESS)
+			break;
+	}
+
+	/* 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},
+	{"Show Signature Database", eficonfig_process_show_signature_db},
 	{"Quit", eficonfig_process_quit},
 };
 
 static struct eficonfig_item key_config_menu_items[] = {
 	{"Enroll New Key", eficonfig_process_enroll_key},
+	{"Show Signature Database", eficonfig_process_show_signature_db},
 	{"Quit", eficonfig_process_quit},
 };
 
-- 
2.17.1

[RFC PATCH 3/3] eficonfig: add "Delete Key" menu entry

[Masahisa Kojima]

This commit add the menu-driven interface to delete the
signature database entry.
EFI Signature Lists can contain the multiple signature
entries, this menu can delete the indivisual entry.

If the PK is enrolled and UEFI Secure Boot is in User Mode,
user can not delete the existing signature lists since the
signature lists must be signed by KEK or PK but signing
information is not stored in the signature database.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 cmd/eficonfig_sbkey.c | 218 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 217 insertions(+), 1 deletion(-)

diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
index 02ab8f8218..142bb4cef5 100644
--- a/cmd/eficonfig_sbkey.c
+++ b/cmd/eficonfig_sbkey.c
@@ -54,6 +54,29 @@ static const struct eficonfig_sigtype_to_str sigtype_to_str[] = {
 /*	{EFI_CERT_SHA512_GUID,		"SHA512",		SIG_TYPE_HASH}, */
 };
 
+static int eficonfig_console_yes_no(void)
+{
+	int esc = 0;
+	enum bootmenu_key key = KEY_NONE;
+
+	while (1) {
+		bootmenu_loop(NULL, &key, &esc);
+
+		switch (key) {
+		case KEY_SELECT:
+			return 1;
+		case KEY_QUIT:
+			return 0;
+		default:
+			break;
+		}
+	}
+
+	/* never happens */
+	debug("eficonfig: this should not happen");
+	return 0;
+}
+
 static void eficonfig_console_wait_enter(void)
 {
 	int esc = 0;
@@ -72,7 +95,19 @@ static void eficonfig_console_wait_enter(void)
 
 	/* never happens */
 	debug("eficonfig: this should not happen");
-	return;
+}
+
+static bool is_setupmode(void)
+{
+	efi_status_t ret;
+	u8 setup_mode;
+	efi_uintn_t size;
+
+	size = sizeof(setup_mode);
+	ret = efi_get_variable_int(u"SetupMode", &efi_global_variable_guid,
+				   NULL, &size, &setup_mode, NULL);
+
+	return setup_mode == 1;
 }
 
 static bool is_secureboot_enabled(void)
@@ -254,6 +289,103 @@ static efi_status_t eficonfig_process_sigdata_show(void *data)
 	return EFI_SUCCESS;
 }
 
+static efi_status_t eficonfig_process_sigdata_delete(void *data)
+{
+	int yes_no;
+	struct eficonfig_sig_data *sg = data;
+
+	display_sigdata_header(sg, "Delete");
+	display_sigdata_info(sg);
+
+	printf("\n\n  Press ENTER to delete, ESC/CTRL+C to quit");
+	yes_no = eficonfig_console_yes_no();
+	if (!yes_no)
+		return EFI_NOT_READY;
+
+	return EFI_SUCCESS;
+}
+
+static void delete_selected_signature_data(void *db, efi_uintn_t *db_size,
+					   struct eficonfig_sig_data *target,
+					   struct list_head *siglist_list)
+{
+	u8 *dest, *start;
+	struct list_head *pos, *n;
+	u32 remain;
+	u32 size = *db_size;
+	u8 *end = (u8 *)db + size;
+	struct eficonfig_sig_data *sg;
+
+	list_for_each_safe(pos, n, siglist_list) {
+		sg = list_entry(pos, struct eficonfig_sig_data, list);
+		if (sg->esl == target->esl && sg->esd == target->esd) {
+			remain = sg->esl->signature_list_size -
+				 (sizeof(struct efi_signature_list) -
+				 sg->esl->signature_header_size) -
+				 sg->esl->signature_size;
+			if (remain > 0) {
+				/* only delete the single signature data */
+				sg->esl->signature_list_size -= sg->esl->signature_size;
+				size -= sg->esl->signature_size;
+				dest = (u8 *)sg->esd;
+				start = (u8 *)sg->esd + sg->esl->signature_size;
+			} else {
+				/* delete entire signature list */
+				dest = (u8 *)sg->esl;
+				start = (u8 *)sg->esl + sg->esl->signature_list_size;
+				size -= sg->esl->signature_list_size;
+			}
+			memmove(dest, start, (end - start));
+		}
+	}
+
+	*db_size = size;
+}
+
+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;
+	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;
+}
+
 static efi_status_t prepare_signature_db_list(struct eficonfig_item **output, void *varname,
 					      void *db, efi_uintn_t db_size,
 					      eficonfig_entry_func func,
@@ -378,6 +510,68 @@ out:
 	return ret;
 }
 
+static efi_status_t process_delete_key(void *varname)
+{
+	u32 attr, i, count = 0;
+	efi_status_t ret;
+	struct eficonfig_item *menu_item = NULL, *iter;
+	void *db = NULL, *new_db = NULL;
+	efi_uintn_t db_size;
+	struct list_head siglist_list;
+	struct eficonfig_sig_data *selected;
+
+	db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
+	if (!db) {
+		eficonfig_print_msg("There is no entry in the signature database.");
+		return EFI_NOT_FOUND;
+	}
+
+	ret = prepare_signature_db_list(&menu_item, varname, db, db_size,
+					eficonfig_process_sigdata_delete, &selected,
+					&siglist_list, &count);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = eficonfig_process_common(menu_item, count, " ** Delete Key **");
+
+	if (ret == EFI_SUCCESS) {
+		delete_selected_signature_data(db, &db_size, selected, &siglist_list);
+
+		ret = create_time_based_payload(db, &new_db, &db_size);
+		if (ret != EFI_SUCCESS)
+			goto out;
+
+		attr = EFI_VARIABLE_NON_VOLATILE |
+		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
+		       EFI_VARIABLE_RUNTIME_ACCESS |
+		       EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
+		ret = efi_set_variable_int((u16 *)varname, efi_auth_var_get_guid((u16 *)varname),
+					   attr, db_size, new_db, false);
+		if (ret != EFI_SUCCESS) {
+			eficonfig_print_msg("ERROR! Fail to delete signature database");
+			goto out;
+		}
+	}
+
+out:
+	if (menu_item) {
+		iter = menu_item;
+		for (i = 0; i < count - 1; iter++, i++) {
+			free(iter->title);
+			free(iter->data);
+		}
+	}
+
+	free(menu_item);
+	free(db);
+	free(new_db);
+
+	/* to stay the parent menu */
+	ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
+
+	return ret;
+}
+
 static efi_status_t eficonfig_process_show_signature_db(void *data)
 {
 	efi_status_t ret;
@@ -394,6 +588,27 @@ static efi_status_t eficonfig_process_show_signature_db(void *data)
 	return ret;
 }
 
+static efi_status_t eficonfig_process_delete_key(void *data)
+{
+	efi_status_t ret;
+
+	if (!is_setupmode()) {
+		eficonfig_print_msg("Not in the SetupMode, can not delete.");
+		return EFI_SUCCESS;
+	}
+
+	while (1) {
+		ret = process_delete_key(data);
+		if (ret != EFI_SUCCESS)
+			break;
+	}
+
+	/* 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},
 	{"Show Signature Database", eficonfig_process_show_signature_db},
@@ -403,6 +618,7 @@ static struct eficonfig_item key_config_pk_menu_items[] = {
 static struct eficonfig_item key_config_menu_items[] = {
 	{"Enroll New Key", eficonfig_process_enroll_key},
 	{"Show Signature Database", eficonfig_process_show_signature_db},
+	{"Delete Key", eficonfig_process_delete_key},
 	{"Quit", eficonfig_process_quit},
 };
 
-- 
2.17.1

Re: [RFC PATCH 3/3] eficonfig: add "Delete Key" menu entry

[Heinrich Schuchardt]

On 6/19/22 07:20, Masahisa Kojima wrote:
> This commit add the menu-driven interface to delete the
> signature database entry.
> EFI Signature Lists can contain the multiple signature
> entries, this menu can delete the indivisual entry.
>
> If the PK is enrolled and UEFI Secure Boot is in User Mode,

Why don't you mention Deployed Mode?

> user can not delete the existing signature lists since the
> signature lists must be signed by KEK or PK but signing
> information is not stored in the signature database.

Updating PK with an empty value must be possible if if the new value is
signed with the old PK.

>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>   cmd/eficonfig_sbkey.c | 218 +++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 217 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> index 02ab8f8218..142bb4cef5 100644
> --- a/cmd/eficonfig_sbkey.c
> +++ b/cmd/eficonfig_sbkey.c
> @@ -54,6 +54,29 @@ static const struct eficonfig_sigtype_to_str sigtype_to_str[] = {
>   /*	{EFI_CERT_SHA512_GUID,		"SHA512",		SIG_TYPE_HASH}, */
>   };
>

Please, add documentation to all functions.

> +static int eficonfig_console_yes_no(void)
> +{
> +	int esc = 0;
> +	enum bootmenu_key key = KEY_NONE;
> +
> +	while (1) {
> +		bootmenu_loop(NULL, &key, &esc);
> +
> +		switch (key) {
> +		case KEY_SELECT:
> +			return 1;
> +		case KEY_QUIT:
> +			return 0;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	/* never happens */
> +	debug("eficonfig: this should not happen");
> +	return 0;

If this code is unreachable, remove it.

> +}
> +
>   static void eficonfig_console_wait_enter(void)
>   {
>   	int esc = 0;
> @@ -72,7 +95,19 @@ static void eficonfig_console_wait_enter(void)
>
>   	/* never happens */
>   	debug("eficonfig: this should not happen");
> -	return;

Please remove unreachable code.

> +}
> +
> +static bool is_setupmode(void)
> +{
> +	efi_status_t ret;
> +	u8 setup_mode;
> +	efi_uintn_t size;
> +
> +	size = sizeof(setup_mode);
> +	ret = efi_get_variable_int(u"SetupMode", &efi_global_variable_guid,
> +				   NULL, &size, &setup_mode, NULL);
> +
> +	return setup_mode == 1;
>   }
>
>   static bool is_secureboot_enabled(void)
> @@ -254,6 +289,103 @@ static efi_status_t eficonfig_process_sigdata_show(void *data)
>   	return EFI_SUCCESS;
>   }
>
> +static efi_status_t eficonfig_process_sigdata_delete(void *data)
> +{
> +	int yes_no;
> +	struct eficonfig_sig_data *sg = data;
> +
> +	display_sigdata_header(sg, "Delete");
> +	display_sigdata_info(sg);
> +
> +	printf("\n\n  Press ENTER to delete, ESC/CTRL+C to quit");
> +	yes_no = eficonfig_console_yes_no();
> +	if (!yes_no)
> +		return EFI_NOT_READY;
> +
> +	return EFI_SUCCESS;
> +}
> +
> +static void delete_selected_signature_data(void *db, efi_uintn_t *db_size,
> +					   struct eficonfig_sig_data *target,
> +					   struct list_head *siglist_list)
> +{
> +	u8 *dest, *start;
> +	struct list_head *pos, *n;
> +	u32 remain;
> +	u32 size = *db_size;
> +	u8 *end = (u8 *)db + size;
> +	struct eficonfig_sig_data *sg;
> +
> +	list_for_each_safe(pos, n, siglist_list) {
> +		sg = list_entry(pos, struct eficonfig_sig_data, list);
> +		if (sg->esl == target->esl && sg->esd == target->esd) {
> +			remain = sg->esl->signature_list_size -
> +				 (sizeof(struct efi_signature_list) -
> +				 sg->esl->signature_header_size) -
> +				 sg->esl->signature_size;
> +			if (remain > 0) {
> +				/* only delete the single signature data */
> +				sg->esl->signature_list_size -= sg->esl->signature_size;
> +				size -= sg->esl->signature_size;
> +				dest = (u8 *)sg->esd;
> +				start = (u8 *)sg->esd + sg->esl->signature_size;
> +			} else {
> +				/* delete entire signature list */
> +				dest = (u8 *)sg->esl;
> +				start = (u8 *)sg->esl + sg->esl->signature_list_size;
> +				size -= sg->esl->signature_list_size;
> +			}
> +			memmove(dest, start, (end - start));
> +		}
> +	}
> +
> +	*db_size = size;
> +}
> +
> +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;
> +	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;
> +}
> +
>   static efi_status_t prepare_signature_db_list(struct eficonfig_item **output, void *varname,
>   					      void *db, efi_uintn_t db_size,
>   					      eficonfig_entry_func func,
> @@ -378,6 +510,68 @@ out:
>   	return ret;
>   }
>
> +static efi_status_t process_delete_key(void *varname)
> +{
> +	u32 attr, i, count = 0;
> +	efi_status_t ret;
> +	struct eficonfig_item *menu_item = NULL, *iter;
> +	void *db = NULL, *new_db = NULL;
> +	efi_uintn_t db_size;
> +	struct list_head siglist_list;
> +	struct eficonfig_sig_data *selected;
> +
> +	db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
> +	if (!db) {
> +		eficonfig_print_msg("There is no entry in the signature database.");

Please, use the terms of the UEFI specification.
%s/signature database/signature store/

> +		return EFI_NOT_FOUND;
> +	}
> +
> +	ret = prepare_signature_db_list(&menu_item, varname, db, db_size,
> +					eficonfig_process_sigdata_delete, &selected,
> +					&siglist_list, &count);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = eficonfig_process_common(menu_item, count, " ** Delete Key **");
> +
> +	if (ret == EFI_SUCCESS) {
> +		delete_selected_signature_data(db, &db_size, selected, &siglist_list);
> +
> +		ret = create_time_based_payload(db, &new_db, &db_size);
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +
> +		attr = EFI_VARIABLE_NON_VOLATILE |
> +		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +		       EFI_VARIABLE_RUNTIME_ACCESS |
> +		       EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
> +		ret = efi_set_variable_int((u16 *)varname, efi_auth_var_get_guid((u16 *)varname),
> +					   attr, db_size, new_db, false);
> +		if (ret != EFI_SUCCESS) {
> +			eficonfig_print_msg("ERROR! Fail to delete signature database");

%s/Fail/Failed/

Please, use the terms of the UEFI specification.
%s/signature database/signature store/

> +			goto out;
> +		}
> +	}
> +
> +out:
> +	if (menu_item) {
> +		iter = menu_item;
> +		for (i = 0; i < count - 1; iter++, i++) {
> +			free(iter->title);
> +			free(iter->data);
> +		}
> +	}
> +
> +	free(menu_item);
> +	free(db);
> +	free(new_db);
> +
> +	/* to stay the parent menu */
> +	ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> +
> +	return ret;
> +}
> +
>   static efi_status_t eficonfig_process_show_signature_db(void *data)
>   {
>   	efi_status_t ret;
> @@ -394,6 +588,27 @@ static efi_status_t eficonfig_process_show_signature_db(void *data)
>   	return ret;
>   }
>
> +static efi_status_t eficonfig_process_delete_key(void *data)
> +{
> +	efi_status_t ret;
> +
> +	if (!is_setupmode()) {
> +		eficonfig_print_msg("Not in the SetupMode, can not delete.");

Both in Setup Mode and in Audit Mode you should be able to edit keys.

> +		return EFI_SUCCESS;
> +	}
> +
> +	while (1) {
> +		ret = process_delete_key(data);
> +		if (ret != EFI_SUCCESS)
> +			break;
> +	}
> +
> +	/* 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},
>   	{"Show Signature Database", eficonfig_process_show_signature_db},

%s/Signature Database/signature store/

> @@ -403,6 +618,7 @@ static struct eficonfig_item key_config_pk_menu_items[] = {
>   static struct eficonfig_item key_config_menu_items[] = {
>   	{"Enroll New Key", eficonfig_process_enroll_key},
>   	{"Show Signature Database", eficonfig_process_show_signature_db},

%s/Signature Database/signature store/

Best regards

Heinrich

> +	{"Delete Key", eficonfig_process_delete_key},
>   	{"Quit", eficonfig_process_quit},
>   };
>

Re: [RFC PATCH 3/3] eficonfig: add "Delete Key" menu entry

[Takahiro Akashi]

On Sun, Jul 10, 2022 at 12:10:13PM +0200, Heinrich Schuchardt wrote:
> On 6/19/22 07:20, Masahisa Kojima wrote:
> > This commit add the menu-driven interface to delete the
> > signature database entry.
> > EFI Signature Lists can contain the multiple signature
> > entries, this menu can delete the indivisual entry.
> > 
> > If the PK is enrolled and UEFI Secure Boot is in User Mode,
> 
> Why don't you mention Deployed Mode?
> 
> > user can not delete the existing signature lists since the
> > signature lists must be signed by KEK or PK but signing
> > information is not stored in the signature database.
> 
> Updating PK with an empty value must be possible if if the new value is
> signed with the old PK.
> 
> > 
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   cmd/eficonfig_sbkey.c | 218 +++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 217 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> > index 02ab8f8218..142bb4cef5 100644
> > --- a/cmd/eficonfig_sbkey.c
> > +++ b/cmd/eficonfig_sbkey.c
> > @@ -54,6 +54,29 @@ static const struct eficonfig_sigtype_to_str sigtype_to_str[] = {
> >   /*	{EFI_CERT_SHA512_GUID,		"SHA512",		SIG_TYPE_HASH}, */
> >   };
> > 
> 
> Please, add documentation to all functions.
> 
> > +static int eficonfig_console_yes_no(void)
> > +{
> > +	int esc = 0;
> > +	enum bootmenu_key key = KEY_NONE;
> > +
> > +	while (1) {
> > +		bootmenu_loop(NULL, &key, &esc);
> > +
> > +		switch (key) {
> > +		case KEY_SELECT:
> > +			return 1;
> > +		case KEY_QUIT:
> > +			return 0;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	/* never happens */
> > +	debug("eficonfig: this should not happen");
> > +	return 0;
> 
> If this code is unreachable, remove it.
> 
> > +}
> > +
> >   static void eficonfig_console_wait_enter(void)
> >   {
> >   	int esc = 0;
> > @@ -72,7 +95,19 @@ static void eficonfig_console_wait_enter(void)
> > 
> >   	/* never happens */
> >   	debug("eficonfig: this should not happen");
> > -	return;
> 
> Please remove unreachable code.
> 
> > +}
> > +
> > +static bool is_setupmode(void)
> > +{
> > +	efi_status_t ret;
> > +	u8 setup_mode;
> > +	efi_uintn_t size;
> > +
> > +	size = sizeof(setup_mode);
> > +	ret = efi_get_variable_int(u"SetupMode", &efi_global_variable_guid,
> > +				   NULL, &size, &setup_mode, NULL);
> > +
> > +	return setup_mode == 1;
> >   }
> > 
> >   static bool is_secureboot_enabled(void)
> > @@ -254,6 +289,103 @@ static efi_status_t eficonfig_process_sigdata_show(void *data)
> >   	return EFI_SUCCESS;
> >   }
> > 
> > +static efi_status_t eficonfig_process_sigdata_delete(void *data)
> > +{
> > +	int yes_no;
> > +	struct eficonfig_sig_data *sg = data;
> > +
> > +	display_sigdata_header(sg, "Delete");
> > +	display_sigdata_info(sg);
> > +
> > +	printf("\n\n  Press ENTER to delete, ESC/CTRL+C to quit");
> > +	yes_no = eficonfig_console_yes_no();
> > +	if (!yes_no)
> > +		return EFI_NOT_READY;
> > +
> > +	return EFI_SUCCESS;
> > +}
> > +
> > +static void delete_selected_signature_data(void *db, efi_uintn_t *db_size,
> > +					   struct eficonfig_sig_data *target,
> > +					   struct list_head *siglist_list)
> > +{
> > +	u8 *dest, *start;
> > +	struct list_head *pos, *n;
> > +	u32 remain;
> > +	u32 size = *db_size;
> > +	u8 *end = (u8 *)db + size;
> > +	struct eficonfig_sig_data *sg;
> > +
> > +	list_for_each_safe(pos, n, siglist_list) {
> > +		sg = list_entry(pos, struct eficonfig_sig_data, list);
> > +		if (sg->esl == target->esl && sg->esd == target->esd) {
> > +			remain = sg->esl->signature_list_size -
> > +				 (sizeof(struct efi_signature_list) -
> > +				 sg->esl->signature_header_size) -
> > +				 sg->esl->signature_size;
> > +			if (remain > 0) {
> > +				/* only delete the single signature data */
> > +				sg->esl->signature_list_size -= sg->esl->signature_size;
> > +				size -= sg->esl->signature_size;
> > +				dest = (u8 *)sg->esd;
> > +				start = (u8 *)sg->esd + sg->esl->signature_size;
> > +			} else {
> > +				/* delete entire signature list */
> > +				dest = (u8 *)sg->esl;
> > +				start = (u8 *)sg->esl + sg->esl->signature_list_size;
> > +				size -= sg->esl->signature_list_size;
> > +			}
> > +			memmove(dest, start, (end - start));
> > +		}
> > +	}
> > +
> > +	*db_size = size;
> > +}
> > +
> > +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;
> > +	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;
> > +}
> > +
> >   static efi_status_t prepare_signature_db_list(struct eficonfig_item **output, void *varname,
> >   					      void *db, efi_uintn_t db_size,
> >   					      eficonfig_entry_func func,
> > @@ -378,6 +510,68 @@ out:
> >   	return ret;
> >   }
> > 
> > +static efi_status_t process_delete_key(void *varname)
> > +{
> > +	u32 attr, i, count = 0;
> > +	efi_status_t ret;
> > +	struct eficonfig_item *menu_item = NULL, *iter;
> > +	void *db = NULL, *new_db = NULL;
> > +	efi_uintn_t db_size;
> > +	struct list_head siglist_list;
> > +	struct eficonfig_sig_data *selected;
> > +
> > +	db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
> > +	if (!db) {
> > +		eficonfig_print_msg("There is no entry in the signature database.");
> 
> Please, use the terms of the UEFI specification.
> %s/signature database/signature store/

Signature database is also a common term throughout the specification.
See "section 32.4.1 Signature Database", for example.

-Takahiro Akashi


> > +		return EFI_NOT_FOUND;
> > +	}
> > +
> > +	ret = prepare_signature_db_list(&menu_item, varname, db, db_size,
> > +					eficonfig_process_sigdata_delete, &selected,
> > +					&siglist_list, &count);
> > +	if (ret != EFI_SUCCESS)
> > +		goto out;
> > +
> > +	ret = eficonfig_process_common(menu_item, count, " ** Delete Key **");
> > +
> > +	if (ret == EFI_SUCCESS) {
> > +		delete_selected_signature_data(db, &db_size, selected, &siglist_list);
> > +
> > +		ret = create_time_based_payload(db, &new_db, &db_size);
> > +		if (ret != EFI_SUCCESS)
> > +			goto out;
> > +
> > +		attr = EFI_VARIABLE_NON_VOLATILE |
> > +		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +		       EFI_VARIABLE_RUNTIME_ACCESS |
> > +		       EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
> > +		ret = efi_set_variable_int((u16 *)varname, efi_auth_var_get_guid((u16 *)varname),
> > +					   attr, db_size, new_db, false);
> > +		if (ret != EFI_SUCCESS) {
> > +			eficonfig_print_msg("ERROR! Fail to delete signature database");
> 
> %s/Fail/Failed/
> 
> Please, use the terms of the UEFI specification.
> %s/signature database/signature store/
> 
> > +			goto out;
> > +		}
> > +	}
> > +
> > +out:
> > +	if (menu_item) {
> > +		iter = menu_item;
> > +		for (i = 0; i < count - 1; iter++, i++) {
> > +			free(iter->title);
> > +			free(iter->data);
> > +		}
> > +	}
> > +
> > +	free(menu_item);
> > +	free(db);
> > +	free(new_db);
> > +
> > +	/* to stay the parent menu */
> > +	ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> > +
> > +	return ret;
> > +}
> > +
> >   static efi_status_t eficonfig_process_show_signature_db(void *data)
> >   {
> >   	efi_status_t ret;
> > @@ -394,6 +588,27 @@ static efi_status_t eficonfig_process_show_signature_db(void *data)
> >   	return ret;
> >   }
> > 
> > +static efi_status_t eficonfig_process_delete_key(void *data)
> > +{
> > +	efi_status_t ret;
> > +
> > +	if (!is_setupmode()) {
> > +		eficonfig_print_msg("Not in the SetupMode, can not delete.");
> 
> Both in Setup Mode and in Audit Mode you should be able to edit keys.
> 
> > +		return EFI_SUCCESS;
> > +	}
> > +
> > +	while (1) {
> > +		ret = process_delete_key(data);
> > +		if (ret != EFI_SUCCESS)
> > +			break;
> > +	}
> > +
> > +	/* 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},
> >   	{"Show Signature Database", eficonfig_process_show_signature_db},
> 
> %s/Signature Database/signature store/
> 
> > @@ -403,6 +618,7 @@ static struct eficonfig_item key_config_pk_menu_items[] = {
> >   static struct eficonfig_item key_config_menu_items[] = {
> >   	{"Enroll New Key", eficonfig_process_enroll_key},
> >   	{"Show Signature Database", eficonfig_process_show_signature_db},
> 
> %s/Signature Database/signature store/
> 
> Best regards
> 
> Heinrich
> 
> > +	{"Delete Key", eficonfig_process_delete_key},
> >   	{"Quit", eficonfig_process_quit},
> >   };
> > 
> 

Re: [RFC PATCH 3/3] eficonfig: add "Delete Key" menu entry

[Masahisa Kojima]

Hi Heinrich, Takahiro,

On Sun, 10 Jul 2022 at 19:10, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/19/22 07:20, Masahisa Kojima wrote:
> > This commit add the menu-driven interface to delete the
> > signature database entry.
> > EFI Signature Lists can contain the multiple signature
> > entries, this menu can delete the indivisual entry.
> >
> > If the PK is enrolled and UEFI Secure Boot is in User Mode,
>
> Why don't you mention Deployed Mode?

Yes, I will mention DeployedMode.

>
> > user can not delete the existing signature lists since the
> > signature lists must be signed by KEK or PK but signing
> > information is not stored in the signature database.
>
> Updating PK with an empty value must be possible if if the new value is
> signed with the old PK.

Yes, I will add this description.

>
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   cmd/eficonfig_sbkey.c | 218 +++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 217 insertions(+), 1 deletion(-)
> >
> > diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> > index 02ab8f8218..142bb4cef5 100644
> > --- a/cmd/eficonfig_sbkey.c
> > +++ b/cmd/eficonfig_sbkey.c
> > @@ -54,6 +54,29 @@ static const struct eficonfig_sigtype_to_str sigtype_to_str[] = {
> >   /*  {EFI_CERT_SHA512_GUID,          "SHA512",               SIG_TYPE_HASH}, */
> >   };
> >
>
> Please, add documentation to all functions.

OK.

>
> > +static int eficonfig_console_yes_no(void)
> > +{
> > +     int esc = 0;
> > +     enum bootmenu_key key = KEY_NONE;
> > +
> > +     while (1) {
> > +             bootmenu_loop(NULL, &key, &esc);
> > +
> > +             switch (key) {
> > +             case KEY_SELECT:
> > +                     return 1;
> > +             case KEY_QUIT:
> > +                     return 0;
> > +             default:
> > +                     break;
> > +             }
> > +     }
> > +
> > +     /* never happens */
> > +     debug("eficonfig: this should not happen");
> > +     return 0;
>
> If this code is unreachable, remove it.

OK.

>
> > +}
> > +
> >   static void eficonfig_console_wait_enter(void)
> >   {
> >       int esc = 0;
> > @@ -72,7 +95,19 @@ static void eficonfig_console_wait_enter(void)
> >
> >       /* never happens */
> >       debug("eficonfig: this should not happen");
> > -     return;
>
> Please remove unreachable code.

OK.

>
> > +}
> > +
> > +static bool is_setupmode(void)
> > +{
> > +     efi_status_t ret;
> > +     u8 setup_mode;
> > +     efi_uintn_t size;
> > +
> > +     size = sizeof(setup_mode);
> > +     ret = efi_get_variable_int(u"SetupMode", &efi_global_variable_guid,
> > +                                NULL, &size, &setup_mode, NULL);
> > +
> > +     return setup_mode == 1;
> >   }
> >
> >   static bool is_secureboot_enabled(void)
> > @@ -254,6 +289,103 @@ static efi_status_t eficonfig_process_sigdata_show(void *data)
> >       return EFI_SUCCESS;
> >   }
> >
> > +static efi_status_t eficonfig_process_sigdata_delete(void *data)
> > +{
> > +     int yes_no;
> > +     struct eficonfig_sig_data *sg = data;
> > +
> > +     display_sigdata_header(sg, "Delete");
> > +     display_sigdata_info(sg);
> > +
> > +     printf("\n\n  Press ENTER to delete, ESC/CTRL+C to quit");
> > +     yes_no = eficonfig_console_yes_no();
> > +     if (!yes_no)
> > +             return EFI_NOT_READY;
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
> > +static void delete_selected_signature_data(void *db, efi_uintn_t *db_size,
> > +                                        struct eficonfig_sig_data *target,
> > +                                        struct list_head *siglist_list)
> > +{
> > +     u8 *dest, *start;
> > +     struct list_head *pos, *n;
> > +     u32 remain;
> > +     u32 size = *db_size;
> > +     u8 *end = (u8 *)db + size;
> > +     struct eficonfig_sig_data *sg;
> > +
> > +     list_for_each_safe(pos, n, siglist_list) {
> > +             sg = list_entry(pos, struct eficonfig_sig_data, list);
> > +             if (sg->esl == target->esl && sg->esd == target->esd) {
> > +                     remain = sg->esl->signature_list_size -
> > +                              (sizeof(struct efi_signature_list) -
> > +                              sg->esl->signature_header_size) -
> > +                              sg->esl->signature_size;
> > +                     if (remain > 0) {
> > +                             /* only delete the single signature data */
> > +                             sg->esl->signature_list_size -= sg->esl->signature_size;
> > +                             size -= sg->esl->signature_size;
> > +                             dest = (u8 *)sg->esd;
> > +                             start = (u8 *)sg->esd + sg->esl->signature_size;
> > +                     } else {
> > +                             /* delete entire signature list */
> > +                             dest = (u8 *)sg->esl;
> > +                             start = (u8 *)sg->esl + sg->esl->signature_list_size;
> > +                             size -= sg->esl->signature_list_size;
> > +                     }
> > +                     memmove(dest, start, (end - start));
> > +             }
> > +     }
> > +
> > +     *db_size = size;
> > +}
> > +
> > +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;
> > +     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;
> > +}
> > +
> >   static efi_status_t prepare_signature_db_list(struct eficonfig_item **output, void *varname,
> >                                             void *db, efi_uintn_t db_size,
> >                                             eficonfig_entry_func func,
> > @@ -378,6 +510,68 @@ out:
> >       return ret;
> >   }
> >
> > +static efi_status_t process_delete_key(void *varname)
> > +{
> > +     u32 attr, i, count = 0;
> > +     efi_status_t ret;
> > +     struct eficonfig_item *menu_item = NULL, *iter;
> > +     void *db = NULL, *new_db = NULL;
> > +     efi_uintn_t db_size;
> > +     struct list_head siglist_list;
> > +     struct eficonfig_sig_data *selected;
> > +
> > +     db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
> > +     if (!db) {
> > +             eficonfig_print_msg("There is no entry in the signature database.");
>
> Please, use the terms of the UEFI specification.
> %s/signature database/signature store/

As Takahiro already mentioned, UEFI specifications use the following term.
 - signature database
 - signature store
 - signature list

The UEFI specification has section "32.4.1 Signature Database" and
I think "signature database" is most common in the spec.

>
> > +             return EFI_NOT_FOUND;
> > +     }
> > +
> > +     ret = prepare_signature_db_list(&menu_item, varname, db, db_size,
> > +                                     eficonfig_process_sigdata_delete, &selected,
> > +                                     &siglist_list, &count);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = eficonfig_process_common(menu_item, count, " ** Delete Key **");
> > +
> > +     if (ret == EFI_SUCCESS) {
> > +             delete_selected_signature_data(db, &db_size, selected, &siglist_list);
> > +
> > +             ret = create_time_based_payload(db, &new_db, &db_size);
> > +             if (ret != EFI_SUCCESS)
> > +                     goto out;
> > +
> > +             attr = EFI_VARIABLE_NON_VOLATILE |
> > +                    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                    EFI_VARIABLE_RUNTIME_ACCESS |
> > +                    EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
> > +             ret = efi_set_variable_int((u16 *)varname, efi_auth_var_get_guid((u16 *)varname),
> > +                                        attr, db_size, new_db, false);
> > +             if (ret != EFI_SUCCESS) {
> > +                     eficonfig_print_msg("ERROR! Fail to delete signature database");
>
> %s/Fail/Failed/

OK.

>
> Please, use the terms of the UEFI specification.
> %s/signature database/signature store/

Same as above.

>
> > +                     goto out;
> > +             }
> > +     }
> > +
> > +out:
> > +     if (menu_item) {
> > +             iter = menu_item;
> > +             for (i = 0; i < count - 1; iter++, i++) {
> > +                     free(iter->title);
> > +                     free(iter->data);
> > +             }
> > +     }
> > +
> > +     free(menu_item);
> > +     free(db);
> > +     free(new_db);
> > +
> > +     /* to stay the parent menu */
> > +     ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> > +
> > +     return ret;
> > +}
> > +
> >   static efi_status_t eficonfig_process_show_signature_db(void *data)
> >   {
> >       efi_status_t ret;
> > @@ -394,6 +588,27 @@ static efi_status_t eficonfig_process_show_signature_db(void *data)
> >       return ret;
> >   }
> >
> > +static efi_status_t eficonfig_process_delete_key(void *data)
> > +{
> > +     efi_status_t ret;
> > +
> > +     if (!is_setupmode()) {
> > +             eficonfig_print_msg("Not in the SetupMode, can not delete.");
>
> Both in Setup Mode and in Audit Mode you should be able to edit keys.

Yes, I will update the code and message.

>
> > +             return EFI_SUCCESS;
> > +     }
> > +
> > +     while (1) {
> > +             ret = process_delete_key(data);
> > +             if (ret != EFI_SUCCESS)
> > +                     break;
> > +     }
> > +
> > +     /* 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},
> >       {"Show Signature Database", eficonfig_process_show_signature_db},
>
> %s/Signature Database/signature store/

Same as above.

>
> > @@ -403,6 +618,7 @@ static struct eficonfig_item key_config_pk_menu_items[] = {
> >   static struct eficonfig_item key_config_menu_items[] = {
> >       {"Enroll New Key", eficonfig_process_enroll_key},
> >       {"Show Signature Database", eficonfig_process_show_signature_db},
>
> %s/Signature Database/signature store/

Same as above.

Thank you for your review.

Regards,
Masahias Kojima

>
> Best regards
>
> Heinrich
>
> > +     {"Delete Key", eficonfig_process_delete_key},
> >       {"Quit", eficonfig_process_quit},
> >   };
> >
>

Re: [RFC PATCH 3/3] eficonfig: add "Delete Key" menu entry

[Heinrich Schuchardt]

On 7/12/22 09:13, Masahisa Kojima wrote:
> Hi Heinrich, Takahiro,
>
> On Sun, 10 Jul 2022 at 19:10, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 6/19/22 07:20, Masahisa Kojima wrote:
>>> This commit add the menu-driven interface to delete the
>>> signature database entry.
>>> EFI Signature Lists can contain the multiple signature
>>> entries, this menu can delete the indivisual entry.
>>>
>>> If the PK is enrolled and UEFI Secure Boot is in User Mode,
>>
>> Why don't you mention Deployed Mode?
>
> Yes, I will mention DeployedMode.
>
>>
>>> user can not delete the existing signature lists since the
>>> signature lists must be signed by KEK or PK but signing
>>> information is not stored in the signature database.
>>
>> Updating PK with an empty value must be possible if if the new value is
>> signed with the old PK.
>
> Yes, I will add this description.
>
>>
>>>
>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>> ---
>>>    cmd/eficonfig_sbkey.c | 218 +++++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 217 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
>>> index 02ab8f8218..142bb4cef5 100644
>>> --- a/cmd/eficonfig_sbkey.c
>>> +++ b/cmd/eficonfig_sbkey.c
>>> @@ -54,6 +54,29 @@ static const struct eficonfig_sigtype_to_str sigtype_to_str[] = {
>>>    /*  {EFI_CERT_SHA512_GUID,          "SHA512",               SIG_TYPE_HASH}, */
>>>    };
>>>
>>
>> Please, add documentation to all functions.
>
> OK.
>
>>
>>> +static int eficonfig_console_yes_no(void)
>>> +{
>>> +     int esc = 0;
>>> +     enum bootmenu_key key = KEY_NONE;
>>> +
>>> +     while (1) {
>>> +             bootmenu_loop(NULL, &key, &esc);
>>> +
>>> +             switch (key) {
>>> +             case KEY_SELECT:
>>> +                     return 1;
>>> +             case KEY_QUIT:
>>> +                     return 0;
>>> +             default:
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     /* never happens */
>>> +     debug("eficonfig: this should not happen");
>>> +     return 0;
>>
>> If this code is unreachable, remove it.
>
> OK.
>
>>
>>> +}
>>> +
>>>    static void eficonfig_console_wait_enter(void)
>>>    {
>>>        int esc = 0;
>>> @@ -72,7 +95,19 @@ static void eficonfig_console_wait_enter(void)
>>>
>>>        /* never happens */
>>>        debug("eficonfig: this should not happen");
>>> -     return;
>>
>> Please remove unreachable code.
>
> OK.
>
>>
>>> +}
>>> +
>>> +static bool is_setupmode(void)
>>> +{
>>> +     efi_status_t ret;
>>> +     u8 setup_mode;
>>> +     efi_uintn_t size;
>>> +
>>> +     size = sizeof(setup_mode);
>>> +     ret = efi_get_variable_int(u"SetupMode", &efi_global_variable_guid,
>>> +                                NULL, &size, &setup_mode, NULL);
>>> +
>>> +     return setup_mode == 1;
>>>    }
>>>
>>>    static bool is_secureboot_enabled(void)
>>> @@ -254,6 +289,103 @@ static efi_status_t eficonfig_process_sigdata_show(void *data)
>>>        return EFI_SUCCESS;
>>>    }
>>>
>>> +static efi_status_t eficonfig_process_sigdata_delete(void *data)
>>> +{
>>> +     int yes_no;
>>> +     struct eficonfig_sig_data *sg = data;
>>> +
>>> +     display_sigdata_header(sg, "Delete");
>>> +     display_sigdata_info(sg);
>>> +
>>> +     printf("\n\n  Press ENTER to delete, ESC/CTRL+C to quit");
>>> +     yes_no = eficonfig_console_yes_no();
>>> +     if (!yes_no)
>>> +             return EFI_NOT_READY;
>>> +
>>> +     return EFI_SUCCESS;
>>> +}
>>> +
>>> +static void delete_selected_signature_data(void *db, efi_uintn_t *db_size,
>>> +                                        struct eficonfig_sig_data *target,
>>> +                                        struct list_head *siglist_list)
>>> +{
>>> +     u8 *dest, *start;
>>> +     struct list_head *pos, *n;
>>> +     u32 remain;
>>> +     u32 size = *db_size;
>>> +     u8 *end = (u8 *)db + size;
>>> +     struct eficonfig_sig_data *sg;
>>> +
>>> +     list_for_each_safe(pos, n, siglist_list) {
>>> +             sg = list_entry(pos, struct eficonfig_sig_data, list);
>>> +             if (sg->esl == target->esl && sg->esd == target->esd) {
>>> +                     remain = sg->esl->signature_list_size -
>>> +                              (sizeof(struct efi_signature_list) -
>>> +                              sg->esl->signature_header_size) -
>>> +                              sg->esl->signature_size;
>>> +                     if (remain > 0) {
>>> +                             /* only delete the single signature data */
>>> +                             sg->esl->signature_list_size -= sg->esl->signature_size;
>>> +                             size -= sg->esl->signature_size;
>>> +                             dest = (u8 *)sg->esd;
>>> +                             start = (u8 *)sg->esd + sg->esl->signature_size;
>>> +                     } else {
>>> +                             /* delete entire signature list */
>>> +                             dest = (u8 *)sg->esl;
>>> +                             start = (u8 *)sg->esl + sg->esl->signature_list_size;
>>> +                             size -= sg->esl->signature_list_size;
>>> +                     }
>>> +                     memmove(dest, start, (end - start));
>>> +             }
>>> +     }
>>> +
>>> +     *db_size = size;
>>> +}
>>> +
>>> +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;
>>> +     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;
>>> +}
>>> +
>>>    static efi_status_t prepare_signature_db_list(struct eficonfig_item **output, void *varname,
>>>                                              void *db, efi_uintn_t db_size,
>>>                                              eficonfig_entry_func func,
>>> @@ -378,6 +510,68 @@ out:
>>>        return ret;
>>>    }
>>>
>>> +static efi_status_t process_delete_key(void *varname)
>>> +{
>>> +     u32 attr, i, count = 0;
>>> +     efi_status_t ret;
>>> +     struct eficonfig_item *menu_item = NULL, *iter;
>>> +     void *db = NULL, *new_db = NULL;
>>> +     efi_uintn_t db_size;
>>> +     struct list_head siglist_list;
>>> +     struct eficonfig_sig_data *selected;
>>> +
>>> +     db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
>>> +     if (!db) {
>>> +             eficonfig_print_msg("There is no entry in the signature database.");
>>
>> Please, use the terms of the UEFI specification.
>> %s/signature database/signature store/
>
> As Takahiro already mentioned, UEFI specifications use the following term.
>   - signature database

Yes.

The term signature database is used in references to db, dbx, dbr, dbt.

>   - signature store

Signature store is only used in the description of dbDefault,
dbrDefault, dbtDefault, dbxDefault.

>   - signature list

Signature list refers to one of the esl files concatenated in a
signature store.

Best regards

Heinrich

>
> The UEFI specification has section "32.4.1 Signature Database" and
> I think "signature database" is most common in the spec.
>
>>
>>> +             return EFI_NOT_FOUND;
>>> +     }
>>> +
>>> +     ret = prepare_signature_db_list(&menu_item, varname, db, db_size,
>>> +                                     eficonfig_process_sigdata_delete, &selected,
>>> +                                     &siglist_list, &count);
>>> +     if (ret != EFI_SUCCESS)
>>> +             goto out;
>>> +
>>> +     ret = eficonfig_process_common(menu_item, count, " ** Delete Key **");
>>> +
>>> +     if (ret == EFI_SUCCESS) {
>>> +             delete_selected_signature_data(db, &db_size, selected, &siglist_list);
>>> +
>>> +             ret = create_time_based_payload(db, &new_db, &db_size);
>>> +             if (ret != EFI_SUCCESS)
>>> +                     goto out;
>>> +
>>> +             attr = EFI_VARIABLE_NON_VOLATILE |
>>> +                    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> +                    EFI_VARIABLE_RUNTIME_ACCESS |
>>> +                    EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
>>> +             ret = efi_set_variable_int((u16 *)varname, efi_auth_var_get_guid((u16 *)varname),
>>> +                                        attr, db_size, new_db, false);
>>> +             if (ret != EFI_SUCCESS) {
>>> +                     eficonfig_print_msg("ERROR! Fail to delete signature database");
>>
>> %s/Fail/Failed/
>
> OK.
>
>>
>> Please, use the terms of the UEFI specification.
>> %s/signature database/signature store/
>
> Same as above.
>
>>
>>> +                     goto out;
>>> +             }
>>> +     }
>>> +
>>> +out:
>>> +     if (menu_item) {
>>> +             iter = menu_item;
>>> +             for (i = 0; i < count - 1; iter++, i++) {
>>> +                     free(iter->title);
>>> +                     free(iter->data);
>>> +             }
>>> +     }
>>> +
>>> +     free(menu_item);
>>> +     free(db);
>>> +     free(new_db);
>>> +
>>> +     /* to stay the parent menu */
>>> +     ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
>>> +
>>> +     return ret;
>>> +}
>>> +
>>>    static efi_status_t eficonfig_process_show_signature_db(void *data)
>>>    {
>>>        efi_status_t ret;
>>> @@ -394,6 +588,27 @@ static efi_status_t eficonfig_process_show_signature_db(void *data)
>>>        return ret;
>>>    }
>>>
>>> +static efi_status_t eficonfig_process_delete_key(void *data)
>>> +{
>>> +     efi_status_t ret;
>>> +
>>> +     if (!is_setupmode()) {
>>> +             eficonfig_print_msg("Not in the SetupMode, can not delete.");
>>
>> Both in Setup Mode and in Audit Mode you should be able to edit keys.
>
> Yes, I will update the code and message.
>
>>
>>> +             return EFI_SUCCESS;
>>> +     }
>>> +
>>> +     while (1) {
>>> +             ret = process_delete_key(data);
>>> +             if (ret != EFI_SUCCESS)
>>> +                     break;
>>> +     }
>>> +
>>> +     /* 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},
>>>        {"Show Signature Database", eficonfig_process_show_signature_db},
>>
>> %s/Signature Database/signature store/
>
> Same as above.
>
>>
>>> @@ -403,6 +618,7 @@ static struct eficonfig_item key_config_pk_menu_items[] = {
>>>    static struct eficonfig_item key_config_menu_items[] = {
>>>        {"Enroll New Key", eficonfig_process_enroll_key},
>>>        {"Show Signature Database", eficonfig_process_show_signature_db},
>>
>> %s/Signature Database/signature store/
>
> Same as above.
>
> Thank you for your review.
>
> Regards,
> Masahias Kojima
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> +     {"Delete Key", eficonfig_process_delete_key},
>>>        {"Quit", eficonfig_process_quit},
>>>    };
>>>
>>

Re: [RFC PATCH 3/3] eficonfig: add "Delete Key" menu entry

[Masahisa Kojima]

On Tue, 12 Jul 2022 at 17:02, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 7/12/22 09:13, Masahisa Kojima wrote:
> > Hi Heinrich, Takahiro,
> >
> > On Sun, 10 Jul 2022 at 19:10, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 6/19/22 07:20, Masahisa Kojima wrote:
> >>> This commit add the menu-driven interface to delete the
> >>> signature database entry.
> >>> EFI Signature Lists can contain the multiple signature
> >>> entries, this menu can delete the indivisual entry.
> >>>
> >>> If the PK is enrolled and UEFI Secure Boot is in User Mode,
> >>
> >> Why don't you mention Deployed Mode?
> >
> > Yes, I will mention DeployedMode.
> >
> >>
> >>> user can not delete the existing signature lists since the
> >>> signature lists must be signed by KEK or PK but signing
> >>> information is not stored in the signature database.
> >>
> >> Updating PK with an empty value must be possible if if the new value is
> >> signed with the old PK.
> >
> > Yes, I will add this description.
> >
> >>
> >>>
> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>> ---
> >>>    cmd/eficonfig_sbkey.c | 218 +++++++++++++++++++++++++++++++++++++++++-
> >>>    1 file changed, 217 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> >>> index 02ab8f8218..142bb4cef5 100644
> >>> --- a/cmd/eficonfig_sbkey.c
> >>> +++ b/cmd/eficonfig_sbkey.c
> >>> @@ -54,6 +54,29 @@ static const struct eficonfig_sigtype_to_str sigtype_to_str[] = {
> >>>    /*  {EFI_CERT_SHA512_GUID,          "SHA512",               SIG_TYPE_HASH}, */
> >>>    };
> >>>
> >>
> >> Please, add documentation to all functions.
> >
> > OK.
> >
> >>
> >>> +static int eficonfig_console_yes_no(void)
> >>> +{
> >>> +     int esc = 0;
> >>> +     enum bootmenu_key key = KEY_NONE;
> >>> +
> >>> +     while (1) {
> >>> +             bootmenu_loop(NULL, &key, &esc);
> >>> +
> >>> +             switch (key) {
> >>> +             case KEY_SELECT:
> >>> +                     return 1;
> >>> +             case KEY_QUIT:
> >>> +                     return 0;
> >>> +             default:
> >>> +                     break;
> >>> +             }
> >>> +     }
> >>> +
> >>> +     /* never happens */
> >>> +     debug("eficonfig: this should not happen");
> >>> +     return 0;
> >>
> >> If this code is unreachable, remove it.
> >
> > OK.
> >
> >>
> >>> +}
> >>> +
> >>>    static void eficonfig_console_wait_enter(void)
> >>>    {
> >>>        int esc = 0;
> >>> @@ -72,7 +95,19 @@ static void eficonfig_console_wait_enter(void)
> >>>
> >>>        /* never happens */
> >>>        debug("eficonfig: this should not happen");
> >>> -     return;
> >>
> >> Please remove unreachable code.
> >
> > OK.
> >
> >>
> >>> +}
> >>> +
> >>> +static bool is_setupmode(void)
> >>> +{
> >>> +     efi_status_t ret;
> >>> +     u8 setup_mode;
> >>> +     efi_uintn_t size;
> >>> +
> >>> +     size = sizeof(setup_mode);
> >>> +     ret = efi_get_variable_int(u"SetupMode", &efi_global_variable_guid,
> >>> +                                NULL, &size, &setup_mode, NULL);
> >>> +
> >>> +     return setup_mode == 1;
> >>>    }
> >>>
> >>>    static bool is_secureboot_enabled(void)
> >>> @@ -254,6 +289,103 @@ static efi_status_t eficonfig_process_sigdata_show(void *data)
> >>>        return EFI_SUCCESS;
> >>>    }
> >>>
> >>> +static efi_status_t eficonfig_process_sigdata_delete(void *data)
> >>> +{
> >>> +     int yes_no;
> >>> +     struct eficonfig_sig_data *sg = data;
> >>> +
> >>> +     display_sigdata_header(sg, "Delete");
> >>> +     display_sigdata_info(sg);
> >>> +
> >>> +     printf("\n\n  Press ENTER to delete, ESC/CTRL+C to quit");
> >>> +     yes_no = eficonfig_console_yes_no();
> >>> +     if (!yes_no)
> >>> +             return EFI_NOT_READY;
> >>> +
> >>> +     return EFI_SUCCESS;
> >>> +}
> >>> +
> >>> +static void delete_selected_signature_data(void *db, efi_uintn_t *db_size,
> >>> +                                        struct eficonfig_sig_data *target,
> >>> +                                        struct list_head *siglist_list)
> >>> +{
> >>> +     u8 *dest, *start;
> >>> +     struct list_head *pos, *n;
> >>> +     u32 remain;
> >>> +     u32 size = *db_size;
> >>> +     u8 *end = (u8 *)db + size;
> >>> +     struct eficonfig_sig_data *sg;
> >>> +
> >>> +     list_for_each_safe(pos, n, siglist_list) {
> >>> +             sg = list_entry(pos, struct eficonfig_sig_data, list);
> >>> +             if (sg->esl == target->esl && sg->esd == target->esd) {
> >>> +                     remain = sg->esl->signature_list_size -
> >>> +                              (sizeof(struct efi_signature_list) -
> >>> +                              sg->esl->signature_header_size) -
> >>> +                              sg->esl->signature_size;
> >>> +                     if (remain > 0) {
> >>> +                             /* only delete the single signature data */
> >>> +                             sg->esl->signature_list_size -= sg->esl->signature_size;
> >>> +                             size -= sg->esl->signature_size;
> >>> +                             dest = (u8 *)sg->esd;
> >>> +                             start = (u8 *)sg->esd + sg->esl->signature_size;
> >>> +                     } else {
> >>> +                             /* delete entire signature list */
> >>> +                             dest = (u8 *)sg->esl;
> >>> +                             start = (u8 *)sg->esl + sg->esl->signature_list_size;
> >>> +                             size -= sg->esl->signature_list_size;
> >>> +                     }
> >>> +                     memmove(dest, start, (end - start));
> >>> +             }
> >>> +     }
> >>> +
> >>> +     *db_size = size;
> >>> +}
> >>> +
> >>> +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;
> >>> +     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;
> >>> +}
> >>> +
> >>>    static efi_status_t prepare_signature_db_list(struct eficonfig_item **output, void *varname,
> >>>                                              void *db, efi_uintn_t db_size,
> >>>                                              eficonfig_entry_func func,
> >>> @@ -378,6 +510,68 @@ out:
> >>>        return ret;
> >>>    }
> >>>
> >>> +static efi_status_t process_delete_key(void *varname)
> >>> +{
> >>> +     u32 attr, i, count = 0;
> >>> +     efi_status_t ret;
> >>> +     struct eficonfig_item *menu_item = NULL, *iter;
> >>> +     void *db = NULL, *new_db = NULL;
> >>> +     efi_uintn_t db_size;
> >>> +     struct list_head siglist_list;
> >>> +     struct eficonfig_sig_data *selected;
> >>> +
> >>> +     db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
> >>> +     if (!db) {
> >>> +             eficonfig_print_msg("There is no entry in the signature database.");
> >>
> >> Please, use the terms of the UEFI specification.
> >> %s/signature database/signature store/
> >
> > As Takahiro already mentioned, UEFI specifications use the following term.
> >   - signature database
>
> Yes.
>
> The term signature database is used in references to db, dbx, dbr, dbt.
>
> >   - signature store
>
> Signature store is only used in the description of dbDefault,
> dbrDefault, dbtDefault, dbxDefault.
>
> >   - signature list
>
> Signature list refers to one of the esl files concatenated in a
> signature store.

Thank you for adding the information.
The KEK variable description in UEFI specification is:
"The Key Exchange Key Signature Database."

So I would like to use "signature database" in this patch series.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > The UEFI specification has section "32.4.1 Signature Database" and
> > I think "signature database" is most common in the spec.
> >
> >>
> >>> +             return EFI_NOT_FOUND;
> >>> +     }
> >>> +
> >>> +     ret = prepare_signature_db_list(&menu_item, varname, db, db_size,
> >>> +                                     eficonfig_process_sigdata_delete, &selected,
> >>> +                                     &siglist_list, &count);
> >>> +     if (ret != EFI_SUCCESS)
> >>> +             goto out;
> >>> +
> >>> +     ret = eficonfig_process_common(menu_item, count, " ** Delete Key **");
> >>> +
> >>> +     if (ret == EFI_SUCCESS) {
> >>> +             delete_selected_signature_data(db, &db_size, selected, &siglist_list);
> >>> +
> >>> +             ret = create_time_based_payload(db, &new_db, &db_size);
> >>> +             if (ret != EFI_SUCCESS)
> >>> +                     goto out;
> >>> +
> >>> +             attr = EFI_VARIABLE_NON_VOLATILE |
> >>> +                    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> +                    EFI_VARIABLE_RUNTIME_ACCESS |
> >>> +                    EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
> >>> +             ret = efi_set_variable_int((u16 *)varname, efi_auth_var_get_guid((u16 *)varname),
> >>> +                                        attr, db_size, new_db, false);
> >>> +             if (ret != EFI_SUCCESS) {
> >>> +                     eficonfig_print_msg("ERROR! Fail to delete signature database");
> >>
> >> %s/Fail/Failed/
> >
> > OK.
> >
> >>
> >> Please, use the terms of the UEFI specification.
> >> %s/signature database/signature store/
> >
> > Same as above.
> >
> >>
> >>> +                     goto out;
> >>> +             }
> >>> +     }
> >>> +
> >>> +out:
> >>> +     if (menu_item) {
> >>> +             iter = menu_item;
> >>> +             for (i = 0; i < count - 1; iter++, i++) {
> >>> +                     free(iter->title);
> >>> +                     free(iter->data);
> >>> +             }
> >>> +     }
> >>> +
> >>> +     free(menu_item);
> >>> +     free(db);
> >>> +     free(new_db);
> >>> +
> >>> +     /* to stay the parent menu */
> >>> +     ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>>    static efi_status_t eficonfig_process_show_signature_db(void *data)
> >>>    {
> >>>        efi_status_t ret;
> >>> @@ -394,6 +588,27 @@ static efi_status_t eficonfig_process_show_signature_db(void *data)
> >>>        return ret;
> >>>    }
> >>>
> >>> +static efi_status_t eficonfig_process_delete_key(void *data)
> >>> +{
> >>> +     efi_status_t ret;
> >>> +
> >>> +     if (!is_setupmode()) {
> >>> +             eficonfig_print_msg("Not in the SetupMode, can not delete.");
> >>
> >> Both in Setup Mode and in Audit Mode you should be able to edit keys.
> >
> > Yes, I will update the code and message.
> >
> >>
> >>> +             return EFI_SUCCESS;
> >>> +     }
> >>> +
> >>> +     while (1) {
> >>> +             ret = process_delete_key(data);
> >>> +             if (ret != EFI_SUCCESS)
> >>> +                     break;
> >>> +     }
> >>> +
> >>> +     /* 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},
> >>>        {"Show Signature Database", eficonfig_process_show_signature_db},
> >>
> >> %s/Signature Database/signature store/
> >
> > Same as above.
> >
> >>
> >>> @@ -403,6 +618,7 @@ static struct eficonfig_item key_config_pk_menu_items[] = {
> >>>    static struct eficonfig_item key_config_menu_items[] = {
> >>>        {"Enroll New Key", eficonfig_process_enroll_key},
> >>>        {"Show Signature Database", eficonfig_process_show_signature_db},
> >>
> >> %s/Signature Database/signature store/
> >
> > Same as above.
> >
> > Thank you for your review.
> >
> > Regards,
> > Masahias Kojima
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +     {"Delete Key", eficonfig_process_delete_key},
> >>>        {"Quit", eficonfig_process_quit},
> >>>    };
> >>>
> >>
>

Re: [RFC PATCH 0/3] eficonfig: add UEFI Secure Boot key maintenance interface

[Ilias Apalodimas]

On Sun, Jun 19, 2022 at 02:20:19PM +0900, Masahisa Kojima wrote:
> This series adds the UEFI Secure Boot key maintenance interface
> to the eficonfig command.
> User can enroll and delete the PK, KEK, db and dbx.
> 
> Note that this series is RFC since this series is implemented
> on the top of the "enable menu-driven UEFI variable maintenance"
> patch series still under review[1].
> 
> [1]https://lore.kernel.org/u-boot/20220619045607.1669-1-masahisa.kojima@linaro.org/T/#m7fe16b6044fbb2947b49c26051563c7cbb696fb3
> 
> Source code can be cloned with:
> $ git clone https://git.linaro.org/people/masahisa.kojima/u-boot.git -b kojima/kojima/efi_seckey_menu_upstream_v1_0619

Thanks Kojima-san.  This is an important step in removing console access
for EFI-enabled devices.

Regards
/Ilias
> 
> Masahisa Kojima (3):
>   eficonfig: add UEFI Secure Boot Key enrollment interface
>   eficonfig: add "Show Signature Database" menu entry
>   eficonfig: add "Delete Key" menu entry
> 
>  cmd/Makefile          |   3 +
>  cmd/eficonfig.c       |   3 +
>  cmd/eficonfig_sbkey.c | 701 ++++++++++++++++++++++++++++++++++++++++++
>  include/efi_config.h  |   3 +
>  4 files changed, 710 insertions(+)
>  create mode 100644 cmd/eficonfig_sbkey.c
> 
> -- 
> 2.17.1
>