On 11/28/18 7:00 AM, AKASHI Takahiro wrote:

An EFI variable is nothing but a wrapper of a corresponding u-boot environment variable (See efi_variable.c), but under the current implementation, NON_VOLATILE attribute is not honored while u-boot environment variables can be saved/restored in storage.

With this patch, the expected semantics will be mimicked by deleting all the EFI variables *without* NON_VOLATILE attribute when loading them from storage at boot time.

This patch would only be needed if we would store non-volatile variables. So here you try to fix an error after it has happened instead of stopping it from happening.

EFI variables are not really useful unless we can change them from the operating system. Saving them via a driver that is not available at operating system runtime does not lead anywhere.

So I would recommend not to follow this route anymore.

Best regards

Heinrich

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

env/env.c | 4 +++ include/efi_loader.h | 1 + lib/efi_loader/efi_variable.c | 64 +++++++++++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/env/env.c b/env/env.c index afed0f3c95c3..c507a4ac5f78 100644 --- a/env/env.c +++ b/env/env.c @@ -5,6 +5,7 @@ */

#include <common.h> +#include <efi_loader.h> #include <environment.h>

DECLARE_GLOBAL_DATA_PTR; @@ -195,6 +196,9 @@ int env_load(void) if (ret) { debug("Failed (%d)\n", ret); } else { +#ifdef CONFIG_EFI_LOADER

• 	efi_purge_volatile_variables();
  

+#endif printf("OK\n"); return 0; } diff --git a/include/efi_loader.h b/include/efi_loader.h index 9f7a4068efa6..9cad1dcd62bb 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -533,6 +533,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, void *data); +int efi_purge_volatile_variables(void);

/*

• See section 3.1.3 in the v2.7 UEFI spec for more details on

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 19d9cb865f25..ad8cd36fa1e1 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -8,6 +8,8 @@ #include <malloc.h> #include <charset.h> #include <efi_loader.h> +#include <environment.h> +#include <search.h>

#define READ_ONLY BIT(31)

@@ -142,6 +144,8 @@ static const char *parse_attr(const char *str, u32 *attrp)

if ((s = prefix(str, "ro"))) {
	attr |= READ_ONLY;

• } else if ((s = prefix(str, "nv"))) {
  

• 	attr |= EFI_VARIABLE_NON_VOLATILE;
  

  } else if ((s = prefix(str, "boot"))) { attr |= EFI_VARIABLE_BOOTSERVICE_ACCESS; } else if ((s = prefix(str, "run"))) {

@@ -293,7 +297,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor, } }

• val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1);

• val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1); if (!val) { ret = EFI_OUT_OF_RESOURCES; goto out;

@@ -302,12 +306,16 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_guid_t *vendor, s = val;

/* store attributes: */

• attributes &= (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS);

• attributes &= (EFI_VARIABLE_NON_VOLATILE |

•        EFI_VARIABLE_BOOTSERVICE_ACCESS |
  

•        EFI_VARIABLE_RUNTIME_ACCESS);
  

  s += sprintf(s, "{"); while (attributes) { u32 attr = 1 << (ffs(attributes) - 1);

• if (attr == EFI_VARIABLE_BOOTSERVICE_ACCESS)
  

• if (attr == EFI_VARIABLE_NON_VOLATILE)
  

• 	s += sprintf(s, "nv");
  

• else if (attr == EFI_VARIABLE_BOOTSERVICE_ACCESS)
  s += sprintf(s, "boot");
  

  else if (attr == EFI_VARIABLE_RUNTIME_ACCESS) s += sprintf(s, "run");

@@ -334,3 +342,53 @@ out:

return EFI_EXIT(ret); }

+/*

• • Purge all the variables which are not marked non volatile.
• • This function is assumed to be called only once at boot time.
• */

+int efi_purge_volatile_variables(void) +{

• char regex[256];
• char * const regexlist[] = {regex};
• char *list = NULL, *name, *value;
• int len, ret = 0;
• u32 attr;
• snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");
• len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,

• 	&list, 0, 1, regexlist);
  

• if (len < 0)

• return -1;
  

• else if (!len)

• return 0;
  

• name = list;
• while (*name) {

• /* variable name */
  

• value = strchr(name, '=');
  

• if (!value)
  

• 	break;
  

• *value = '\0';
  

• value++;
  

• parse_attr(value, &attr);
  

• if (!(attr & EFI_VARIABLE_NON_VOLATILE)) {
  

• 	if (env_set(name, NULL)) {
  

• 		printf("cannot purge efi variable: %s\n", name);
  

• 		ret = -1;
  

• 	}
  

• }
  

• name = strchr(value, '\n');
  

• if (!name)
  

• 	break;
  

• name++;
  

• }
• free(list);
• return ret;

+}

On 11/28/18 7:00 AM, AKASHI Takahiro wrote:

