[PATCH v3 0/6] eficonfig: add UEFI Secure Boot key maintenance interface

[PATCH v3 0/6] 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.

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

[Major Changes]
- rebased on top of u-boot/master

Masahisa Kojima (6):
  eficonfig: refactor eficonfig_select_file_handler()
  eficonfig: expose append entry function
  eficonfig: add UEFI Secure Boot Key enrollment interface
  eficonfig: add "Show/Delete Signature Database" menu entry
  test/eficonfig: support secure boot key maintenance menu
  test: add test for eficonfig secure boot key management

 cmd/Makefile                                  |   3 +
 cmd/eficonfig.c                               |  48 +-
 cmd/eficonfig_sbkey.c                         | 751 ++++++++++++++++++
 include/efi_config.h                          |  10 +
 test/py/tests/test_eficonfig/conftest.py      |  84 +-
 test/py/tests/test_eficonfig/defs.py          |  14 +
 .../py/tests/test_eficonfig/test_eficonfig.py |   4 +-
 .../test_eficonfig/test_eficonfig_sbkey.py    | 472 +++++++++++
 8 files changed, 1360 insertions(+), 26 deletions(-)
 create mode 100644 cmd/eficonfig_sbkey.c
 create mode 100644 test/py/tests/test_eficonfig/defs.py
 create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py

-- 
2.17.1

[PATCH v3 1/6] eficonfig: refactor eficonfig_select_file_handler()

[Masahisa Kojima]

eficonfig_select_file_handler() is commonly used to select the
file. eficonfig_display_select_file_option() intends to add the
additional menu mainly to clear the selected file information.
eficonfig_display_select_file_option() is not necessary for the
file selection process, so it should be outside of
eficonfig_select_file_handler().

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No change since v2

newly created in v2

 cmd/eficonfig.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 2595dd9563..f6a99bd01a 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -968,7 +968,7 @@ efi_status_t eficonfig_process_clear_file_selection(void *data)
 }
 
 static struct eficonfig_item select_file_menu_items[] = {
-	{"Select File", eficonfig_process_select_file},
+	{"Select File", eficonfig_select_file_handler},
 	{"Clear", eficonfig_process_clear_file_selection},
 	{"Quit", eficonfig_process_quit},
 };
