[v9,4/5] eficonfig: add UEFI Secure Boot Key enrollment interface

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.kojima@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@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

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.kojima@linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@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.

> +{
> +	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?

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

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

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

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

Hi Heinrhch,

On Fri, 18 Nov 2022 at 08:07, Heinrich Schuchardt <xypron.glpk@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.kojima@linaro.org>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@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().

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

On 11/18/22 10:37, Masahisa Kojima wrote:
> Hi Heinrhch,
>
> On Fri, 18 Nov 2022 at 08:07, Heinrich Schuchardt <xypron.glpk@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.kojima@linaro.org>
>>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@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().
>
>>
>>> +
>>> +     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.

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

On Sat, 19 Nov 2022 at 02:30, Heinrich Schuchardt <xypron.glpk@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.glpk@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.kojima@linaro.org>
> >>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@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
> >>
>