Hi Kojima-san
On Wed, Nov 09, 2022 at 12:37:27PM +0900, Masahisa Kojima wrote: > This commit adds the menu-driven UEFI Secure Boot Key > enrollment interface. User can enroll PK, KEK, db > and dbx by selecting file. > Only the signed EFI Signature List(s) with an authenticated > header, typically '.auth' file, is accepted. > > To clear the PK, KEK, db and dbx, user needs to enroll the null key > signed by PK or KEK. > > Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org> > --- > Changes in v7: > - only accept .auth file. > - remove creating time based authenticated variable > - update commit message > - use efi_file_size() > > Changes in v6: > - use efi_secure_boot_enabled() > - replace with WIN_CERT_REVISION_2_0 from pe.h > - call efi_build_signature_store() to check the valid EFI Signature List > - update comment > > Changes in v4: > - add CONFIG_EFI_MM_COMM_TEE dependency > - fix error handling > > Changes in v3: > - fix error handling > > Changes in v2: > - allow to enroll .esl file > - fix typos > - add function comments > > cmd/Makefile | 5 + > cmd/eficonfig.c | 3 + > cmd/eficonfig_sbkey.c | 242 ++++++++++++++++++++++++++++++++++++++++++ > include/efi_config.h | 5 + > 4 files changed, 255 insertions(+) > create mode 100644 cmd/eficonfig_sbkey.c > > diff --git a/cmd/Makefile b/cmd/Makefile > index 2444d116c0..0b6a96c1d9 100644 > --- a/cmd/Makefile > +++ b/cmd/Makefile > @@ -66,6 +66,11 @@ 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 > +ifdef CONFIG_EFI_MM_COMM_TEE > +obj-$(CONFIG_EFI_SECURE_BOOT) += eficonfig_sbkey.o > +endif > +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 12babb76c2..d79194794b 100644 > --- a/cmd/eficonfig.c > +++ b/cmd/eficonfig.c > @@ -2435,6 +2435,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) && > CONFIG_IS_ENABLED(EFI_MM_COMM_TEE)) > + {"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..1e9eb3f51e > --- /dev/null > +++ b/cmd/eficonfig_sbkey.c > @@ -0,0 +1,242 @@ > +// 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}, > */ > +}; > + > +/** > + * 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; > +} > + > +/** > + * 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; > + 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; > + goto out; > + } > + > + ret = eficonfig_process_select_file(&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; I think it would be better here if we could use efi_file_from_path(). I think we can't easily do that atm since we can't convert the filename to a device path with efi_dp_from_file() since we don't have the block info. Since that requires a further clean up, I am fine keeping it as-is for now, but add a comment saying we should replace that with efi_file_from_path() eventually. > + > + ret = efi_file_size(f, &size); > + if (ret != EFI_SUCCESS) > + goto out; > + > + buf = calloc(1, size); > + if (!buf) { > + ret = EFI_OUT_OF_RESOURCES; > + goto out; > + } > + if (size == 0) { > + eficonfig_print_msg("ERROR! File is empty."); > + 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 (!file_have_auth_header(buf, size)) { > + eficonfig_print_msg("ERROR! Invalid file format. Only .auth > variables is allowed."); > + ret = EFI_INVALID_PARAMETER; > + goto out; > + } > + > + attr = EFI_VARIABLE_NON_VOLATILE | > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_RUNTIME_ACCESS | > + EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS; > + > + /* PK can enroll only one certificate */ > + if (u16_strcmp(data, u"PK")) { > + efi_uintn_t db_size = 0; > + > + /* check the variable exists. If exists, add APPEND_WRITE > attribute */ > + ret = efi_get_variable_int(data, efi_auth_var_get_guid(data), > NULL, > + &db_size, NULL, NULL); > + if (ret == EFI_BUFFER_TOO_SMALL) > + attr |= EFI_VARIABLE_APPEND_WRITE; > + } > + > + ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 > *)data), > + attr, size, buf, false); > + if (ret != EFI_SUCCESS) > + eficonfig_print_msg("ERROR! Failed to update signature > database"); > + > +out: > + free(file_info.current_path); > + free(buf); Shouldn't we close the file handle here as well? > + > + /* return to 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) [...]