@@ -980,12 +980,13 @@ static struct eficonfig_item select_file_menu_items[] = {
  * @file_info:	pointer to the file information structure
  * Return:	status code
  */
-efi_status_t eficonfig_display_select_file_option(struct eficonfig_select_file_info *file_info)
+efi_status_t eficonfig_display_select_file_option(void *data)
 {
 	efi_status_t ret;
 	struct efimenu *efi_menu;
 
-	select_file_menu_items[1].data = file_info;
+	select_file_menu_items[0].data = data;
+	select_file_menu_items[1].data = data;
 	efi_menu = eficonfig_create_fixed_menu(select_file_menu_items,
 					       ARRAY_SIZE(select_file_menu_items));
 	if (!efi_menu)
@@ -1016,10 +1017,6 @@ efi_status_t eficonfig_select_file_handler(void *data)
 	struct eficonfig_select_file_info *tmp = NULL;
 	struct eficonfig_select_file_info *file_info = data;
 
-	ret = eficonfig_display_select_file_option(file_info);
-	if (ret != EFI_SUCCESS)
-		return ret;
-
 	tmp = calloc(1, sizeof(struct eficonfig_select_file_info));
 	if (!tmp)
 		return EFI_OUT_OF_RESOURCES;
@@ -1284,7 +1281,7 @@ static efi_status_t prepare_file_selection_entry(struct efimenu *efi_menu, char
 	utf8_utf16_strcpy(&p, devname);
 	u16_strlcat(file_name, file_info->current_path, len);
 	ret = create_boot_option_entry(efi_menu, title, file_name,
-				       eficonfig_select_file_handler, file_info);
+				       eficonfig_display_select_file_option, file_info);
 out:
 	free(devname);
 	free(file_name);
-- 
2.17.1

[PATCH v3 2/6] eficonfig: expose append entry function

[Masahisa Kojima]

This commit exposes the eficonfig menu entry append function.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No change since v2

newly created in v2

 cmd/eficonfig.c      | 32 +++++++++++++++++---------------
 include/efi_config.h |  5 +++++
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index f6a99bd01a..0cb0770ac3 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -263,7 +263,7 @@ efi_status_t eficonfig_process_quit(void *data)
 }
 
 /**
- * append_entry() - append menu item
+ * eficonfig_append_menu_entry() - append menu item
  *
  * @efi_menu:	pointer to the efimenu structure
  * @title:	pointer to the entry title
@@ -271,8 +271,9 @@ efi_status_t eficonfig_process_quit(void *data)
  * @data:	pointer to the data to be passed to each entry callback
  * Return:	status code
  */
-static efi_status_t append_entry(struct efimenu *efi_menu,
-				 char *title, eficonfig_entry_func func, void *data)
+efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
+					 char *title, eficonfig_entry_func func,
+					 void *data)
 {
 	struct eficonfig_entry *entry;
 
@@ -295,12 +296,12 @@ static efi_status_t append_entry(struct efimenu *efi_menu,
 }
 
 /**
- * append_quit_entry() - append quit entry
+ * eficonfig_append_quit_entry() - append quit entry
  *
  * @efi_menu:	pointer to the efimenu structure
  * Return:	status code
  */
-static efi_status_t append_quit_entry(struct efimenu *efi_menu)
+efi_status_t eficonfig_append_quit_entry(struct efimenu *efi_menu)
 {
 	char *title;
 	efi_status_t ret;
@@ -309,7 +310,7 @@ static efi_status_t append_quit_entry(struct efimenu *efi_menu)
 	if (!title)
 		return EFI_OUT_OF_RESOURCES;
 
-	ret = append_entry(efi_menu, title, eficonfig_process_quit, NULL);
+	ret = eficonfig_append_menu_entry(efi_menu, title, eficonfig_process_quit, NULL);
 	if (ret != EFI_SUCCESS)
 		free(title);
 
@@ -341,7 +342,7 @@ void *eficonfig_create_fixed_menu(const struct eficonfig_item *items, int count)
 		if (!title)
 			goto out;
 
-		ret = append_entry(efi_menu, title, iter->func, iter->data);
+		ret = eficonfig_append_menu_entry(efi_menu, title, iter->func, iter->data);
 		if (ret != EFI_SUCCESS) {
 			free(title);
 			goto out;
@@ -634,14 +635,15 @@ static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *f
 		info->v = v;
 		info->dp = device_path;
 		info->file_info = file_info;
-		ret = append_entry(efi_menu, devname, eficonfig_volume_selected, info);
+		ret = eficonfig_append_menu_entry(efi_menu, devname, eficonfig_volume_selected,
+						  info);
 		if (ret != EFI_SUCCESS) {
 			free(info);
 			goto out;
 		}
 	}
 
-	ret = append_quit_entry(efi_menu);
+	ret = eficonfig_append_quit_entry(efi_menu);
 	if (ret != EFI_SUCCESS)
 		goto out;
 
@@ -745,8 +747,8 @@ eficonfig_create_file_entry(struct efimenu *efi_menu, u32 count,
 	      (int (*)(const void *, const void *))sort_file);
 
 	for (i = 0; i < entry_num; i++) {
-		ret = append_entry(efi_menu, tmp_infos[i]->file_name,
-				   eficonfig_file_selected, tmp_infos[i]);
+		ret = eficonfig_append_menu_entry(efi_menu, tmp_infos[i]->file_name,
+						  eficonfig_file_selected, tmp_infos[i]);
 		if (ret != EFI_SUCCESS)
 			goto out;
 	}
@@ -815,7 +817,7 @@ static efi_status_t eficonfig_select_file(struct eficonfig_select_file_info *fil
 		if (ret != EFI_SUCCESS)
 			goto err;
 
-		ret = append_quit_entry(efi_menu);
+		ret = eficonfig_append_quit_entry(efi_menu);
 		if (ret != EFI_SUCCESS)
 			goto err;
 
@@ -1218,7 +1220,7 @@ static efi_status_t create_boot_option_entry(struct efimenu *efi_menu, char *tit
 		utf16_utf8_strcpy(&p, val);
 	}
 
-	return append_entry(efi_menu, buf, func, data);
+	return eficonfig_append_menu_entry(efi_menu, buf, func, data);
 }
 
 /**
@@ -1677,7 +1679,7 @@ static efi_status_t eficonfig_add_boot_selection_entry(struct efimenu *efi_menu,
 	utf16_utf8_strcpy(&p, lo.label);
 	info->boot_index = boot_index;
 	info->selected = selected;
-	ret = append_entry(efi_menu, buf, eficonfig_process_boot_selected, info);
+	ret = eficonfig_append_menu_entry(efi_menu, buf, eficonfig_process_boot_selected, info);
 	if (ret != EFI_SUCCESS) {
 		free(load_option);
 		free(info);
@@ -1736,7 +1738,7 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
 			break;
 	}
 
-	ret = append_quit_entry(efi_menu);
+	ret = eficonfig_append_quit_entry(efi_menu);
 	if (ret != EFI_SUCCESS)
 		goto out;
 
diff --git a/include/efi_config.h b/include/efi_config.h
index 098cac2115..86bc801211 100644
--- a/include/efi_config.h
+++ b/include/efi_config.h
@@ -95,4 +95,9 @@ efi_status_t eficonfig_get_unused_bootoption(u16 *buf,
 efi_status_t eficonfig_append_bootorder(u16 index);
 efi_status_t eficonfig_generate_media_device_boot_option(void);
 
+efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
+					 char *title, eficonfig_entry_func func,
+					 void *data);
+efi_status_t eficonfig_append_quit_entry(struct efimenu *efi_menu);
+
 #endif
-- 
2.17.1

[PATCH v3 3/6] 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>
---
Changes in v3:
- fix error handling

Changes in v2:
- allow to enroll .esl file
- fix typos
- add function comments

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

diff --git a/cmd/Makefile b/cmd/Makefile
index c95e09d058..f2f2857146 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -66,6 +66,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 0cb0770ac3..a72f07e671 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -2442,6 +2442,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..cc27f78e66
--- /dev/null
+++ b/cmd/eficonfig_sbkey.c
@@ -0,0 +1,357 @@
+// 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>
+
+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}, */
+};
+
+/**
+ * is_secureboot_enabled() - check UEFI Secure Boot is enabled
+ *
+ * Return:	true when UEFI Secure Boot is enabled, false otherwise
+ */
+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;
+}
+
+/**
+ * create_time_based_payload() - create payload for time based authenticate variable
+ *
+ * @db:		pointer to the original signature database
+ * @new_db:	pointer to the authenticated variable payload
+ * @size:	pointer to payload size
+ * Return:	status code
+ */
+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;
+}
+
+/**
+ * file_have_auth_header() - check file has EFI_VARIABLE_AUTHENTICATION_2 header
+ * @buf:	pointer to file
+ * @size:	file size
+ * Return:	true if file has auth header, false otherwise
+ */
+static bool file_have_auth_header(void *buf, efi_uintn_t size)
+{
+	struct efi_variable_authentication_2 *auth = buf;
+
+	if (auth->auth_info.hdr.wCertificateType != WIN_CERT_TYPE_EFI_GUID)
+		return false;
+
+	if (guidcmp(&auth->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
+		return false;
+
+	return true;
+}
+
+/**
+ * file_is_efi_signature_list() - check the file is efi signature list
+ * @buf:	pointer to file
+ * Return:	true if file is efi signature list, false otherwise
+ */
+static bool file_is_efi_signature_list(void *buf)
+{
+	u32 i;
+	struct efi_signature_list *sig_list = buf;
+
+	for (i = 0; i < ARRAY_SIZE(sigtype_to_str); i++) {
+		if (!guidcmp(&sig_list->signature_type, &sigtype_to_str[i].sig_type))
+			return true;
+	}
+
+	return false;
+}
+
+/**
+ * eficonfig_process_enroll_key() - enroll key into signature database
+ *
+ * @data:	pointer to the data for each entry
+ * Return:	status code
+ */
+static efi_status_t eficonfig_process_enroll_key(void *data)
+{
+	u32 attr;
+	char *buf = NULL;
+	efi_uintn_t size;
+	efi_status_t ret;
+	void *new_db = NULL;
+	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) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+	ret = efi_file_read_int(f, &size, buf);
+	if (ret != EFI_SUCCESS) {
+		eficonfig_print_msg("ERROR! Failed to read file.");
+		goto out;
+	}
+	if (size == 0) {
+		eficonfig_print_msg("ERROR! File is empty.");
+		goto out;
+	}
+
+	/* We expect that file is EFI Signature Lists or signed EFI Signature Lists */
+	if (!file_have_auth_header(buf, size)) {
+		if (!file_is_efi_signature_list(buf)) {
+			eficonfig_print_msg("ERROR! Invalid file format.");
+			ret = EFI_INVALID_PARAMETER;
+			goto out;
+		}
+
+		ret = create_time_based_payload(buf, &new_db, &size);
+		if (ret != EFI_SUCCESS) {
+			eficonfig_print_msg("ERROR! Failed to create payload with timestamp.");
+			goto out;
+		}
+
+		free(buf);
+		buf = new_db;
+	}
+
+	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! Failed 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_menu_items[] = {
+	{"Enroll New Key", eficonfig_process_enroll_key},
+	{"Quit", eficonfig_process_quit},
+};
+
+/**
+ * eficonfig_process_set_secure_boot_key() - display the key configuration menu
+ *
+ * @data:	pointer to the data for each entry
+ * Return:	status code
+ */
+static efi_status_t eficonfig_process_set_secure_boot_key(void *data)
+{
+	u32 i;
+	efi_status_t ret;
+	char header_str[32];
+	struct efimenu *efi_menu;
+
+	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) {
+		efi_menu = eficonfig_create_fixed_menu(key_config_menu_items,
+						       ARRAY_SIZE(key_config_menu_items));
+
+		ret = eficonfig_process_common(efi_menu, header_str);
+		eficonfig_destroy(efi_menu);
+
+		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_key, 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},
+};
+
+/**
+ * eficonfig_process_secure_boot_config() - display the key list menu
+ *
+ * @data:	pointer to the data for each entry
+ * Return:	status code
+ */
+efi_status_t eficonfig_process_secure_boot_config(void *data)
+{
+	efi_status_t ret;
+	struct efimenu *efi_menu;
+
+	while (1) {
+		char header_str[64];
+
+		snprintf(header_str, sizeof(header_str),
+			 "  ** UEFI Secure Boot Key Configuration (SecureBoot : %s) **",
+			 (is_secureboot_enabled() ? "ON" : "OFF"));
+
+		efi_menu = eficonfig_create_fixed_menu(secure_boot_menu_items,
+						       ARRAY_SIZE(secure_boot_menu_items));
+		if (!efi_menu) {
+			ret = EFI_OUT_OF_RESOURCES;
+			break;
+		}
+
+		ret = eficonfig_process_common(efi_menu, header_str);
+		eficonfig_destroy(efi_menu);
+
+		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 86bc801211..6db8e123f0 100644
--- a/include/efi_config.h
+++ b/include/efi_config.h
@@ -99,5 +99,10 @@ efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
 					 char *title, eficonfig_entry_func func,
 					 void *data);
 efi_status_t eficonfig_append_quit_entry(struct efimenu *efi_menu);
+void *eficonfig_create_fixed_menu(const struct eficonfig_item *items, int count);
+
+#ifdef CONFIG_EFI_SECURE_BOOT
+efi_status_t eficonfig_process_secure_boot_config(void *data);
+#endif
 
 #endif
-- 
2.17.1

[PATCH v3 4/6] eficonfig: add "Show/Delete Signature Database" menu entry

[Masahisa Kojima]

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

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 or
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.

To delete PK, user needs to enroll the new key with an empty
value and this new key must be signed with the old PK.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No change since v2

Changes in v2:
- integrate show and delete signature database menu
- add confirmation message before delete
- add function comment

 cmd/eficonfig_sbkey.c | 394 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 394 insertions(+)

diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
index cc27f78e66..e8ba15726d 100644
--- a/cmd/eficonfig_sbkey.c
+++ b/cmd/eficonfig_sbkey.c
@@ -17,6 +17,14 @@
 #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,
@@ -46,6 +54,32 @@ static const struct eficonfig_sigtype_to_str sigtype_to_str[] = {
 /*	{EFI_CERT_SHA512_GUID,		"SHA512",		SIG_TYPE_HASH}, */
 };
 
+/**
+ * eficonfig_console_wait_enter() - wait ENTER key press
+ *
+ * Return:	1 if ENTER key is pressed, 0 if user selects to quit
+ */
+static int eficonfig_console_wait_enter(void)
+{
+	int esc = 0;
+	enum bootmenu_key key = KEY_NONE;
+
+	puts(ANSI_CURSOR_HIDE);
+
+	while (1) {
+		bootmenu_loop(NULL, &key, &esc);
+
+		switch (key) {
+		case KEY_SELECT:
+			return 1;
+		case KEY_QUIT:
+			return 0;
+		default:
+			break;
+		}
+	}
+}
+
 /**
  * is_secureboot_enabled() - check UEFI Secure Boot is enabled
  *
@@ -270,8 +304,368 @@ out:
 	return ret;
 }
 
+/**
+ * delete_selected_signature_data() - delete the signature data from signature list
+ *
+ * @db:		pointer to the signature database
+ * @db_size:	pointer to the signature database size
+ * @target:	pointer to the signature data to be deleted
+ * Return:	status code
+ */
+static void delete_selected_signature_data(void *db, efi_uintn_t *db_size,
+					   struct eficonfig_sig_data *target)
+{
+	u32 remain;
+	u8 *dest, *start, *end;
+	efi_uintn_t total_size, esd_size, size;
+	struct efi_signature_list *esl;
+	struct efi_signature_data *esd;
+
+	esl = db;
+	total_size = *db_size;
+	size = *db_size;
+	end = (u8 *)db + *db_size;
+	while (total_size > 0) {
+		esd = (struct efi_signature_data *)((u8 *)esl +
+		      sizeof(struct efi_signature_list) + esl->signature_header_size);
+		esd_size = esl->signature_list_size - sizeof(struct efi_signature_list) -
+			   esl->signature_header_size;
+		for (; esd_size > 0; esd_size -= esl->signature_size) {
+			if (esl == target->esl && esd == target->esd) {
+				remain = esl->signature_list_size -
+					 (sizeof(struct efi_signature_list) -
+					 esl->signature_header_size) -
+					 esl->signature_size;
+				if (remain > 0) {
+					/* only delete the single signature data */
+					esl->signature_list_size -= esl->signature_size;
+					size -= esl->signature_size;
+					dest = (u8 *)esd;
+					start = (u8 *)esd + esl->signature_size;
+				} else {
+					/* delete entire signature list */
+					dest = (u8 *)esl;
+					start = (u8 *)esl + esl->signature_list_size;
+					size -= esl->signature_list_size;
+				}
+				memmove(dest, start, (end - start));
+				goto out;
+			}
+			esd = (struct efi_signature_data *)((u8 *)esd + esl->signature_size);
+		}
+		total_size -= esl->signature_list_size;
+		esl = (struct efi_signature_list *)((u8 *)esl + esl->signature_list_size);
+	}
+out:
+	*db_size = size;
+}
+
+/**
+ * display_sigdata_info() - display signature data information
+ *
+ * @sg:		pointer to the internal signature data structure
+ * Return:	status code
+ */
+static void display_sigdata_info(struct eficonfig_sig_data *sg)
+{
+	u32 i;
+
+	puts(ANSI_CURSOR_HIDE);
+	puts(ANSI_CLEAR_CONSOLE);
+	printf(ANSI_CURSOR_POSITION, 1, 1);
+
+	*sg->selected = sg;
+	printf("\n  ** Show/Delete Signature Database (%ls) **\n\n"
+	       "    Owner GUID:\n"
+	       "      %pUL\n",
+	       sg->varname, sg->esd->signature_owner.b);
+
+	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;
+			}
+		}
+	}
+}
+
+/**
+ * eficonfig_process_sigdata_delete() - delete signature data
+ *
+ * @data:	pointer to the data for each entry
+ * Return:	status code
+ */
+static efi_status_t eficonfig_process_sigdata_delete(void *data)
+{
+	int delete;
+	efi_status_t ret;
+	efi_uintn_t size;
+	u8 setup_mode = 0;
+	u8 audit_mode = 0;
+
+	struct eficonfig_sig_data *sg = data;
+
+	display_sigdata_info(sg);
+
+	if (!u16_strcmp(sg->varname, u"PK")) {
+		while (tstc())
+			getchar();
+
+		printf("\n\n  Can not delete PK, Press any key to continue");
+		getchar();
+		return EFI_NOT_READY;
+	}
+
+	printf("\n\n  Press ENTER to delete, ESC/CTRL+C to quit");
+	delete = eficonfig_console_wait_enter();
+	if (!delete)
+		return EFI_NOT_READY;
+
+	size = sizeof(setup_mode);
+	ret = efi_get_variable_int(u"SetupMode", &efi_global_variable_guid,
+				   NULL, &size, &setup_mode, NULL);
+	size = sizeof(audit_mode);
+	ret = efi_get_variable_int(u"AuditMode", &efi_global_variable_guid,
+				   NULL, &size, &audit_mode, NULL);
+
+	if (!setup_mode && !audit_mode) {
+		eficonfig_print_msg("Not in the SetupMode or AuditMode, can not delete.");
+		return EFI_NOT_READY;
+	}
+
+	return EFI_SUCCESS;
+}
+
+/**
+ * prepare_signature_db_list() - create the signature data menu entry
+ *
+ * @efimenu:	pointer to the efimenu structure
+ * @varname:	pointer to the variable name
+ * @db:		pointer to the variable raw data
+ * @db_size:	variable data size
+ * @func:	callback of each entry
+ * @selected:	pointer to selected signature data
+ * Return:	status code
+ */
+static efi_status_t prepare_signature_db_list(struct efimenu *efi_menu, void *varname,
+					      void *db, efi_uintn_t db_size,
+					      eficonfig_entry_func func,
+					      struct eficonfig_sig_data **selected)
+{
+	u32 num = 0;
+	efi_uintn_t size;
+	struct eficonfig_sig_data *sg;
+	struct efi_signature_list *esl;
+	struct efi_signature_data *esd;
+	efi_status_t ret = EFI_SUCCESS;
+
+	INIT_LIST_HEAD(&efi_menu->list);
+
+	esl = db;
+	size = db_size;
+	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) {
+			char buf[40];
+			char *title;
+
+			if (num >= EFICONFIG_ENTRY_NUM_MAX - 1) {
+				ret = EFI_OUT_OF_RESOURCES;
+				goto out;
+			}
+
+			sg = calloc(1, sizeof(struct eficonfig_sig_data));
+			if (!sg) {
+				ret = EFI_OUT_OF_RESOURCES;
+				goto err;
+			}
+
+			snprintf(buf, sizeof(buf), "%pUL", &esd->signature_owner);
+			title = calloc(1, (strlen(buf) + 1));
+			if (!title) {
+				free(sg);
+				ret = EFI_OUT_OF_RESOURCES;
+				goto err;
+			}
+			strlcpy(title, buf, strlen(buf) + 1);
+
+			sg->esl = esl;
+			sg->esd = esd;
+			sg->selected = selected;
+			sg->varname = varname;
+			ret = eficonfig_append_menu_entry(efi_menu, title, func, sg);
+			if (ret != EFI_SUCCESS) {
+				free(sg);
+				free(title);
+				goto err;
+			}
+			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);
+	}
+out:
+	ret = eficonfig_append_quit_entry(efi_menu);
+err:
+	return ret;
+}
+
+/**
+ * process_show_signature_db() - display the signature data list
+ *
+ * @data:	pointer to the data for each entry
+ * Return:	status code
+ */
+static efi_status_t process_show_signature_db(void *varname)
+{
+	char buf[50];
+	efi_status_t ret;
+	efi_uintn_t db_size;
+	void *db, *new_db = NULL;
+	struct efimenu *efi_menu;
+	struct list_head *pos, *n;
+	struct eficonfig_entry *entry;
+	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;
+	}
+
+	efi_menu = calloc(1, sizeof(struct efimenu));
+	if (!efi_menu) {
+		free(db);
+		return EFI_OUT_OF_RESOURCES;
+	}
+
+	ret = prepare_signature_db_list(efi_menu, varname, db, db_size,
+					eficonfig_process_sigdata_delete, &selected);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	snprintf(buf, sizeof(buf), "  ** Show/Delete Signature Database (%ls) **",
+		 (u16 *)varname);
+	ret = eficonfig_process_common(efi_menu, buf);
+	if (ret == EFI_SUCCESS) {
+		u32 attr;
+		int delete;
+
+		printf(ANSI_CURSOR_HIDE
+		       "\n\n   Are you sure you want to delete this item?\n\n"
+		       "  Press ENTER to delete, ESC/CTRL+C to quit");
+		delete = eficonfig_console_wait_enter();
+		if (!delete)
+			goto out;
+
+		delete_selected_signature_data(db, &db_size, selected);
+
+		ret = create_time_based_payload(db, &new_db, &db_size);
+		if (ret != EFI_SUCCESS) {
+			eficonfig_print_msg("ERROR! Failed to create payload with timestamp.");
+			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! Failed to delete signature database");
+			goto out;
+		}
+	}
+out:
+	list_for_each_safe(pos, n, &efi_menu->list) {
+		entry = list_entry(pos, struct eficonfig_entry, list);
+		free(entry->data);
+	}
+	eficonfig_destroy(efi_menu);
+	free(new_db);
+	free(db);
+
+	return ret;
+}
+
+/**
+ * eficonfig_process_set_secure_boot_key() - display the key configuration menu
+ *
+ * @data:	pointer to the data for each entry
+ * Return:	status code
+ */
+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 && ret != EFI_NOT_READY)
+			break;
+	}
+
+	/* to stay the parent menu */
+	ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
+
+	return ret;
+}
+
 static struct eficonfig_item key_config_menu_items[] = {
 	{"Enroll New Key", eficonfig_process_enroll_key},
+	{"Show/Delete Signature Database", eficonfig_process_show_signature_db},
 	{"Quit", eficonfig_process_quit},
 };
 
-- 
2.17.1

[PATCH v3 5/6] test/eficonfig: support secure boot key maintenance menu

[Masahisa Kojima]

eficonfig test is get aligned with the addition of
secure boot key management menu.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No change since v2

newly created in v2

 test/py/tests/test_eficonfig/test_eficonfig.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/test/py/tests/test_eficonfig/test_eficonfig.py b/test/py/tests/test_eficonfig/test_eficonfig.py
index 99606d9c4b..f7cb031af2 100644
--- a/test/py/tests/test_eficonfig/test_eficonfig.py
+++ b/test/py/tests/test_eficonfig/test_eficonfig.py
@@ -8,6 +8,7 @@ import time
 @pytest.mark.boardspec('sandbox')
 @pytest.mark.buildconfigspec('cmd_eficonfig')
 @pytest.mark.buildconfigspec('cmd_bootefi_bootmgr')
+@pytest.mark.buildconfigspec('efi_secure_boot')
 def test_efi_eficonfig(u_boot_console, efi_eficonfig_data):
 
     def send_user_input_and_wait(user_str, expect_str):
