On 7/8/22 11:14, Ilias Apalodimas wrote:
On Sun, Jun 19, 2022 at 02:20:20PM +0900, Masahisa Kojima wrote:
This commit adds the menu-driven UEFI Secure Boot Key
enrollment interface. User can enroll the PK, KEK, db
and dbx by selecting EFI Signature Lists file.
After the PK is enrolled, UEFI Secure Boot is enabled and
EFI Signature Lists file must be signed by KEK or PK.
Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org>
---
cmd/Makefile | 3 +
cmd/eficonfig.c | 3 +
cmd/eficonfig_sbkey.c | 202 ++++++++++++++++++++++++++++++++++++++++++
include/efi_config.h | 3 +
4 files changed, 211 insertions(+)
create mode 100644 cmd/eficonfig_sbkey.c
diff --git a/cmd/Makefile b/cmd/Makefile
index 0afa687e94..9d87b639fc 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -64,6 +64,9 @@ obj-$(CONFIG_CMD_EEPROM) += eeprom.o
obj-$(CONFIG_EFI) += efi.o
obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o
obj-$(CONFIG_CMD_EFICONFIG) += eficonfig.o
+ifdef CONFIG_CMD_EFICONFIG
+obj-$(CONFIG_EFI_SECURE_BOOT) += eficonfig_sbkey.o
+endif
obj-$(CONFIG_CMD_ELF) += elf.o
obj-$(CONFIG_CMD_EROFS) += erofs.o
obj-$(CONFIG_HUSH_PARSER) += exit.o
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index e62f5e41a4..e6d2cba9c5 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -1832,6 +1832,9 @@ static const struct eficonfig_item
maintenance_menu_items[] = {
{"Edit Boot Option", eficonfig_process_edit_boot_option},
{"Change Boot Order", eficonfig_process_change_boot_order},
{"Delete Boot Option", eficonfig_process_delete_boot_option},
+#if (CONFIG_IS_ENABLED(EFI_SECURE_BOOT))
+ {"Secure Boot Configuration", eficonfig_process_secure_boot_config},
+#endif
{"Quit", eficonfig_process_quit},
};
diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
new file mode 100644
index 0000000000..a5c0dbe9b3
--- /dev/null
+++ b/cmd/eficonfig_sbkey.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Menu-driven UEFI Secure Boot key maintenance
+ *
+ * Copyright (c) 2022 Masahisa Kojima, Linaro Limited
+ */
+
+#include <ansi.h>
+#include <common.h>
+#include <charset.h>
+#include <hexdump.h>
+#include <log.h>
+#include <malloc.h>
+#include <menu.h>
+#include <efi_loader.h>
+#include <efi_config.h>
+#include <efi_variable.h>
+#include <crypto/pkcs7_parser.h>
+
Please, provide function descriptions.
+static bool is_secureboot_enabled(void)
+{
+ efi_status_t ret;
+ u8 secure_boot;
+ efi_uintn_t size;
+
+ size = sizeof(secure_boot);
+ ret = efi_get_variable_int(u"SecureBoot", &efi_global_variable_guid,
+ NULL, &size, &secure_boot, NULL);
+
+ return secure_boot == 1;
+}
+
+static efi_status_t eficonfig_process_enroll_key(void *data)
+{
+ u32 attr;
+ char *buf = NULL;
+ efi_uintn_t size;
+ efi_status_t ret;
+ struct efi_file_handle *f;
+ struct efi_file_handle *root;
+ struct eficonfig_select_file_info file_info;
+
+ file_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
+ if (!file_info.current_path)
+ goto out;
+
+ ret = eficonfig_select_file_handler(&file_info);
+ if (ret != EFI_SUCCESS)
+ goto out;
+
+ ret = efi_open_volume_int(file_info.current_volume, &root);
+ if (ret != EFI_SUCCESS)
+ goto out;
+
+ ret = efi_file_open_int(root, &f, file_info.current_path,
EFI_FILE_MODE_READ, 0);
+ if (ret != EFI_SUCCESS)
+ goto out;
+
+ size = 0;
+ ret = EFI_CALL(f->getinfo(f, &efi_file_info_guid, &size, NULL));
+ if (ret != EFI_BUFFER_TOO_SMALL)
+ goto out;
+
+ buf = calloc(1, size);
+ if (!buf) {
+ ret = EFI_OUT_OF_RESOURCES;
+ goto out;
+ }
+ ret = EFI_CALL(f->getinfo(f, &efi_file_info_guid, &size, buf));
+ if (ret != EFI_SUCCESS)
+ goto out;
+
+ size = ((struct efi_file_info *)buf)->file_size;
+ free(buf);
You should set buf to NULL here.
Assigning NULL would have no effect. The variable is reassigned in the
next line.
+
+ buf = calloc(1, size);
+ if (!buf)
+ goto out;
+
+ ret = efi_file_read_int(f, &size, buf);
+ if (ret != EFI_SUCCESS || size == 0)
+ goto out;
+
+ attr = EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
+ /* PK can enroll only one certificate */
+ if (u16_strcmp(data, u"PK")) {
+ efi_uintn_t db_size = 0;
+
+ /* check the variable exists. If exists, add APPEND_WRITE
attribute */
+ ret = efi_get_variable_int(data, efi_auth_var_get_guid(data),
NULL,
+ &db_size, NULL, NULL);
+ if (ret == EFI_BUFFER_TOO_SMALL)
+ attr |= EFI_VARIABLE_APPEND_WRITE;
+ }
+
Why are we appending? Shouldn't we always overwrite the platform key?
The UEFI specification says:
"The PK variable contains *the* current Platform Key."
So there should always be only one key in the variable.
+ ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16
*)data),
+ attr, size, buf, false);
+ if (ret != EFI_SUCCESS) {
+ eficonfig_print_msg("ERROR! Fail to update signature database");
*%s/Fail/Failed/
Please, add unit tests for your patches.
My expectation is that efi_set_variable_int() will only succeed if the
variable change request is signed with the old PK or if PK does not exist.
Under which circumstances shall a board owner be able to remove PK if he
does not possess the private key?
Best regards
Heinrich
+ goto out;
+ }
+
+out:
+ free(file_info.current_path);
+ free(buf);
+
[...]
Thanks
/Ilias
