On Sat, 19 Nov 2022 at 02:30, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > On 11/18/22 10:37, Masahisa Kojima wrote: > > Hi Heinrhch, > > > > On Fri, 18 Nov 2022 at 08:07, Heinrich Schuchardt <xypron.g...@gmx.de> > > wrote: > >> > >> On 11/16/22 11:28, 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> > >>> Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org> > >>> --- > >>> Changes in v9: > >>> - move file size check, set ret = EFI_INVALID_PARAMETER > >>> > >>> Changes in v8: > >>> - fix missing efi_file_close_int() call > >>> > >>> 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 | 246 ++++++++++++++++++++++++++++++++++++++++++ > >>> include/efi_config.h | 5 + > >>> 4 files changed, 259 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..e9aaf76bf8 > >>> --- /dev/null > >>> +++ b/cmd/eficonfig_sbkey.c > >>> @@ -0,0 +1,246 @@ > >>> +// 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) > >> > >> The parameter should be u16 *data. You can be more specific than the > >> eficonfig_entry_func typedef here. > > > > I think we can't use u16* data here. > > This is the callback function called from eficonfig_process_common() > > when the entry is selected, and it is a generic eficonfig_entry_func > > function pointer. > > > >> > >>> +{ > >>> + u32 attr; > >>> + char *buf = NULL; > >>> + efi_uintn_t size; > >>> + efi_status_t ret; > >>> + struct efi_file_handle *root; > >>> + struct efi_file_handle *f = NULL; > >>> + struct eficonfig_select_file_info file_info; > >>> + > >>> + file_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE); > >> > >> The FAT specification has no maximum file path length. Windows 10 made > >> the 260 character limit for file paths a group policy choice. Why should > >> we add such a constraint? > > > > eficonfig_process_select_file() is commonly used when user selects the > > file as UEFI load option. > > In UEFI load option, U-Boot efi_convert_device_path_to_text() > > implementation has a file path limit by > > MAX_NODE_LEN(512). > > The file path size limit in eficonfig is derived from it. > > > >> > >>> + if (!file_info.current_path) { > >>> + ret = EFI_OUT_OF_RESOURCES; > >>> + goto out; > >>> + } > >>> + > >>> + ret = eficonfig_process_select_file(&file_info); > >> > >> Please, fix the signature of eficonfig_process_select_file(). > >> > >> If a function expects struct *eficonfig_select_file_info, it should not > >> have a void * parameter for it. > > > > Same as eficonfig_process_enroll_key() above, I think we can only use > > void* here. > > > >> > >>> + 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; > >>> + > >>> + ret = efi_file_size(f, &size); > >>> + if (ret != EFI_SUCCESS) > >>> + goto out; > >>> + > >>> + if (!size) { > >>> + eficonfig_print_msg("ERROR! File is empty."); > >>> + ret = EFI_INVALID_PARAMETER; > >>> + goto out; > >>> + } > >> > >> Move the non-zero file size check to the file chooser. Choosing a file > >> and afterwards being informed that it should never have been listed as a > >> choice is just frustrating. > > > > OK, I will move size check ineficonfig_process_select_file().
I'm not sure whether eliminating the empty file from the file list is good or not, and adding size check in eficonfig_process_select_file() does not change the user experience(choose file and afterwards being informed that file is empty). Let me keep an empty file check here. > > > >> > >>> + > >>> + buf = calloc(1, size); > >> > >> Why should we zero out the buffer? > >> > >>> + if (!buf) { > >>> + ret = EFI_OUT_OF_RESOURCES; > >>> + goto out; > >>> + } > >>> + > >>> + ret = efi_file_read_int(f, &size, buf); > >> Don't assume that the driver for the simple file protocol is provided by > >> U-Boot. It could be provided by a boot service driver that you loaded > >> via the bootefi command before invoking the eficonfig command. > > > > OK, I will call EFI_CALL(f->read()) instead. > > This is not the only place to change. You have to avoid > efi_open_volume_int, efi_file_open_int, efi_file_size. Not only here but > also inside eficonfig_select_file_handler. Yes, I will also update. Thanks, Masahisa kojima > > Best regards > > Heinrich > > > > Together with this modification, it is better that the opening file is > > replaced with > > efi_file_from_path() as Ilias suggested before. > > > > Thanks, > > Masahisa Kojima > > > >> > >> Best regards > >> > >> Heinrich > >> > >>> + 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); > >>> + if (f) > >>> + efi_file_close_int(f); > >>> + > >>> + /* 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) > >>> +{ > >>> + 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; > >>> + } > >>> + > >>> + /* return to 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) **", > >>> + (efi_secure_boot_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; > >>> + } > >>> + > >>> + /* return to 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 064f2efe3f..7a02fc68ac 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 > >> >