@@ -47,7 +48,7 @@ def test_efi_eficonfig(u_boot_console, efi_eficonfig_data):
 
     def check_current_is_maintenance_menu():
         for i in ('UEFI Maintenance Menu', 'Add Boot Option', 'Edit Boot Option',
-                  'Change Boot Order', 'Delete Boot Option', 'Quit'):
+                  'Change Boot Order', 'Delete Boot Option', 'Secure Boot Configuration', 'Quit'):
             u_boot_console.p.expect([i])
 
     """ Unit test for "eficonfig" command
@@ -349,6 +350,7 @@ def test_efi_eficonfig(u_boot_console, efi_eficonfig_data):
         press_up_down_enter_and_wait(0, 1, True, 'Quit')
         press_up_down_enter_and_wait(0, 0, True, 'No block device found!')
         press_escape_key(False)
+        press_escape_key(False)
         check_current_is_maintenance_menu()
         # Return to U-Boot console
         press_escape_key(True)
-- 
2.17.1

[PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Masahisa Kojima]

Provide a unit test for the eficonfig secure boot key
management menu.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No change since v2

newly created in v2

 test/py/tests/test_eficonfig/conftest.py      |  84 +++-
 test/py/tests/test_eficonfig/defs.py          |  14 +
 .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
 3 files changed, 568 insertions(+), 2 deletions(-)
 create mode 100644 test/py/tests/test_eficonfig/defs.py
 create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py

diff --git a/test/py/tests/test_eficonfig/conftest.py b/test/py/tests/test_eficonfig/conftest.py
index f289df0362..6750d33989 100644
--- a/test/py/tests/test_eficonfig/conftest.py
+++ b/test/py/tests/test_eficonfig/conftest.py
@@ -2,11 +2,12 @@
 
 """Fixture for UEFI eficonfig test
 """
-
 import os
+import os.path
 import shutil
-from subprocess import check_call
+from subprocess import call, check_call, check_output, CalledProcessError
 import pytest
+from defs import *
 
 @pytest.fixture(scope='session')
 def efi_eficonfig_data(u_boot_config):
@@ -38,3 +39,82 @@ def efi_eficonfig_data(u_boot_config):
                shell=True)
 
     return image_path
+
+@pytest.fixture(scope='session')
+def efi_boot_env(request, u_boot_config):
+    """Set up a file system to be used in UEFI secure boot test.
+
+    Args:
+        request: Pytest request object.
+        u_boot_config: U-boot configuration.
+
+    Return:
+        A path to disk image to be used for testing
+    """
+    image_path = u_boot_config.persistent_data_dir
+    image_path = image_path + '/test_eficonfig_sb.img'
+
+    try:
+        mnt_point = u_boot_config.build_dir + '/mnt_eficonfig_sb'
+        check_call('rm -rf {}'.format(mnt_point), shell=True)
+        check_call('mkdir -p {}'.format(mnt_point), shell=True)
+
+        # suffix
+        # *.key: RSA private key in PEM
+        # *.crt: X509 certificate (self-signed) in PEM
+        # *.esl: signature list
+        # *.hash: message digest of image as signature list
+        # *.auth: signed signature list in signature database format
+        # *.efi: UEFI image
+        # *.efi.signed: signed UEFI image
+
+        # Create signature database
+        # PK
+        check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_PK/ -keyout PK.key -out PK.crt -nodes -days 365'
+                   % mnt_point, shell=True)
+        check_call('cd %s; %scert-to-efi-sig-list -g %s PK.crt PK.esl; %ssign-efi-sig-list -t "2020-04-01" -c PK.crt -k PK.key PK PK.esl PK.auth'
+                   % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+                   shell=True)
+        # PK_null for deletion
+        check_call('cd %s; touch PK_null.esl; %ssign-efi-sig-list -t "2020-04-02" -c PK.crt -k PK.key PK PK_null.esl PK_null.auth'
+                   % (mnt_point, EFITOOLS_PATH), shell=True)
+        # KEK
+        check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_KEK/ -keyout KEK.key -out KEK.crt -nodes -days 365'
+                   % mnt_point, shell=True)
+        check_call('cd %s; %scert-to-efi-sig-list -g %s KEK.crt KEK.esl; %ssign-efi-sig-list -t "2020-04-03" -c PK.crt -k PK.key KEK KEK.esl KEK.auth'
+                   % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+                   shell=True)
+        # db
+        check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_db/ -keyout db.key -out db.crt -nodes -days 365'
+                   % mnt_point, shell=True)
+        check_call('cd %s; %scert-to-efi-sig-list -g %s db.crt db.esl; %ssign-efi-sig-list -t "2020-04-04" -c KEK.crt -k KEK.key db db.esl db.auth'
+                   % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+                   shell=True)
+
+        # dbx_hash (digest of TEST_db certificate)
+        check_call('cd %s; %scert-to-efi-hash-list -g %s -t "2013-05-27 01:02:03" -s 256 db.crt dbx_hash.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash.crl dbx_hash.auth'
+                   % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+                   shell=True)
+
+        # Copy image
+        check_call('cp %s/lib/efi_loader/helloworld.efi %s' %
+                   (u_boot_config.build_dir, mnt_point), shell=True)
+
+        # Sign image
+        check_call('cd %s; sbsign --key db.key --cert db.crt helloworld.efi'
+                   % mnt_point, shell=True)
+
+        check_call('cd %s; rm -f *.key' % mnt_point, shell=True)
+        check_call('cd %s; rm -f *.crt' % mnt_point, shell=True)
+        check_call('cd %s; rm -f *.hash' % mnt_point, shell=True)
+        check_call('virt-make-fs --partition=gpt --size=+1M --type=vfat {} {}'.format(
+            mnt_point, image_path), shell=True)
+        check_call('rm -rf {}'.format(mnt_point), shell=True)
+
+    except CalledProcessError as exception:
+        pytest.skip('Setup failed: %s' % exception.cmd)
+        return
+    else:
+        yield image_path
+    finally:
+        call('rm -f %s' % image_path, shell=True)
diff --git a/test/py/tests/test_eficonfig/defs.py b/test/py/tests/test_eficonfig/defs.py
new file mode 100644
index 0000000000..b7a2a11851
--- /dev/null
+++ b/test/py/tests/test_eficonfig/defs.py
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier:      GPL-2.0+
+
+# Owner guid
+GUID = '11111111-2222-3333-4444-123456789abc'
+
+# v1.5.1 or earlier of efitools has a bug in sha256 calculation, and
+# you need build a newer version on your own.
+# The path must terminate with '/'.
+EFITOOLS_PATH = ''
+
+# "--addcert" option of sbsign must be available, otherwise
+# you need build a newer version on your own.
+# The path must terminate with '/'.
+SBSIGN_PATH = ''
diff --git a/test/py/tests/test_eficonfig/test_eficonfig_sbkey.py b/test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
new file mode 100644
index 0000000000..727288964d
--- /dev/null
+++ b/test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
@@ -0,0 +1,472 @@
+# SPDX-License-Identifier:      GPL-2.0+
+""" Unit test for UEFI menu-driven configuration
+"""
+
+import pytest
+import time
+from defs import *
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_eficonfig')
+@pytest.mark.buildconfigspec('cmd_bootefi_bootmgr')
+@pytest.mark.buildconfigspec('efi_secure_boot')
+def test_efi_eficonfig_sbkey(u_boot_config, u_boot_console, efi_boot_env):
+    def send_user_input_and_wait(user_str, expect_str):
+        time.sleep(0.1) # TODO: does not work correctly without sleep
+        u_boot_console.run_command(cmd=user_str, wait_for_prompt=False,
+                                   wait_for_echo=True, send_nl=False)
+        u_boot_console.run_command(cmd='\x0d', wait_for_prompt=False,
+                                   wait_for_echo=False, send_nl=False)
+        if expect_str is not None:
+            for i in expect_str:
+                u_boot_console.p.expect([i])
+
+    def press_up_down_enter_and_wait(up_count, down_count, enter, expect_str):
+        # press UP key
+        for i in range(up_count):
+            u_boot_console.run_command(cmd='\x1b\x5b\x41', wait_for_prompt=False,
+                                       wait_for_echo=False, send_nl=False)
+        # press DOWN key
+        for i in range(down_count):
+            u_boot_console.run_command(cmd='\x1b\x5b\x42', wait_for_prompt=False,
+                                       wait_for_echo=False, send_nl=False)
+        # press ENTER if requested
+        if enter:
+            u_boot_console.run_command(cmd='\x0d', wait_for_prompt=False,
+                                       wait_for_echo=False, send_nl=False)
+        # wait expected output
+        if expect_str is not None:
+            for i in expect_str:
+                u_boot_console.p.expect([i])
+
+    def press_escape_key(wait_prompt):
+        u_boot_console.run_command(cmd='\x1b', wait_for_prompt=wait_prompt, wait_for_echo=False, send_nl=False)
+
+    def press_enter_key(wait_prompt):
+        u_boot_console.run_command(cmd='\x0d', wait_for_prompt=wait_prompt,
+                                   wait_for_echo=False, send_nl=False)
+
+    def check_current_is_maintenance_menu():
+        for i in ('UEFI Maintenance Menu', 'Add Boot Option', 'Edit Boot Option',
+                  'Change Boot Order', 'Delete Boot Option', 'Secure Boot Configuration', 'Quit'):
+            u_boot_console.p.expect([i])
+
+    # Restart the system to clean the previous state
+    u_boot_console.restart_uboot()
+    # bind the test disk image for succeeding tests
+    u_boot_console.run_command(cmd = f'host bind 0 {efi_boot_env}')
+
+    #
+    # Test Case 1: Enroll non-signed ESL(.esl or .crl) in order of KEK, DB, DBX and PK
+    #
+    with u_boot_console.temporary_timeout(500):
+        u_boot_console.run_command('eficonfig', wait_for_prompt=False)
+        check_current_is_maintenance_menu()
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        for i in ('UEFI Secure Boot Key Configuration', 'SecureBoot :', 'OFF'):
+            u_boot_console.p.expect([i])
+
+        # set KEK.esl to KEK
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 7, True, 'Quit')
+        # check KEK is expected value
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, None)
+        for i in ('Show/Delete Signature Database', 'KEK',
+                  'Owner GUID:', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type:', 'X509', 'Subject:', 'TEST_KEK', 'Issuer:', 'TEST_KEK'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+        # set db.esl to db
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        # check db is expected value
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, None)
+        for i in ('Show/Delete Signature Database', 'db',
+                  'Owner GUID:', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type:', 'X509', 'Subject:', 'TEST_db', 'Issuer:', 'TEST_db'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+        # set dbx_hash.crl to dbx
+        press_up_down_enter_and_wait(0, 3, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 3, True, 'Quit')
+        # check dbx is expected value
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, None)
+        # verify CRL, skip hash comparison because it varies in every test
+        for i in ('Show/Delete Signature Database', 'dbx',
+                  'Owner GUID:', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type:', 'X509_SHA256 CRL',
+                  'TimeOfRevocation:', '2013-5-27 01:02:03'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+        # set PK.esl to PK
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 9, True, 'Quit')
+        # check PK is expected value
+        press_up_down_enter_and_wait(0, 1, True, None)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 0, True, None)
+        for i in ('Show/Delete Signature Database', 'PK',
+                  'Owner GUID', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type', 'X509', 'Subject', 'TEST_PK', 'Issuer', 'TEST_PK',
+                  'Can not delete PK, Press any key to continue'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+        for i in ('UEFI Secure Boot Key Configuration', 'SecureBoot :', 'ON'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        check_current_is_maintenance_menu()
+
+    #
+    # Test Case 2: Enroll PK first, then non-signed esl fails to enroll
+    #
+
+    # Restart the system to clean the previous state
+    u_boot_console.restart_uboot()
+    # bind the test disk image for succeeding tests
+    u_boot_console.run_command(cmd = f'host bind 0 {efi_boot_env}')
+
+    with u_boot_console.temporary_timeout(500):
+        u_boot_console.run_command('eficonfig', wait_for_prompt=False)
+        check_current_is_maintenance_menu()
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        for i in ('UEFI Secure Boot Key Configuration', 'SecureBoot :', 'OFF'):
+            u_boot_console.p.expect([i])
+
+        # set PK.auth to PK
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 8, True, 'Quit')
+        # check PK is expected value
+        press_up_down_enter_and_wait(0, 1, True, None)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 0, True, None)
+        for i in ('Show/Delete Signature Database', 'PK',
+                  'Owner GUID', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type', 'X509', 'Subject', 'TEST_PK', 'Issuer', 'TEST_PK',
+                  'Can not delete PK, Press any key to continue'):
+            u_boot_console.p.expect([i])
+
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+        for i in ('UEFI Secure Boot Key Configuration', 'SecureBoot :', 'ON'):
+            u_boot_console.p.expect([i])
+
+        # fail to set KEK.esl
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 7, True, 'Quit')
+        for i in ('ERROR! Failed to update signature database',
+                  'Press any key to continue'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+        # fail to set db.esl
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        for i in ('ERROR! Failed to update signature database',
+                  'Press any key to continue'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+        # fail to set dbx_hash.crl
+        press_up_down_enter_and_wait(0, 3, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 3, True, 'Quit')
+        for i in ('ERROR! Failed to update signature database',
+                  'Press any key to continue'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+    #
+    # Test Case 3: Enroll signed ESL(.auth) in order of PK, KEK, and db, then check status
+    #
+
+    # Restart the system to clean the previous state
+    u_boot_console.restart_uboot()
+    # bind the test disk image for succeeding tests
+    u_boot_console.run_command(cmd = f'host bind 0 {efi_boot_env}')
+
+    with u_boot_console.temporary_timeout(500):
+        u_boot_console.run_command('eficonfig', wait_for_prompt=False)
+        check_current_is_maintenance_menu()
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        for i in ('UEFI Secure Boot Key Configuration', 'SecureBoot :', 'OFF'):
+            u_boot_console.p.expect([i])
+
+        # set PK.auth to PK
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 8, True, 'Quit')
+        # check PK is expected value
+        press_up_down_enter_and_wait(0, 1, True, None)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 0, True, None)
+        for i in ('Show/Delete Signature Database', 'PK',
+                  'Owner GUID', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type', 'X509', 'Subject', 'TEST_PK', 'Issuer', 'TEST_PK',
+                  'Can not delete PK, Press any key to continue'):
+            u_boot_console.p.expect([i])
+
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+        # set KEK.auth to KEK
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 6, True, 'Quit')
+        # check KEK is expected value
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, None)
+        for i in ('Show/Delete Signature Database', 'KEK',
+                  'Owner GUID:', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type:', 'X509', 'Subject:', 'TEST_KEK', 'Issuer:', 'TEST_KEK'):
+            u_boot_console.p.expect([i])
+
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+        # set db.auth to db
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        # check db is expected value
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, None)
+        for i in ('Show/Delete Signature Database', 'db',
+                  'Owner GUID:', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type:', 'X509', 'Subject:', 'TEST_db', 'Issuer:', 'TEST_db'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+        for i in ('UEFI Secure Boot Key Configuration', 'SecureBoot :', 'ON'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        check_current_is_maintenance_menu()
+
+        #
+        # Test Case 4: start signed image allowed in db
+        #
+
+        # Select 'Add Boot Option'
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        # Press the enter key to select 'Description:' entry, then enter Description
+        press_up_down_enter_and_wait(0, 0, True, 'enter description:')
+        # Send Description user input, press ENTER key to complete
+        send_user_input_and_wait('hello', 'Quit')
+
+        # Set EFI image(helloworld.efi.signed)
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'host 0:1')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 5, True, 'Quit')
+        for i in ('Description: hello', 'File: host 0:1/helloworld.efi.signed',
+                  'Initrd File:', 'Optional Data:', 'Save', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        press_escape_key(False)
+        check_current_is_maintenance_menu()
+        press_escape_key(True)
+        response = u_boot_console.run_command(cmd = 'bootefi bootmgr')
+        assert 'Hello, world!' in response
+
+        #
+        # Test Case 5: can not start the image if it is not signed
+        #
+
+        u_boot_console.run_command('eficonfig', wait_for_prompt=False)
+        check_current_is_maintenance_menu()
+        # Select 'Edit Boot Option'
+        press_up_down_enter_and_wait(0, 1, True, None)
+        # Check the curren BootOrder
+        for i in ('hello', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 0, True, None)
+        # Set EFI image(helloworld.efi)
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'host 0:1')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        for i in ('Description: hello', 'File: host 0:1/helloworld.efi',
+                  'Initrd File:', 'Optional Data:', 'Save', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        press_escape_key(False)
+        check_current_is_maintenance_menu()
+        press_escape_key(True)
+        response = u_boot_console.run_command(cmd = 'bootefi bootmgr')
+        assert 'Image not authenticated' in response
+
+        #
+        # Test Case 6: can not start the signed image if dbx revokes db certificate
+        #
+
+        u_boot_console.run_command('eficonfig', wait_for_prompt=False)
+        check_current_is_maintenance_menu()
+        # Select 'Edit Boot Option'
+        press_up_down_enter_and_wait(0, 1, True, None)
+        # Check the curren BootOrder
+        for i in ('hello', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        # Set EFI image(helloworld.efi.signed)
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'host 0:1')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 5, True, 'Quit')
+        for i in ('Description: hello', 'File: host 0:1/helloworld.efi.signed',
+                  'Initrd File:', 'Optional Data:', 'Save', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        press_escape_key(False)
+        check_current_is_maintenance_menu()
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        # set dbx_hash.auth to dbx
+        press_up_down_enter_and_wait(0, 3, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+        # check db is expected value
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, None)
+        # verify CRL, skip hash comparison because it varies in every test
+        for i in ('Show/Delete Signature Database', 'dbx',
+                  'Owner GUID:', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type:', 'X509_SHA256 CRL',
+                  'TimeOfRevocation:', '2013-5-27 01:02:03'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+        press_escape_key(False)
+        check_current_is_maintenance_menu()
+        press_escape_key(True)
+        response = u_boot_console.run_command(cmd = 'bootefi bootmgr')
+        assert 'Image not authenticated' in response
+
+        #
+        # Test Case 7: clear PK with null key, check secure boot is OFF
+        #
+
+        u_boot_console.run_command('eficonfig', wait_for_prompt=False)
+        check_current_is_maintenance_menu()
+        press_up_down_enter_and_wait(0, 4, True, 'Quit')
+        for i in ('UEFI Secure Boot Key Configuration', 'SecureBoot :', 'ON'):
+            u_boot_console.p.expect([i])
+
+        # clear PK with null key
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 10, True, 'Quit')
+
+        press_up_down_enter_and_wait(0, 1, True, None)
+        for i in ('There is no entry in the signature database.', 'Press any key to continue'):
+            u_boot_console.p.expect([i])
+
+        press_enter_key(False)
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+        for i in ('UEFI Secure Boot Key Configuration', 'SecureBoot :', 'OFF'):
+            u_boot_console.p.expect([i])
+
+        # delete dbx
+        press_up_down_enter_and_wait(0, 3, True, 'Quit')
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_enter_key(False)
+        for i in ('TimeOfRevocation:', '2013-5-27 01:02:03'):
+            u_boot_console.p.expect([i])
+        press_enter_key(False)
+        for i in ('Are you sure you want to delete this item?', 'Press ENTER to delete'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 0, True, None)
+        for i in ('There is no entry in the signature database.',
+                  'Press any key to continue'):
+            u_boot_console.p.expect([i])
+        press_enter_key(False)
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+
+        # set PK.auth to PK
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 0, True, 'Quit')
+        press_up_down_enter_and_wait(0, 8, True, 'Quit')
+        # check PK is expected value
+        press_up_down_enter_and_wait(0, 1, True, None)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 0, True, None)
+        for i in ('Show/Delete Signature Database', 'PK',
+                  'Owner GUID', '11111111-2222-3333-4444-123456789ABC',
+                  'Signature Type', 'X509', 'Subject', 'TEST_PK', 'Issuer', 'TEST_PK',
+                  'Can not delete PK, Press any key to continue'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        for i in ('11111111-2222-3333-4444-123456789ABC', 'Quit'):
+            u_boot_console.p.expect([i])
+        press_up_down_enter_and_wait(0, 1, True, 'Quit')
+        press_up_down_enter_and_wait(0, 2, True, 'Quit')
+        for i in ('UEFI Secure Boot Key Configuration', 'SecureBoot :', 'ON'):
+            u_boot_console.p.expect([i])
+        press_escape_key(False)
+        check_current_is_maintenance_menu()
+        press_escape_key(True)
+        response = u_boot_console.run_command(cmd = 'bootefi bootmgr')
+        assert 'Hello, world!' in response
-- 
2.17.1

Re: [PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Simon Glass]

Hi,

On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Provide a unit test for the eficonfig secure boot key
> management menu.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> No change since v2
>
> newly created in v2
>
>  test/py/tests/test_eficonfig/conftest.py      |  84 +++-
>  test/py/tests/test_eficonfig/defs.py          |  14 +
>  .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
>  3 files changed, 568 insertions(+), 2 deletions(-)
>  create mode 100644 test/py/tests/test_eficonfig/defs.py
>  create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py

Please can this test be in C? Also, using down-arrow to select menus
is brittle. Add a function to select the one you want, e.g. by name.

Regards,
Simon

Re: [PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Ilias Apalodimas]

Hi Simon,

On Fri, 14 Oct 2022 at 18:56, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > Provide a unit test for the eficonfig secure boot key
> > management menu.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > No change since v2
> >
> > newly created in v2
> >
> >  test/py/tests/test_eficonfig/conftest.py      |  84 +++-
> >  test/py/tests/test_eficonfig/defs.py          |  14 +
> >  .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
> >  3 files changed, 568 insertions(+), 2 deletions(-)
> >  create mode 100644 test/py/tests/test_eficonfig/defs.py
> >  create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
>
> Please can this test be in C? Also, using down-arrow to select menus
> is brittle. Add a function to select the one you want, e.g. by name.
>

Is there a very specific reason why we should do stuff like that in C?
 Python is way easier to extend and test in our case.

Thanks
/Ilias
> Regards,
> Simon

Re: [PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Simon Glass]

Hi Ilias,

On Fri, 14 Oct 2022 at 09:59, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 14 Oct 2022 at 18:56, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi,
> >
> > On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
> > <masahisa.kojima@linaro.org> wrote:
> > >
> > > Provide a unit test for the eficonfig secure boot key
> > > management menu.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > > No change since v2
> > >
> > > newly created in v2
> > >
> > >  test/py/tests/test_eficonfig/conftest.py      |  84 +++-
> > >  test/py/tests/test_eficonfig/defs.py          |  14 +
> > >  .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
> > >  3 files changed, 568 insertions(+), 2 deletions(-)
> > >  create mode 100644 test/py/tests/test_eficonfig/defs.py
> > >  create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
> >
> > Please can this test be in C? Also, using down-arrow to select menus
> > is brittle. Add a function to select the one you want, e.g. by name.
> >
>
> Is there a very specific reason why we should do stuff like that in C?

Yes, see here.

>  Python is way easier to extend and test in our case.

In what way? It seems a lot more complicated, plus the brittle nature
of this test suggests it will be a hassle to maintain.

https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html#python-or-c

There is a pending update here too:

https://patchwork.ozlabs.org/project/uboot/patch/20221013122927.636867-15-sjg@chromium.org/

Regards,
SImon

Re: [PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Heinrich Schuchardt]

On 10/15/22 03:10, Simon Glass wrote:
> Hi Ilias,
>
> On Fri, 14 Oct 2022 at 09:59, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> Hi Simon,
>>
>> On Fri, 14 Oct 2022 at 18:56, Simon Glass <sjg@chromium.org> wrote:
>>>
>>> Hi,
>>>
>>> On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
>>> <masahisa.kojima@linaro.org> wrote:
>>>>
>>>> Provide a unit test for the eficonfig secure boot key
>>>> management menu.
>>>>
>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>> ---
>>>> No change since v2
>>>>
>>>> newly created in v2
>>>>
>>>>   test/py/tests/test_eficonfig/conftest.py      |  84 +++-
>>>>   test/py/tests/test_eficonfig/defs.py          |  14 +
>>>>   .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
>>>>   3 files changed, 568 insertions(+), 2 deletions(-)
>>>>   create mode 100644 test/py/tests/test_eficonfig/defs.py
>>>>   create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
>>>
>>> Please can this test be in C? Also, using down-arrow to select menus
>>> is brittle. Add a function to select the one you want, e.g. by name.
>>>
>>
>> Is there a very specific reason why we should do stuff like that in C?
>
> Yes, see here.
>
>>   Python is way easier to extend and test in our case.
>
> In what way? It seems a lot more complicated, plus the brittle nature
> of this test suggests it will be a hassle to maintain.
>
> https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html#python-or-c
>
> There is a pending update here too:
>
> https://patchwork.ozlabs.org/project/uboot/patch/20221013122927.636867-15-sjg@chromium.org/

The discussion touches different aspects:

** What does it take to make a GUI easily testable? **

Relying on cursor movement for testing is fragile. This is why GUIs
often assign to each executable element the following:

* Access key, e.g  <CTRL><S>
   A key combination that when entered will have the the same
   effect as selecting the GUI item
* Access string, '@SAVE'
   A string that when typed into the command field will have
   the same effect as selecting the GUI item

Let's look at U-Boot's menu entry definition:

struct bootmenu_entry {
     unsigned short int num;      // unique number 0 .. MAX_COUNT
     char key[3];                 // key identifier of number
     char *title;                 // title of entry
     char *command;               // hush command of entry
     enum boot_type type;         // boot type of entry
     u16 bootorder;               // order for each boot type
     struct bootmenu_data *menu;  // this bootmenu
     struct bootmenu_entry *next; // next menu entry (num+1)
}

Our structure lacks an accessor element that can be used to select a
menu item without using cursor actions.

Compound keystrokes like <CTRL><F6> are send as multiple bytes on the
console, e.g. 1b 5b 31 37 3b 35 7e.

We may define a field shortcut of type char *. If the string is received
by the menu loop, let it activate the matching menu entry. Let cursor
actions (up, down, enter, space '+', '-') interrupt matching the
shortcut string.

Instead we could also use a convention for the title:

If a letter in the title is preceded by '&', this is the shortcut key.
This letter will be shown highlighted in the menu and the ampersand will
not be shown.

This is probably easier to implement.

Adding the shortcut facility will allow both for easier testing and
faster navigation.

** Choice of programming language **

Several aspects control the choice of the programming language for tests:

- Testing single library functions is only possible in C.
- Checking contents of internal structures is only possible in C.
- Testing the U-Boot's CLI is easily accessible in our Python framework.
- Preparation of complex test data is easier to do in Python.
- Mixed language tests should be avoided if not strictly necessary.
   It is much easier to maintain a single code source for a test.

** What to do for secure boot key management? **

Secure boot key management requires complex preparation which fits well
into out Python testing framework.

Once we provide shortcut keys to U-Boot menus the entries will be easily
accessible from Python.

As we want to avoid complexity due to mixed language tests we should
stick to Python for testing key management at the user level.

Additional tests in C for library functions managing keys and verifying
signatures would improve our code coverage.

Best regards

Heinrich

Re: [PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Masahisa Kojima]

Hi Heinrich,

On Sat, 15 Oct 2022 at 13:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/15/22 03:10, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Fri, 14 Oct 2022 at 09:59, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> On Fri, 14 Oct 2022 at 18:56, Simon Glass <sjg@chromium.org> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
> >>> <masahisa.kojima@linaro.org> wrote:
> >>>>
> >>>> Provide a unit test for the eficonfig secure boot key
> >>>> management menu.
> >>>>
> >>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>> ---
> >>>> No change since v2
> >>>>
> >>>> newly created in v2
> >>>>
> >>>>   test/py/tests/test_eficonfig/conftest.py      |  84 +++-
> >>>>   test/py/tests/test_eficonfig/defs.py          |  14 +
> >>>>   .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
> >>>>   3 files changed, 568 insertions(+), 2 deletions(-)
> >>>>   create mode 100644 test/py/tests/test_eficonfig/defs.py
> >>>>   create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
> >>>
> >>> Please can this test be in C? Also, using down-arrow to select menus
> >>> is brittle. Add a function to select the one you want, e.g. by name.
> >>>
> >>
> >> Is there a very specific reason why we should do stuff like that in C?
> >
> > Yes, see here.
> >
> >>   Python is way easier to extend and test in our case.
> >
> > In what way? It seems a lot more complicated, plus the brittle nature
> > of this test suggests it will be a hassle to maintain.
> >
> > https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html#python-or-c
> >
> > There is a pending update here too:
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20221013122927.636867-15-sjg@chromium.org/
>
> The discussion touches different aspects:
>
> ** What does it take to make a GUI easily testable? **
>
> Relying on cursor movement for testing is fragile. This is why GUIs
> often assign to each executable element the following:
>
> * Access key, e.g  <CTRL><S>
>    A key combination that when entered will have the the same
>    effect as selecting the GUI item
> * Access string, '@SAVE'
>    A string that when typed into the command field will have
>    the same effect as selecting the GUI item
>
> Let's look at U-Boot's menu entry definition:
>
> struct bootmenu_entry {
>      unsigned short int num;      // unique number 0 .. MAX_COUNT
>      char key[3];                 // key identifier of number
>      char *title;                 // title of entry
>      char *command;               // hush command of entry
>      enum boot_type type;         // boot type of entry
>      u16 bootorder;               // order for each boot type
>      struct bootmenu_data *menu;  // this bootmenu
>      struct bootmenu_entry *next; // next menu entry (num+1)
> }
>
> Our structure lacks an accessor element that can be used to select a
> menu item without using cursor actions.
>
> Compound keystrokes like <CTRL><F6> are send as multiple bytes on the
> console, e.g. 1b 5b 31 37 3b 35 7e.
>
> We may define a field shortcut of type char *. If the string is received
> by the menu loop, let it activate the matching menu entry. Let cursor
> actions (up, down, enter, space '+', '-') interrupt matching the
> shortcut string.
>
> Instead we could also use a convention for the title:
>
> If a letter in the title is preceded by '&', this is the shortcut key.
> This letter will be shown highlighted in the menu and the ampersand will
> not be shown.
>
> This is probably easier to implement.

I agree to the shortcut by '&', I will implement it and update the
python test code.

Thanks,
Masahisa Kojima

>
> Adding the shortcut facility will allow both for easier testing and
> faster navigation.
>
> ** Choice of programming language **
>
> Several aspects control the choice of the programming language for tests:
>
> - Testing single library functions is only possible in C.
> - Checking contents of internal structures is only possible in C.
> - Testing the U-Boot's CLI is easily accessible in our Python framework.
> - Preparation of complex test data is easier to do in Python.
> - Mixed language tests should be avoided if not strictly necessary.
>    It is much easier to maintain a single code source for a test.
>
> ** What to do for secure boot key management? **
>
> Secure boot key management requires complex preparation which fits well
> into out Python testing framework.
>
> Once we provide shortcut keys to U-Boot menus the entries will be easily
> accessible from Python.
>
> As we want to avoid complexity due to mixed language tests we should
> stick to Python for testing key management at the user level.
>
> Additional tests in C for library functions managing keys and verifying
> signatures would improve our code coverage.
>
> Best regards
>
> Heinrich

Re: [PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Masahisa Kojima]

Hi Heinrich, Ilias, Simon,

On Mon, 17 Oct 2022 at 13:49, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Heinrich,
>
> On Sat, 15 Oct 2022 at 13:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 10/15/22 03:10, Simon Glass wrote:
> > > Hi Ilias,
> > >
> > > On Fri, 14 Oct 2022 at 09:59, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > >>
> > >> Hi Simon,
> > >>
> > >> On Fri, 14 Oct 2022 at 18:56, Simon Glass <sjg@chromium.org> wrote:
> > >>>
> > >>> Hi,
> > >>>
> > >>> On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
> > >>> <masahisa.kojima@linaro.org> wrote:
> > >>>>
> > >>>> Provide a unit test for the eficonfig secure boot key
> > >>>> management menu.
> > >>>>
> > >>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > >>>> ---
> > >>>> No change since v2
> > >>>>
> > >>>> newly created in v2
> > >>>>
> > >>>>   test/py/tests/test_eficonfig/conftest.py      |  84 +++-
> > >>>>   test/py/tests/test_eficonfig/defs.py          |  14 +
> > >>>>   .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
> > >>>>   3 files changed, 568 insertions(+), 2 deletions(-)
> > >>>>   create mode 100644 test/py/tests/test_eficonfig/defs.py
> > >>>>   create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
> > >>>
> > >>> Please can this test be in C? Also, using down-arrow to select menus
> > >>> is brittle. Add a function to select the one you want, e.g. by name.
> > >>>
> > >>
> > >> Is there a very specific reason why we should do stuff like that in C?
> > >
> > > Yes, see here.
> > >
> > >>   Python is way easier to extend and test in our case.
> > >
> > > In what way? It seems a lot more complicated, plus the brittle nature
> > > of this test suggests it will be a hassle to maintain.
> > >
> > > https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html#python-or-c
> > >
> > > There is a pending update here too:
> > >
> > > https://patchwork.ozlabs.org/project/uboot/patch/20221013122927.636867-15-sjg@chromium.org/
> >
> > The discussion touches different aspects:
> >
> > ** What does it take to make a GUI easily testable? **
> >
> > Relying on cursor movement for testing is fragile. This is why GUIs
> > often assign to each executable element the following:
> >
> > * Access key, e.g  <CTRL><S>
> >    A key combination that when entered will have the the same
> >    effect as selecting the GUI item
> > * Access string, '@SAVE'
> >    A string that when typed into the command field will have
> >    the same effect as selecting the GUI item
> >
> > Let's look at U-Boot's menu entry definition:
> >
> > struct bootmenu_entry {
> >      unsigned short int num;      // unique number 0 .. MAX_COUNT
> >      char key[3];                 // key identifier of number
> >      char *title;                 // title of entry
> >      char *command;               // hush command of entry
> >      enum boot_type type;         // boot type of entry
> >      u16 bootorder;               // order for each boot type
> >      struct bootmenu_data *menu;  // this bootmenu
> >      struct bootmenu_entry *next; // next menu entry (num+1)
> > }
> >
> > Our structure lacks an accessor element that can be used to select a
> > menu item without using cursor actions.
> >
> > Compound keystrokes like <CTRL><F6> are send as multiple bytes on the
> > console, e.g. 1b 5b 31 37 3b 35 7e.
> >
> > We may define a field shortcut of type char *. If the string is received
> > by the menu loop, let it activate the matching menu entry. Let cursor
> > actions (up, down, enter, space '+', '-') interrupt matching the
> > shortcut string.
> >
> > Instead we could also use a convention for the title:
> >
> > If a letter in the title is preceded by '&', this is the shortcut key.
> > This letter will be shown highlighted in the menu and the ampersand will
> > not be shown.
> >
> > This is probably easier to implement.
>
> I agree to the shortcut by '&', I will implement it and update the
> python test code.

The shortcut functionality is a kind of feature enhancement of eficonfig menus.
If there is no additional comment and feedback other than the shortcut feature,
could you consider to merge this series first?
I would like to get more user feedback.

I will send the follow up patches to support the shortcut feature
after this original
series is merged.

Thanks,
Masahisa Kojima

>
> Thanks,
> Masahisa Kojima
>
> >
> > Adding the shortcut facility will allow both for easier testing and
> > faster navigation.
> >
> > ** Choice of programming language **
> >
> > Several aspects control the choice of the programming language for tests:
> >
> > - Testing single library functions is only possible in C.
> > - Checking contents of internal structures is only possible in C.
> > - Testing the U-Boot's CLI is easily accessible in our Python framework.
> > - Preparation of complex test data is easier to do in Python.
> > - Mixed language tests should be avoided if not strictly necessary.
> >    It is much easier to maintain a single code source for a test.
> >
> > ** What to do for secure boot key management? **
> >
> > Secure boot key management requires complex preparation which fits well
> > into out Python testing framework.
> >
> > Once we provide shortcut keys to U-Boot menus the entries will be easily
> > accessible from Python.
> >
> > As we want to avoid complexity due to mixed language tests we should
> > stick to Python for testing key management at the user level.
> >
> > Additional tests in C for library functions managing keys and verifying
> > signatures would improve our code coverage.
> >
> > Best regards
> >
> > Heinrich

Re: [PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Simon Glass]

iHi Heinrich,

On Fri, 14 Oct 2022 at 22:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/15/22 03:10, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Fri, 14 Oct 2022 at 09:59, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> On Fri, 14 Oct 2022 at 18:56, Simon Glass <sjg@chromium.org> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
> >>> <masahisa.kojima@linaro.org> wrote:
> >>>>
> >>>> Provide a unit test for the eficonfig secure boot key
> >>>> management menu.
> >>>>
> >>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>> ---
> >>>> No change since v2
> >>>>
> >>>> newly created in v2
> >>>>
> >>>>   test/py/tests/test_eficonfig/conftest.py      |  84 +++-
> >>>>   test/py/tests/test_eficonfig/defs.py          |  14 +
> >>>>   .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
> >>>>   3 files changed, 568 insertions(+), 2 deletions(-)
> >>>>   create mode 100644 test/py/tests/test_eficonfig/defs.py
> >>>>   create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
> >>>
> >>> Please can this test be in C? Also, using down-arrow to select menus
> >>> is brittle. Add a function to select the one you want, e.g. by name.
> >>>
> >>
> >> Is there a very specific reason why we should do stuff like that in C?
> >
> > Yes, see here.
> >
> >>   Python is way easier to extend and test in our case.
> >
> > In what way? It seems a lot more complicated, plus the brittle nature
> > of this test suggests it will be a hassle to maintain.
> >
> > https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html#python-or-c
> >
> > There is a pending update here too:
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20221013122927.636867-15-sjg@chromium.org/
>
> The discussion touches different aspects:
>
> ** What does it take to make a GUI easily testable? **
>
> Relying on cursor movement for testing is fragile. This is why GUIs
> often assign to each executable element the following:
>
> * Access key, e.g  <CTRL><S>
>    A key combination that when entered will have the the same
>    effect as selecting the GUI item
> * Access string, '@SAVE'
>    A string that when typed into the command field will have
>    the same effect as selecting the GUI item
>
> Let's look at U-Boot's menu entry definition:
>
> struct bootmenu_entry {
>      unsigned short int num;      // unique number 0 .. MAX_COUNT
>      char key[3];                 // key identifier of number
>      char *title;                 // title of entry
>      char *command;               // hush command of entry
>      enum boot_type type;         // boot type of entry
>      u16 bootorder;               // order for each boot type
>      struct bootmenu_data *menu;  // this bootmenu
>      struct bootmenu_entry *next; // next menu entry (num+1)
> }
>
> Our structure lacks an accessor element that can be used to select a
> menu item without using cursor actions.
>
> Compound keystrokes like <CTRL><F6> are send as multiple bytes on the
> console, e.g. 1b 5b 31 37 3b 35 7e.
>
> We may define a field shortcut of type char *. If the string is received
> by the menu loop, let it activate the matching menu entry. Let cursor
> actions (up, down, enter, space '+', '-') interrupt matching the
> shortcut string.
>
> Instead we could also use a convention for the title:
>
> If a letter in the title is preceded by '&', this is the shortcut key.
> This letter will be shown highlighted in the menu and the ampersand will
> not be shown.
>
> This is probably easier to implement.
>
> Adding the shortcut facility will allow both for easier testing and
> faster navigation.
>
> ** Choice of programming language **
>
> Several aspects control the choice of the programming language for tests:
>
> - Testing single library functions is only possible in C.
> - Checking contents of internal structures is only possible in C.
> - Testing the U-Boot's CLI is easily accessible in our Python framework.
> - Preparation of complex test data is easier to do in Python.
> - Mixed language tests should be avoided if not strictly necessary.
>    It is much easier to maintain a single code source for a test.
>
> ** What to do for secure boot key management? **
>
> Secure boot key management requires complex preparation which fits well
> into out Python testing framework.
>
> Once we provide shortcut keys to U-Boot menus the entries will be easily
> accessible from Python.
>
> As we want to avoid complexity due to mixed language tests we should
> stick to Python for testing key management at the user level.

The test setup should be in Python (and needs to be) but the actual
test should be in C, in general.

From what I can tell these tests should be split up, one for each test case.

Using keyboard shortcuts for menus would certainly help, but you still
have the dependency between the menu code in C and the Python code
that uses it. The complexity of this dependency is quite significant.

Can we not design this all for easier testing? The functionality
should be broken down a little more so we can change the configs in a
test (e.g. using the eficonfig command), then check that booting does
what we expect.

Overall I feel that this 'black box' testing is not a good approach
and we should try to test the components. It is OK to have a
happy-path test but here we are just writing hundreds of lines of
Python to get a very high-level signal about correctness.

>
> Additional tests in C for library functions managing keys and verifying
> signatures would improve our code coverage.
>
> Best regards
>
> Heinrich

Regards,
Simon

Re: [PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Heinrich Schuchardt]

On 10/19/22 15:17, Simon Glass wrote:
> iHi Heinrich,
>
> On Fri, 14 Oct 2022 at 22:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/15/22 03:10, Simon Glass wrote:
>>> Hi Ilias,
>>>
>>> On Fri, 14 Oct 2022 at 09:59, Ilias Apalodimas
>>> <ilias.apalodimas@linaro.org> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On Fri, 14 Oct 2022 at 18:56, Simon Glass <sjg@chromium.org> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
>>>>> <masahisa.kojima@linaro.org> wrote:
>>>>>>
>>>>>> Provide a unit test for the eficonfig secure boot key
>>>>>> management menu.
>>>>>>
>>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>>>> ---
>>>>>> No change since v2
>>>>>>
>>>>>> newly created in v2
>>>>>>
>>>>>>    test/py/tests/test_eficonfig/conftest.py      |  84 +++-
>>>>>>    test/py/tests/test_eficonfig/defs.py          |  14 +
>>>>>>    .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
>>>>>>    3 files changed, 568 insertions(+), 2 deletions(-)
>>>>>>    create mode 100644 test/py/tests/test_eficonfig/defs.py
>>>>>>    create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
>>>>>
>>>>> Please can this test be in C? Also, using down-arrow to select menus
>>>>> is brittle. Add a function to select the one you want, e.g. by name.
>>>>>
>>>>
>>>> Is there a very specific reason why we should do stuff like that in C?
>>>
>>> Yes, see here.
>>>
>>>>    Python is way easier to extend and test in our case.
>>>
>>> In what way? It seems a lot more complicated, plus the brittle nature
>>> of this test suggests it will be a hassle to maintain.
>>>
>>> https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html#python-or-c
>>>
>>> There is a pending update here too:
>>>
>>> https://patchwork.ozlabs.org/project/uboot/patch/20221013122927.636867-15-sjg@chromium.org/
>>
>> The discussion touches different aspects:
>>
>> ** What does it take to make a GUI easily testable? **
>>
>> Relying on cursor movement for testing is fragile. This is why GUIs
>> often assign to each executable element the following:
>>
>> * Access key, e.g  <CTRL><S>
>>     A key combination that when entered will have the the same
>>     effect as selecting the GUI item
>> * Access string, '@SAVE'
>>     A string that when typed into the command field will have
>>     the same effect as selecting the GUI item
>>
>> Let's look at U-Boot's menu entry definition:
>>
>> struct bootmenu_entry {
>>       unsigned short int num;      // unique number 0 .. MAX_COUNT
>>       char key[3];                 // key identifier of number
>>       char *title;                 // title of entry
>>       char *command;               // hush command of entry
>>       enum boot_type type;         // boot type of entry
>>       u16 bootorder;               // order for each boot type
>>       struct bootmenu_data *menu;  // this bootmenu
>>       struct bootmenu_entry *next; // next menu entry (num+1)
>> }
>>
>> Our structure lacks an accessor element that can be used to select a
>> menu item without using cursor actions.
>>
>> Compound keystrokes like <CTRL><F6> are send as multiple bytes on the
>> console, e.g. 1b 5b 31 37 3b 35 7e.
>>
>> We may define a field shortcut of type char *. If the string is received
>> by the menu loop, let it activate the matching menu entry. Let cursor
>> actions (up, down, enter, space '+', '-') interrupt matching the
>> shortcut string.
>>
>> Instead we could also use a convention for the title:
>>
>> If a letter in the title is preceded by '&', this is the shortcut key.
>> This letter will be shown highlighted in the menu and the ampersand will
>> not be shown.
>>
>> This is probably easier to implement.
>>
>> Adding the shortcut facility will allow both for easier testing and
>> faster navigation.
>>
>> ** Choice of programming language **
>>
>> Several aspects control the choice of the programming language for tests:
>>
>> - Testing single library functions is only possible in C.
>> - Checking contents of internal structures is only possible in C.
>> - Testing the U-Boot's CLI is easily accessible in our Python framework.
>> - Preparation of complex test data is easier to do in Python.
>> - Mixed language tests should be avoided if not strictly necessary.
>>     It is much easier to maintain a single code source for a test.
>>
>> ** What to do for secure boot key management? **
>>
>> Secure boot key management requires complex preparation which fits well
>> into out Python testing framework.
>>
>> Once we provide shortcut keys to U-Boot menus the entries will be easily
>> accessible from Python.
>>
>> As we want to avoid complexity due to mixed language tests we should
>> stick to Python for testing key management at the user level.
>
> The test setup should be in Python (and needs to be) but the actual
> test should be in C, in general.
>
>>From what I can tell these tests should be split up, one for each test case.
>
> Using keyboard shortcuts for menus would certainly help, but you still
> have the dependency between the menu code in C and the Python code
> that uses it. The complexity of this dependency is quite significant.
>
> Can we not design this all for easier testing? The functionality
> should be broken down a little more so we can change the configs in a
> test (e.g. using the eficonfig command), then check that booting does
> what we expect.
>
> Overall I feel that this 'black box' testing is not a good approach
> and we should try to test the components. It is OK to have a
> happy-path test but here we are just writing hundreds of lines of
> Python to get a very high-level signal about correctness.

 From other projects I am used to differentiate between unit tests and
integration tests.

An integration test is what checks that all the parts work together
while a unit test checks a single functionality works as designed.

A description can be found in
https://circleci.com/blog/unit-testing-vs-integration-testing/#c-consent-modal

For an integration test this Python script is fine (except for the menu
navigation.

As I said in my last mail additional tests on a lower level may be
added. But unit tests cannot replace an integration test.

Best regards

Heinrich

>
>>
>> Additional tests in C for library functions managing keys and verifying
>> signatures would improve our code coverage.
>>
>> Best regards
>>
>> Heinrich
>
> Regards,
> Simon

Re: [PATCH v3 0/6] eficonfig: add UEFI Secure Boot key maintenance interface

[Heinrich Schuchardt]

On 10/14/22 08:56, 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.
>
> Source code can be cloned with:
> $ git clone https://git.linaro.org/people/masahisa.kojima/u-boot.git -b kojima/eficonfig_sbkey_v3
>
> [Major Changes]
> - rebased on top of u-boot/master
>
> Masahisa Kojima (6):
>    eficonfig: refactor eficonfig_select_file_handler()
>    eficonfig: expose append entry function
>    eficonfig: add UEFI Secure Boot Key enrollment interface
>    eficonfig: add "Show/Delete Signature Database" menu entry
>    test/eficonfig: support secure boot key maintenance menu
>    test: add test for eficonfig secure boot key management
>
>   cmd/Makefile                                  |   3 +
>   cmd/eficonfig.c                               |  48 +-
>   cmd/eficonfig_sbkey.c                         | 751 ++++++++++++++++++
>   include/efi_config.h                          |  10 +
>   test/py/tests/test_eficonfig/conftest.py      |  84 +-
>   test/py/tests/test_eficonfig/defs.py          |  14 +
>   .../py/tests/test_eficonfig/test_eficonfig.py |   4 +-
>   .../test_eficonfig/test_eficonfig_sbkey.py    | 472 +++++++++++
>   8 files changed, 1360 insertions(+), 26 deletions(-)
>   create mode 100644 cmd/eficonfig_sbkey.c
>   create mode 100644 test/py/tests/test_eficonfig/defs.py
>   create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
>

Python tests with this series fail. See

https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/518130

Please, run 'make tests' before resubmitting.

Best regards

Heinrich

Re: [PATCH v3 0/6] eficonfig: add UEFI Secure Boot key maintenance interface

[Masahisa Kojima]

Hi Heinrich,


On Sat, 22 Oct 2022 at 17:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/14/22 08:56, 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.
> >
> > Source code can be cloned with:
> > $ git clone https://git.linaro.org/people/masahisa.kojima/u-boot.git -b kojima/eficonfig_sbkey_v3
> >
> > [Major Changes]
> > - rebased on top of u-boot/master
> >
> > Masahisa Kojima (6):
> >    eficonfig: refactor eficonfig_select_file_handler()
> >    eficonfig: expose append entry function
> >    eficonfig: add UEFI Secure Boot Key enrollment interface
> >    eficonfig: add "Show/Delete Signature Database" menu entry
> >    test/eficonfig: support secure boot key maintenance menu
> >    test: add test for eficonfig secure boot key management
> >
> >   cmd/Makefile                                  |   3 +
> >   cmd/eficonfig.c                               |  48 +-
> >   cmd/eficonfig_sbkey.c                         | 751 ++++++++++++++++++
> >   include/efi_config.h                          |  10 +
> >   test/py/tests/test_eficonfig/conftest.py      |  84 +-
> >   test/py/tests/test_eficonfig/defs.py          |  14 +
> >   .../py/tests/test_eficonfig/test_eficonfig.py |   4 +-
> >   .../test_eficonfig/test_eficonfig_sbkey.py    | 472 +++++++++++
> >   8 files changed, 1360 insertions(+), 26 deletions(-)
> >   create mode 100644 cmd/eficonfig_sbkey.c
> >   create mode 100644 test/py/tests/test_eficonfig/defs.py
> >   create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
> >
>
> Python tests with this series fail. See
>
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/518130

Failing item is test_capsule_firmware_raw.py, not eficonfig.
test_eficonfig_sbkey.py test is successful.

Thanks,
Masahisa Kojima

>
> Please, run 'make tests' before resubmitting.
>
> Best regards
>
> Heinrich

Re: [PATCH v3 0/6] eficonfig: add UEFI Secure Boot key maintenance interface

[Heinrich Schuchardt]

On 10/23/22 08:13, Masahisa Kojima wrote:
> Hi Heinrich,
>
>
> On Sat, 22 Oct 2022 at 17:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/14/22 08:56, Masahisa Kojima wrote:

<snip />

>> Python tests with this series fail. See
>>
>> https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/518130
>
> Failing item is test_capsule_firmware_raw.py, not eficonfig.
> test_eficonfig_sbkey.py test is successful.
>
> Thanks,
> Masahisa Kojima
>

Bisecting showed that the problem was with one of my patches which I
have corrected now:

https://lists.denx.de/pipermail/u-boot/2022-October/498097.html
[PATCH v2 1/1] efi_loader: discover if no efi_system_partition is set

Sorry.

Best regards

Heinrich

Re: [PATCH v3 3/6] eficonfig: add UEFI Secure Boot Key enrollment interface

[Heinrich Schuchardt]

On 10/14/22 08:56, 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>
> ---
> Changes in v3:
> - fix error handling
>
> Changes in v2:
> - allow to enroll .esl file
> - fix typos
> - add function comments
>
>   cmd/Makefile          |   3 +
>   cmd/eficonfig.c       |   3 +
>   cmd/eficonfig_sbkey.c | 357 ++++++++++++++++++++++++++++++++++++++++++
>   include/efi_config.h  |   5 +
>   4 files changed, 368 insertions(+)
>   create mode 100644 cmd/eficonfig_sbkey.c
>
> diff --git a/cmd/Makefile b/cmd/Makefile
> index c95e09d058..f2f2857146 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -66,6 +66,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 0cb0770ac3..a72f07e671 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -2442,6 +2442,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..cc27f78e66
> --- /dev/null
> +++ b/cmd/eficonfig_sbkey.c
> @@ -0,0 +1,357 @@
> +// 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>
> +
> +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}, */
> +};
> +
> +/**
> + * is_secureboot_enabled() - check UEFI Secure Boot is enabled
> + *
> + * Return:	true when UEFI Secure Boot is enabled, false otherwise
> + */
> +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;
> +}
> +
> +/**
> + * create_time_based_payload() - create payload for time based authenticate variable
> + *
> + * @db:		pointer to the original signature database
> + * @new_db:	pointer to the authenticated variable payload
> + * @size:	pointer to payload size
> + * Return:	status code
> + */
> +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;
> +}
> +
> +/**
> + * file_have_auth_header() - check file has EFI_VARIABLE_AUTHENTICATION_2 header
> + * @buf:	pointer to file
> + * @size:	file size
> + * Return:	true if file has auth header, false otherwise
> + */
> +static bool file_have_auth_header(void *buf, efi_uintn_t size)
> +{
> +	struct efi_variable_authentication_2 *auth = buf;
> +
> +	if (auth->auth_info.hdr.wCertificateType != WIN_CERT_TYPE_EFI_GUID)
> +		return false;
> +
> +	if (guidcmp(&auth->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * file_is_efi_signature_list() - check the file is efi signature list
> + * @buf:	pointer to file
> + * Return:	true if file is efi signature list, false otherwise
> + */
> +static bool file_is_efi_signature_list(void *buf)
> +{
> +	u32 i;
> +	struct efi_signature_list *sig_list = buf;
> +
> +	for (i = 0; i < ARRAY_SIZE(sigtype_to_str); i++) {
> +		if (!guidcmp(&sig_list->signature_type, &sigtype_to_str[i].sig_type))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +/**
> + * eficonfig_process_enroll_key() - enroll key into signature database
> + *
> + * @data:	pointer to the data for each entry
> + * Return:	status code
> + */
> +static efi_status_t eficonfig_process_enroll_key(void *data)
> +{
> +	u32 attr;
> +	char *buf = NULL;
> +	efi_uintn_t size;
> +	efi_status_t ret;
> +	void *new_db = NULL;
> +	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) {
> +		ret = EFI_OUT_OF_RESOURCES;

This leaves ret undefined and you get an error when compiling with clang:

+cmd/eficonfig_sbkey.c:208:6: error: variable 'ret' is used
uninitialized whenever 'if' condition is true
[-Werror,-Wsometimes-uninitialized]
+        if (!file_info.current_path)
+            ^~~~~~~~~~~~~~~~~~~~~~~
+cmd/eficonfig_sbkey.c:294:9: note: uninitialized use occurs here
+        ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
+               ^~~
+cmd/eficonfig_sbkey.c:208:2: note: remove the 'if' if its condition is
always false
+        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
+cmd/eficonfig_sbkey.c:201:18: note: initialize the variable 'ret' to
silence this warning
+        efi_status_t ret;
+                        ^
+                         = 0

> +		goto out;
> +	}
> +
> +	ret = efi_file_read_int(f, &size, buf);
> +	if (ret != EFI_SUCCESS) {
> +		eficonfig_print_msg("ERROR! Failed to read file.");
> +		goto out;
> +	}
> +	if (size == 0) {
> +		eficonfig_print_msg("ERROR! File is empty.");
> +		goto out;
> +	}
> +
> +	/* We expect that file is EFI Signature Lists or signed EFI Signature Lists */
> +	if (!file_have_auth_header(buf, size)) {
> +		if (!file_is_efi_signature_list(buf)) {
> +			eficonfig_print_msg("ERROR! Invalid file format.");
> +			ret = EFI_INVALID_PARAMETER;
> +			goto out;
> +		}
> +
> +		ret = create_time_based_payload(buf, &new_db, &size);
> +		if (ret != EFI_SUCCESS) {
> +			eficonfig_print_msg("ERROR! Failed to create payload with timestamp.");
> +			goto out;
> +		}
> +
> +		free(buf);
> +		buf = new_db;
> +	}
> +
> +	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! Failed to update signature database");
> +		goto out;

This goto is superfluous.

Best regards

Heinrich

> +	}
> +
> +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_menu_items[] = {
> +	{"Enroll New Key", eficonfig_process_enroll_key},
> +	{"Quit", eficonfig_process_quit},
> +};
> +
> +/**
> + * eficonfig_process_set_secure_boot_key() - display the key configuration menu
> + *
> + * @data:	pointer to the data for each entry
> + * Return:	status code
> + */
> +static efi_status_t eficonfig_process_set_secure_boot_key(void *data)
> +{
> +	u32 i;
> +	efi_status_t ret;
> +	char header_str[32];
> +	struct efimenu *efi_menu;
> +
> +	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) {
> +		efi_menu = eficonfig_create_fixed_menu(key_config_menu_items,
> +						       ARRAY_SIZE(key_config_menu_items));
> +
> +		ret = eficonfig_process_common(efi_menu, header_str);
> +		eficonfig_destroy(efi_menu);
> +
> +		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_key, 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} > +};
> +
> +/**
> + * eficonfig_process_secure_boot_config() - display the key list menu
> + *
> + * @data:	pointer to the data for each entry
> + * Return:	status code
> + */
> +efi_status_t eficonfig_process_secure_boot_config(void *data)
> +{
> +	efi_status_t ret;
> +	struct efimenu *efi_menu;
> +
> +	while (1) {
> +		char header_str[64];
> +
> +		snprintf(header_str, sizeof(header_str),
> +			 "  ** UEFI Secure Boot Key Configuration (SecureBoot : %s) **",
> +			 (is_secureboot_enabled() ? "ON" : "OFF"));
> +
> +		efi_menu = eficonfig_create_fixed_menu(secure_boot_menu_items,
> +						       ARRAY_SIZE(secure_boot_menu_items));
> +		if (!efi_menu) {
> +			ret = EFI_OUT_OF_RESOURCES;
> +			break;
> +		}
> +
> +		ret = eficonfig_process_common(efi_menu, header_str);
> +		eficonfig_destroy(efi_menu);
> +
> +		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 86bc801211..6db8e123f0 100644
> --- a/include/efi_config.h
> +++ b/include/efi_config.h
> @@ -99,5 +99,10 @@ efi_status_t eficonfig_append_menu_entry(struct efimenu *efi_menu,
>   					 char *title, eficonfig_entry_func func,
>   					 void *data);
>   efi_status_t eficonfig_append_quit_entry(struct efimenu *efi_menu);
> +void *eficonfig_create_fixed_menu(const struct eficonfig_item *items, int count);
> +
> +#ifdef CONFIG_EFI_SECURE_BOOT
> +efi_status_t eficonfig_process_secure_boot_config(void *data);
> +#endif
>
>   #endif

Re: [PATCH v3 3/6] eficonfig: add UEFI Secure Boot Key enrollment interface

[Heinrich Schuchardt]

On 10/23/22 10:17, Heinrich Schuchardt wrote:
> On 10/14/22 08:56, 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>
>> ---
>> Changes in v3:
>> - fix error handling
>>
>> Changes in v2:
>> - allow to enroll .esl file
>> - fix typos
>> - add function comments
>>
>>   cmd/Makefile          |   3 +
>>   cmd/eficonfig.c       |   3 +
>>   cmd/eficonfig_sbkey.c | 357 ++++++++++++++++++++++++++++++++++++++++++
>>   include/efi_config.h  |   5 +
>>   4 files changed, 368 insertions(+)
>>   create mode 100644 cmd/eficonfig_sbkey.c
>>
>> diff --git a/cmd/Makefile b/cmd/Makefile
>> index c95e09d058..f2f2857146 100644
>> --- a/cmd/Makefile
>> +++ b/cmd/Makefile
>> @@ -66,6 +66,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

Key enrollment does not make sense if the keys are not persisted across
boots. The new feature should depend on CONFIG_EFI_MM_COMM_TEE.

>> +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 0cb0770ac3..a72f07e671 100644
>> --- a/cmd/eficonfig.c
>> +++ b/cmd/eficonfig.c
>> @@ -2442,6 +2442,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..cc27f78e66
>> --- /dev/null
>> +++ b/cmd/eficonfig_sbkey.c
>> @@ -0,0 +1,357 @@
>> +// 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>
>> +
>> +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}, */
>> +};
>> +
>> +/**
>> + * is_secureboot_enabled() - check UEFI Secure Boot is enabled
>> + *
>> + * Return:    true when UEFI Secure Boot is enabled, false otherwise
>> + */
>> +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;
>> +}
>> +
>> +/**
>> + * create_time_based_payload() - create payload for time based
>> authenticate variable
>> + *
>> + * @db:        pointer to the original signature database
>> + * @new_db:    pointer to the authenticated variable payload
>> + * @size:    pointer to payload size
>> + * Return:    status code
>> + */
>> +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;
>> +}
>> +
>> +/**
>> + * file_have_auth_header() - check file has
>> EFI_VARIABLE_AUTHENTICATION_2 header
>> + * @buf:    pointer to file
>> + * @size:    file size
>> + * Return:    true if file has auth header, false otherwise
>> + */
>> +static bool file_have_auth_header(void *buf, efi_uintn_t size)
>> +{
>> +    struct efi_variable_authentication_2 *auth = buf;
>> +
>> +    if (auth->auth_info.hdr.wCertificateType != WIN_CERT_TYPE_EFI_GUID)
>> +        return false;
>> +
>> +    if (guidcmp(&auth->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>> +/**
>> + * file_is_efi_signature_list() - check the file is efi signature list
>> + * @buf:    pointer to file
>> + * Return:    true if file is efi signature list, false otherwise
>> + */
>> +static bool file_is_efi_signature_list(void *buf)
>> +{
>> +    u32 i;
>> +    struct efi_signature_list *sig_list = buf;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(sigtype_to_str); i++) {
>> +        if (!guidcmp(&sig_list->signature_type,
>> &sigtype_to_str[i].sig_type))
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +/**
>> + * eficonfig_process_enroll_key() - enroll key into signature database
>> + *
>> + * @data:    pointer to the data for each entry
>> + * Return:    status code
>> + */
>> +static efi_status_t eficonfig_process_enroll_key(void *data)
>> +{
>> +    u32 attr;
>> +    char *buf = NULL;
>> +    efi_uintn_t size;
>> +    efi_status_t ret;
>> +    void *new_db = NULL;
>> +    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)

ret = EFI_OUT_OF_RESOURCES; is missing here.

Best regards

Heinrich

>> +        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) {
>> +        ret = EFI_OUT_OF_RESOURCES;
>
> This leaves ret undefined and you get an error when compiling with clang:
>
> +cmd/eficonfig_sbkey.c:208:6: error: variable 'ret' is used
> uninitialized whenever 'if' condition is true
> [-Werror,-Wsometimes-uninitialized]
> +        if (!file_info.current_path)
> +            ^~~~~~~~~~~~~~~~~~~~~~~
> +cmd/eficonfig_sbkey.c:294:9: note: uninitialized use occurs here
> +        ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> +               ^~~
> +cmd/eficonfig_sbkey.c:208:2: note: remove the 'if' if its condition is
> always false
> +        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +cmd/eficonfig_sbkey.c:201:18: note: initialize the variable 'ret' to
> silence this warning
> +        efi_status_t ret;
> +                        ^
> +                         = 0
>
>> +        goto out;
>> +    }
>> +
>> +    ret = efi_file_read_int(f, &size, buf);
>> +    if (ret != EFI_SUCCESS) {
>> +        eficonfig_print_msg("ERROR! Failed to read file.");
>> +        goto out;
>> +    }
>> +    if (size == 0) {
>> +        eficonfig_print_msg("ERROR! File is empty.");
>> +        goto out;
>> +    }
>> +
>> +    /* We expect that file is EFI Signature Lists or signed EFI
>> Signature Lists */
>> +    if (!file_have_auth_header(buf, size)) {
>> +        if (!file_is_efi_signature_list(buf)) {
>> +            eficonfig_print_msg("ERROR! Invalid file format.");
>> +            ret = EFI_INVALID_PARAMETER;
>> +            goto out;
>> +        }
>> +
>> +        ret = create_time_based_payload(buf, &new_db, &size);
>> +        if (ret != EFI_SUCCESS) {
>> +            eficonfig_print_msg("ERROR! Failed to create payload with
>> timestamp.");
>> +            goto out;
>> +        }
>> +
>> +        free(buf);
>> +        buf = new_db;
>> +    }
>> +
>> +    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! Failed to update signature
>> database");
>> +        goto out;
>
> This goto is superfluous.
>
> Best regards
>
> Heinrich
>
>> +    }
>> +
>> +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_menu_items[] = {
>> +    {"Enroll New Key", eficonfig_process_enroll_key},
>> +    {"Quit", eficonfig_process_quit},
>> +};
>> +
>> +/**
>> + * eficonfig_process_set_secure_boot_key() - display the key
>> configuration menu
>> + *
>> + * @data:    pointer to the data for each entry
>> + * Return:    status code
>> + */
>> +static efi_status_t eficonfig_process_set_secure_boot_key(void *data)
>> +{
>> +    u32 i;
>> +    efi_status_t ret;
>> +    char header_str[32];
>> +    struct efimenu *efi_menu;
>> +
>> +    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) {
>> +        efi_menu = eficonfig_create_fixed_menu(key_config_menu_items,
>> +                               ARRAY_SIZE(key_config_menu_items));
>> +
>> +        ret = eficonfig_process_common(efi_menu, header_str);
>> +        eficonfig_destroy(efi_menu);
>> +
>> +        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_key, 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} > +};
>> +
>> +/**
>> + * eficonfig_process_secure_boot_config() - display the key list menu
>> + *
>> + * @data:    pointer to the data for each entry
>> + * Return:    status code
>> + */
>> +efi_status_t eficonfig_process_secure_boot_config(void *data)
>> +{
>> +    efi_status_t ret;
>> +    struct efimenu *efi_menu;
>> +
>> +    while (1) {
>> +        char header_str[64];
>> +
>> +        snprintf(header_str, sizeof(header_str),
>> +             "  ** UEFI Secure Boot Key Configuration (SecureBoot :
>> %s) **",
>> +             (is_secureboot_enabled() ? "ON" : "OFF"));
>> +
>> +        efi_menu = eficonfig_create_fixed_menu(secure_boot_menu_items,
>> +                               ARRAY_SIZE(secure_boot_menu_items));
>> +        if (!efi_menu) {
>> +            ret = EFI_OUT_OF_RESOURCES;
>> +            break;
>> +        }
>> +
>> +        ret = eficonfig_process_common(efi_menu, header_str);
>> +        eficonfig_destroy(efi_menu);
>> +
>> +        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 86bc801211..6db8e123f0 100644
>> --- a/include/efi_config.h
>> +++ b/include/efi_config.h
>> @@ -99,5 +99,10 @@ efi_status_t eficonfig_append_menu_entry(struct
>> efimenu *efi_menu,
>>                        char *title, eficonfig_entry_func func,
>>                        void *data);
>>   efi_status_t eficonfig_append_quit_entry(struct efimenu *efi_menu);
>> +void *eficonfig_create_fixed_menu(const struct eficonfig_item *items,
>> int count);
>> +
>> +#ifdef CONFIG_EFI_SECURE_BOOT
>> +efi_status_t eficonfig_process_secure_boot_config(void *data);
>> +#endif
>>
>>   #endif
>

Re: [PATCH v3 3/6] eficonfig: add UEFI Secure Boot Key enrollment interface

[Masahisa Kojima]

On Sun, 23 Oct 2022 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/23/22 10:17, Heinrich Schuchardt wrote:
> > On 10/14/22 08:56, 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>
> >> ---
> >> Changes in v3:
> >> - fix error handling
> >>
> >> Changes in v2:
> >> - allow to enroll .esl file
> >> - fix typos
> >> - add function comments
> >>
> >>   cmd/Makefile          |   3 +
> >>   cmd/eficonfig.c       |   3 +
> >>   cmd/eficonfig_sbkey.c | 357 ++++++++++++++++++++++++++++++++++++++++++
> >>   include/efi_config.h  |   5 +
> >>   4 files changed, 368 insertions(+)
> >>   create mode 100644 cmd/eficonfig_sbkey.c
> >>
> >> diff --git a/cmd/Makefile b/cmd/Makefile
> >> index c95e09d058..f2f2857146 100644
> >> --- a/cmd/Makefile
> >> +++ b/cmd/Makefile
> >> @@ -66,6 +66,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
>
> Key enrollment does not make sense if the keys are not persisted across
> boots. The new feature should depend on CONFIG_EFI_MM_COMM_TEE.

Yes, persistent storage is required.
The problem is that not many platforms support CONFIG_EFI_MM_COMM_TEE and
this secure boot key menu feature can not be tested on sandbox.

>
> >> +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 0cb0770ac3..a72f07e671 100644
> >> --- a/cmd/eficonfig.c
> >> +++ b/cmd/eficonfig.c
> >> @@ -2442,6 +2442,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..cc27f78e66
> >> --- /dev/null
> >> +++ b/cmd/eficonfig_sbkey.c
> >> @@ -0,0 +1,357 @@
> >> +// 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>
> >> +
> >> +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}, */
> >> +};
> >> +
> >> +/**
> >> + * is_secureboot_enabled() - check UEFI Secure Boot is enabled
> >> + *
> >> + * Return:    true when UEFI Secure Boot is enabled, false otherwise
> >> + */
> >> +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;
> >> +}
> >> +
> >> +/**
> >> + * create_time_based_payload() - create payload for time based
> >> authenticate variable
> >> + *
> >> + * @db:        pointer to the original signature database
> >> + * @new_db:    pointer to the authenticated variable payload
> >> + * @size:    pointer to payload size
> >> + * Return:    status code
> >> + */
> >> +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;
> >> +}
> >> +
> >> +/**
> >> + * file_have_auth_header() - check file has
> >> EFI_VARIABLE_AUTHENTICATION_2 header
> >> + * @buf:    pointer to file
> >> + * @size:    file size
> >> + * Return:    true if file has auth header, false otherwise
> >> + */
> >> +static bool file_have_auth_header(void *buf, efi_uintn_t size)
> >> +{
> >> +    struct efi_variable_authentication_2 *auth = buf;
> >> +
> >> +    if (auth->auth_info.hdr.wCertificateType != WIN_CERT_TYPE_EFI_GUID)
> >> +        return false;
> >> +
> >> +    if (guidcmp(&auth->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
> >> +        return false;
> >> +
> >> +    return true;
> >> +}
> >> +
> >> +/**
> >> + * file_is_efi_signature_list() - check the file is efi signature list
> >> + * @buf:    pointer to file
> >> + * Return:    true if file is efi signature list, false otherwise
> >> + */
> >> +static bool file_is_efi_signature_list(void *buf)
> >> +{
> >> +    u32 i;
> >> +    struct efi_signature_list *sig_list = buf;
> >> +
> >> +    for (i = 0; i < ARRAY_SIZE(sigtype_to_str); i++) {
> >> +        if (!guidcmp(&sig_list->signature_type,
> >> &sigtype_to_str[i].sig_type))
> >> +            return true;
> >> +    }
> >> +
> >> +    return false;
> >> +}
> >> +
> >> +/**
> >> + * eficonfig_process_enroll_key() - enroll key into signature database
> >> + *
> >> + * @data:    pointer to the data for each entry
> >> + * Return:    status code
> >> + */
> >> +static efi_status_t eficonfig_process_enroll_key(void *data)
> >> +{
> >> +    u32 attr;
> >> +    char *buf = NULL;
> >> +    efi_uintn_t size;
> >> +    efi_status_t ret;
> >> +    void *new_db = NULL;
> >> +    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)
>
> ret = EFI_OUT_OF_RESOURCES; is missing here.

Yes, this will fix the compilation error in clang.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >> +        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) {
> >> +        ret = EFI_OUT_OF_RESOURCES;
> >
> > This leaves ret undefined and you get an error when compiling with clang:
> >
> > +cmd/eficonfig_sbkey.c:208:6: error: variable 'ret' is used
> > uninitialized whenever 'if' condition is true
> > [-Werror,-Wsometimes-uninitialized]
> > +        if (!file_info.current_path)
> > +            ^~~~~~~~~~~~~~~~~~~~~~~
> > +cmd/eficonfig_sbkey.c:294:9: note: uninitialized use occurs here
> > +        ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> > +               ^~~
> > +cmd/eficonfig_sbkey.c:208:2: note: remove the 'if' if its condition is
> > always false
> > +        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +cmd/eficonfig_sbkey.c:201:18: note: initialize the variable 'ret' to
> > silence this warning
> > +        efi_status_t ret;
> > +                        ^
> > +                         = 0
> >
> >> +        goto out;
> >> +    }
> >> +
> >> +    ret = efi_file_read_int(f, &size, buf);
> >> +    if (ret != EFI_SUCCESS) {
> >> +        eficonfig_print_msg("ERROR! Failed to read file.");
> >> +        goto out;
> >> +    }
> >> +    if (size == 0) {
> >> +        eficonfig_print_msg("ERROR! File is empty.");
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* We expect that file is EFI Signature Lists or signed EFI
> >> Signature Lists */
> >> +    if (!file_have_auth_header(buf, size)) {
> >> +        if (!file_is_efi_signature_list(buf)) {
> >> +            eficonfig_print_msg("ERROR! Invalid file format.");
> >> +            ret = EFI_INVALID_PARAMETER;
> >> +            goto out;
> >> +        }
> >> +
> >> +        ret = create_time_based_payload(buf, &new_db, &size);
> >> +        if (ret != EFI_SUCCESS) {
> >> +            eficonfig_print_msg("ERROR! Failed to create payload with
> >> timestamp.");
> >> +            goto out;
> >> +        }
> >> +
> >> +        free(buf);
> >> +        buf = new_db;
> >> +    }
> >> +
> >> +    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! Failed to update signature
> >> database");
> >> +        goto out;
> >
> > This goto is superfluous.
> >
> > Best regards
> >
> > Heinrich
> >
> >> +    }
> >> +
> >> +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_menu_items[] = {
> >> +    {"Enroll New Key", eficonfig_process_enroll_key},
> >> +    {"Quit", eficonfig_process_quit},
> >> +};
> >> +
> >> +/**
> >> + * eficonfig_process_set_secure_boot_key() - display the key
> >> configuration menu
> >> + *
> >> + * @data:    pointer to the data for each entry
> >> + * Return:    status code
> >> + */
> >> +static efi_status_t eficonfig_process_set_secure_boot_key(void *data)
> >> +{
> >> +    u32 i;
> >> +    efi_status_t ret;
> >> +    char header_str[32];
> >> +    struct efimenu *efi_menu;
> >> +
> >> +    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) {
> >> +        efi_menu = eficonfig_create_fixed_menu(key_config_menu_items,
> >> +                               ARRAY_SIZE(key_config_menu_items));
> >> +
> >> +        ret = eficonfig_process_common(efi_menu, header_str);
> >> +        eficonfig_destroy(efi_menu);
> >> +
> >> +        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_key, 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} > +};
> >> +
> >> +/**
> >> + * eficonfig_process_secure_boot_config() - display the key list menu
> >> + *
> >> + * @data:    pointer to the data for each entry
> >> + * Return:    status code
> >> + */
> >> +efi_status_t eficonfig_process_secure_boot_config(void *data)
> >> +{
> >> +    efi_status_t ret;
> >> +    struct efimenu *efi_menu;
> >> +
> >> +    while (1) {
> >> +        char header_str[64];
> >> +
> >> +        snprintf(header_str, sizeof(header_str),
> >> +             "  ** UEFI Secure Boot Key Configuration (SecureBoot :
> >> %s) **",
> >> +             (is_secureboot_enabled() ? "ON" : "OFF"));
> >> +
> >> +        efi_menu = eficonfig_create_fixed_menu(secure_boot_menu_items,
> >> +                               ARRAY_SIZE(secure_boot_menu_items));
> >> +        if (!efi_menu) {
> >> +            ret = EFI_OUT_OF_RESOURCES;
> >> +            break;
> >> +        }
> >> +
> >> +        ret = eficonfig_process_common(efi_menu, header_str);
> >> +        eficonfig_destroy(efi_menu);
> >> +
> >> +        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 86bc801211..6db8e123f0 100644
> >> --- a/include/efi_config.h
> >> +++ b/include/efi_config.h
> >> @@ -99,5 +99,10 @@ efi_status_t eficonfig_append_menu_entry(struct
> >> efimenu *efi_menu,
> >>                        char *title, eficonfig_entry_func func,
> >>                        void *data);
> >>   efi_status_t eficonfig_append_quit_entry(struct efimenu *efi_menu);
> >> +void *eficonfig_create_fixed_menu(const struct eficonfig_item *items,
> >> int count);
> >> +
> >> +#ifdef CONFIG_EFI_SECURE_BOOT
> >> +efi_status_t eficonfig_process_secure_boot_config(void *data);
> >> +#endif
> >>
> >>   #endif
> >
>

Re: [PATCH v3 0/6] eficonfig: add UEFI Secure Boot key maintenance interface

[Masahisa Kojima]

Hi Heinrich,

On Sun, 23 Oct 2022 at 17:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/23/22 08:13, Masahisa Kojima wrote:
> > Hi Heinrich,
> >
> >
> > On Sat, 22 Oct 2022 at 17:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/14/22 08:56, Masahisa Kojima wrote:
>
> <snip />
>
> >> Python tests with this series fail. See
> >>
> >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/518130
> >
> > Failing item is test_capsule_firmware_raw.py, not eficonfig.
> > test_eficonfig_sbkey.py test is successful.
> >
> > Thanks,
> > Masahisa Kojima
> >
>
> Bisecting showed that the problem was with one of my patches which I
> have corrected now:
>
> https://lists.denx.de/pipermail/u-boot/2022-October/498097.html
> [PATCH v2 1/1] efi_loader: discover if no efi_system_partition is set

Thank you for your check.

>
> Sorry.

No problem.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich

Re: [PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Simon Glass]

Hi Heinrich,

On Wed, 19 Oct 2022 at 15:39, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/19/22 15:17, Simon Glass wrote:
> > iHi Heinrich,
> >
> > On Fri, 14 Oct 2022 at 22:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/15/22 03:10, Simon Glass wrote:
> >>> Hi Ilias,
> >>>
> >>> On Fri, 14 Oct 2022 at 09:59, Ilias Apalodimas
> >>> <ilias.apalodimas@linaro.org> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On Fri, 14 Oct 2022 at 18:56, Simon Glass <sjg@chromium.org> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
> >>>>> <masahisa.kojima@linaro.org> wrote:
> >>>>>>
> >>>>>> Provide a unit test for the eficonfig secure boot key
> >>>>>> management menu.
> >>>>>>
> >>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>>>> ---
> >>>>>> No change since v2
> >>>>>>
> >>>>>> newly created in v2
> >>>>>>
> >>>>>>    test/py/tests/test_eficonfig/conftest.py      |  84 +++-
> >>>>>>    test/py/tests/test_eficonfig/defs.py          |  14 +
> >>>>>>    .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
> >>>>>>    3 files changed, 568 insertions(+), 2 deletions(-)
> >>>>>>    create mode 100644 test/py/tests/test_eficonfig/defs.py
> >>>>>>    create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
> >>>>>
> >>>>> Please can this test be in C? Also, using down-arrow to select menus
> >>>>> is brittle. Add a function to select the one you want, e.g. by name.
> >>>>>
> >>>>
> >>>> Is there a very specific reason why we should do stuff like that in C?
> >>>
> >>> Yes, see here.
> >>>
> >>>>    Python is way easier to extend and test in our case.
> >>>
> >>> In what way? It seems a lot more complicated, plus the brittle nature
> >>> of this test suggests it will be a hassle to maintain.
> >>>
> >>> https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html#python-or-c
> >>>
> >>> There is a pending update here too:
> >>>
> >>> https://patchwork.ozlabs.org/project/uboot/patch/20221013122927.636867-15-sjg@chromium.org/
> >>
> >> The discussion touches different aspects:
> >>
> >> ** What does it take to make a GUI easily testable? **
> >>
> >> Relying on cursor movement for testing is fragile. This is why GUIs
> >> often assign to each executable element the following:
> >>
> >> * Access key, e.g  <CTRL><S>
> >>     A key combination that when entered will have the the same
> >>     effect as selecting the GUI item
> >> * Access string, '@SAVE'
> >>     A string that when typed into the command field will have
> >>     the same effect as selecting the GUI item
> >>
> >> Let's look at U-Boot's menu entry definition:
> >>
> >> struct bootmenu_entry {
> >>       unsigned short int num;      // unique number 0 .. MAX_COUNT
> >>       char key[3];                 // key identifier of number
> >>       char *title;                 // title of entry
> >>       char *command;               // hush command of entry
> >>       enum boot_type type;         // boot type of entry
> >>       u16 bootorder;               // order for each boot type
> >>       struct bootmenu_data *menu;  // this bootmenu
> >>       struct bootmenu_entry *next; // next menu entry (num+1)
> >> }
> >>
> >> Our structure lacks an accessor element that can be used to select a
> >> menu item without using cursor actions.
> >>
> >> Compound keystrokes like <CTRL><F6> are send as multiple bytes on the
> >> console, e.g. 1b 5b 31 37 3b 35 7e.
> >>
> >> We may define a field shortcut of type char *. If the string is received
> >> by the menu loop, let it activate the matching menu entry. Let cursor
> >> actions (up, down, enter, space '+', '-') interrupt matching the
> >> shortcut string.
> >>
> >> Instead we could also use a convention for the title:
> >>
> >> If a letter in the title is preceded by '&', this is the shortcut key.
> >> This letter will be shown highlighted in the menu and the ampersand will
> >> not be shown.
> >>
> >> This is probably easier to implement.
> >>
> >> Adding the shortcut facility will allow both for easier testing and
> >> faster navigation.
> >>
> >> ** Choice of programming language **
> >>
> >> Several aspects control the choice of the programming language for tests:
> >>
> >> - Testing single library functions is only possible in C.
> >> - Checking contents of internal structures is only possible in C.
> >> - Testing the U-Boot's CLI is easily accessible in our Python framework.
> >> - Preparation of complex test data is easier to do in Python.
> >> - Mixed language tests should be avoided if not strictly necessary.
> >>     It is much easier to maintain a single code source for a test.
> >>
> >> ** What to do for secure boot key management? **
> >>
> >> Secure boot key management requires complex preparation which fits well
> >> into out Python testing framework.
> >>
> >> Once we provide shortcut keys to U-Boot menus the entries will be easily
> >> accessible from Python.
> >>
> >> As we want to avoid complexity due to mixed language tests we should
> >> stick to Python for testing key management at the user level.
> >
> > The test setup should be in Python (and needs to be) but the actual
> > test should be in C, in general.
> >
> >>From what I can tell these tests should be split up, one for each test case.
> >
> > Using keyboard shortcuts for menus would certainly help, but you still
> > have the dependency between the menu code in C and the Python code
> > that uses it. The complexity of this dependency is quite significant.
> >
> > Can we not design this all for easier testing? The functionality
> > should be broken down a little more so we can change the configs in a
> > test (e.g. using the eficonfig command), then check that booting does
> > what we expect.
> >
> > Overall I feel that this 'black box' testing is not a good approach
> > and we should try to test the components. It is OK to have a
> > happy-path test but here we are just writing hundreds of lines of
> > Python to get a very high-level signal about correctness.
>
>  From other projects I am used to differentiate between unit tests and
> integration tests.
>
> An integration test is what checks that all the parts work together
> while a unit test checks a single functionality works as designed.
>
> A description can be found in
> https://circleci.com/blog/unit-testing-vs-integration-testing/#c-consent-modal
>
> For an integration test this Python script is fine (except for the menu
> navigation.
>
> As I said in my last mail additional tests on a lower level may be
> added. But unit tests cannot replace an integration test.

We should do the C test first, then. It just has so much more
visibility into what is going on. I am OK with an integration test,
but it should never find a bug that is not found by the sandbox unit
test. The Python tests are much harder to debug!

So let's get this sorted out so we don't end up with a complete mess here.

Regards,
Simon