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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Sun, Jun 19, 2022 at 02:20:20PM +0900, Masahisa Kojima wrote:
> This commit adds the menu-driven UEFI Secure Boot Key
> enrollment interface. User can enroll the PK, KEK, db
> and dbx by selecting EFI Signature Lists file.
> After the PK is enrolled, UEFI Secure Boot is enabled and
> EFI Signature Lists file must be signed by KEK or PK.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  cmd/Makefile          |   3 +
>  cmd/eficonfig.c       |   3 +
>  cmd/eficonfig_sbkey.c | 202 ++++++++++++++++++++++++++++++++++++++++++
>  include/efi_config.h  |   3 +
>  4 files changed, 211 insertions(+)
>  create mode 100644 cmd/eficonfig_sbkey.c
> 
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 0afa687e94..9d87b639fc 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -64,6 +64,9 @@ obj-$(CONFIG_CMD_EEPROM) += eeprom.o
>  obj-$(CONFIG_EFI) += efi.o
>  obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o
>  obj-$(CONFIG_CMD_EFICONFIG) += eficonfig.o
> +ifdef CONFIG_CMD_EFICONFIG
> +obj-$(CONFIG_EFI_SECURE_BOOT) += eficonfig_sbkey.o
> +endif
>  obj-$(CONFIG_CMD_ELF) += elf.o
>  obj-$(CONFIG_CMD_EROFS) += erofs.o
>  obj-$(CONFIG_HUSH_PARSER) += exit.o
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index e62f5e41a4..e6d2cba9c5 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -1832,6 +1832,9 @@ static const struct eficonfig_item maintenance_menu_items[] = {
>  	{"Edit Boot Option", eficonfig_process_edit_boot_option},
>  	{"Change Boot Order", eficonfig_process_change_boot_order},
>  	{"Delete Boot Option", eficonfig_process_delete_boot_option},
> +#if (CONFIG_IS_ENABLED(EFI_SECURE_BOOT))
> +	{"Secure Boot Configuration", eficonfig_process_secure_boot_config},
> +#endif
>  	{"Quit", eficonfig_process_quit},
>  };
>  
> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> new file mode 100644
> index 0000000000..a5c0dbe9b3
> --- /dev/null
> +++ b/cmd/eficonfig_sbkey.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  Menu-driven UEFI Secure Boot key maintenance
> + *
> + *  Copyright (c) 2022 Masahisa Kojima, Linaro Limited
> + */
> +
> +#include <ansi.h>
> +#include <common.h>
> +#include <charset.h>
> +#include <hexdump.h>
> +#include <log.h>
> +#include <malloc.h>
> +#include <menu.h>
> +#include <efi_loader.h>
> +#include <efi_config.h>
> +#include <efi_variable.h>
> +#include <crypto/pkcs7_parser.h>
> +
> +static bool is_secureboot_enabled(void)
> +{
> +	efi_status_t ret;
> +	u8 secure_boot;
> +	efi_uintn_t size;
> +
> +	size = sizeof(secure_boot);
> +	ret = efi_get_variable_int(u"SecureBoot", &efi_global_variable_guid,
> +				   NULL, &size, &secure_boot, NULL);
> +
> +	return secure_boot == 1;
> +}
> +
> +static efi_status_t eficonfig_process_enroll_key(void *data)
> +{
> +	u32 attr;
> +	char *buf = NULL;
> +	efi_uintn_t size;
> +	efi_status_t ret;
> +	struct efi_file_handle *f;
> +	struct efi_file_handle *root;
> +	struct eficonfig_select_file_info file_info;
> +
> +	file_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
> +	if (!file_info.current_path)
> +		goto out;
> +
> +	ret = eficonfig_select_file_handler(&file_info);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = efi_open_volume_int(file_info.current_volume, &root);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = efi_file_open_int(root, &f, file_info.current_path, EFI_FILE_MODE_READ, 0);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	size = 0;
> +	ret = EFI_CALL(f->getinfo(f, &efi_file_info_guid, &size, NULL));
> +	if (ret != EFI_BUFFER_TOO_SMALL)
> +		goto out;
> +
> +	buf = calloc(1, size);
> +	if (!buf) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out;
> +	}
> +	ret = EFI_CALL(f->getinfo(f, &efi_file_info_guid, &size, buf));
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	size = ((struct efi_file_info *)buf)->file_size;
> +	free(buf);

You should set buf to NULL here. 

> +
> +	buf = calloc(1, size);
> +	if (!buf)
> +		goto out;
> +
> +	ret = efi_file_read_int(f, &size, buf);
> +	if (ret != EFI_SUCCESS || size == 0)
> +		goto out;
> +
> +	attr = EFI_VARIABLE_NON_VOLATILE |
> +	       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +	       EFI_VARIABLE_RUNTIME_ACCESS |
> +	       EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
> +	/* PK can enroll only one certificate */
> +	if (u16_strcmp(data, u"PK")) {
> +		efi_uintn_t db_size = 0;
> +
> +		/* check the variable exists. If exists, add APPEND_WRITE attribute */
> +		ret = efi_get_variable_int(data, efi_auth_var_get_guid(data), NULL,
> +					   &db_size, NULL,  NULL);
> +		if (ret == EFI_BUFFER_TOO_SMALL)
> +			attr |= EFI_VARIABLE_APPEND_WRITE;
> +	}
> +

Why are we appending? Shouldn't we always overwrite the platform key?

> +	ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
> +				   attr, size, buf, false);
> +	if (ret != EFI_SUCCESS) {
> +		eficonfig_print_msg("ERROR! Fail to update signature database");
> +		goto out;
> +	}
> +
> +out:
> +	free(file_info.current_path);
> +	free(buf);
> +
> 
[...]

Thanks
/Ilias