With -nv specified, a variable to be created will have NON_VOLATILE attribute.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

Why would we only make the NON_VOLATILE attribute available and not all the other attributes like EFI_VARIABLE_BOOTSERVICE_ACCESS?

Where is the patch that ensures that non-volatile variables are not stored?

Currently we do not make EFI variables available to the operating system so they are not really of much use.

Best regards

Heinrich

---

cmd/efishell.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/cmd/efishell.c b/cmd/efishell.c index 7cd3ca489559..7cdff757b06c 100644 --- a/cmd/efishell.c +++ b/cmd/efishell.c @@ -249,11 +249,22 @@ static int _do_efi_set_var(int argc, char * const argv[]) unsigned long size = 0; u16 *var_name16, *p; efi_guid_t guid;

• u32 attributes; efi_status_t ret;

  if (argc == 1) return CMD_RET_SUCCESS;

• attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |

•      EFI_VARIABLE_RUNTIME_ACCESS;
  

• if (!strcmp(argv[1], "-nv")) {

• attributes |= EFI_VARIABLE_NON_VOLATILE;
  

• argc--;
  

• argv++;
  

• if (argc == 1)
  

• 	return CMD_RET_SUCCESS;
  

• }

• var_name = argv[1]; if (argc == 2) { /* remove */

@@ -275,9 +286,7 @@ static int _do_efi_set_var(int argc, char * const argv[]) utf8_utf16_strncpy(&p, var_name, strlen(var_name) + 1);

guid = efi_global_variable_guid;

• ret = efi_set_variable(var_name16, &guid,

• 	       EFI_VARIABLE_BOOTSERVICE_ACCESS |
  

• 	       EFI_VARIABLE_RUNTIME_ACCESS, size, value);
  

• ret = efi_set_variable(var_name16, &guid, attributes, size, value); ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);

out: return ret; @@ -978,7 +987,7 @@ static char efishell_help_text[] = "\n" "efishell dumpvar [<name>]\n" " - get uefi variable's value\n"

• "efishell setvar <name> [<value>]\n"

• "efishell setvar [-nv] <name> [<value>]\n" " - set/delete uefi variable's value\n" " <value> may be "="..."", "=0x..." (set) or "=" (delete)\n" "efishell devices\n"

On 11/28/18 7:00 AM, AKASHI Takahiro wrote:

See UEFI specification v2.7a, section 3.3 for details attributes.

I cannot find file cmd/efishell.c in U-Boot. And I cannot find it in https://patchwork.ozlabs.org/project/uboot/list/?submitter=61166

So I have no clue what this patch is based on.

Best regards

Heinrich

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/efishell.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/cmd/efishell.c b/cmd/efishell.c index 7cdff757b06c..a594278a04fe 100644 --- a/cmd/efishell.c +++ b/cmd/efishell.c @@ -641,6 +641,7 @@ static int do_efi_boot_add(int argc, char * const argv[]) }

ret = efi_set_variable(var_name16, &guid,

• 	       EFI_VARIABLE_NON_VOLATILE |
         EFI_VARIABLE_BOOTSERVICE_ACCESS |
         EFI_VARIABLE_RUNTIME_ACCESS, size, data);
  

  ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);

@@ -676,6 +677,7 @@ static int do_efi_boot_rm(int argc, char * const argv[]) utf8_utf16_strncpy((u16 **)&var_name16, var_name, 9);

ret = efi_set_variable(var_name16, &guid,

• 		       EFI_VARIABLE_NON_VOLATILE |
  	       EFI_VARIABLE_BOOTSERVICE_ACCESS |
  	       EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL);
  

  if (ret) {

@@ -898,6 +900,7 @@ static int do_efi_boot_order(int argc, char * const argv[])

guid = efi_global_variable_guid; ret = efi_set_variable(L"BootOrder", &guid,

• 	       EFI_VARIABLE_NON_VOLATILE |
         EFI_VARIABLE_BOOTSERVICE_ACCESS |
         EFI_VARIABLE_RUNTIME_ACCESS, size, bootorder);
  

  ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);

On 11/28/18 7:00 AM, AKASHI Takahiro wrote:

See UEFI specification v2.7a, section 3.3 for details attributes.

This patch is not applicable to U-Boot master not to efi-next. There is no preceding patch in https://patchwork.ozlabs.org/project/uboot/list/?submitter=61166

I have no clue what this patch is based on.

Best regards

Heinrich

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

lib/efi_loader/efi_bootmgr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index db391147fb2d..128d1e887cb4 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -210,7 +210,8 @@ void *efi_bootmgr_load(int boot_id, if (!bootnext) goto run_list;

• attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |

• attributes = EFI_VARIABLE_NON_VOLATILE |

•      EFI_VARIABLE_BOOTSERVICE_ACCESS |
  EFI_VARIABLE_RUNTIME_ACCESS;
  

  size = 0; ret = rs->set_variable(L"BootNext",