[U-Boot] [PATCH v2 0/3] efi_loader: variable: attributes may not be changed if a variable exists

[U-Boot] [PATCH v2 0/3] efi_loader: variable: attributes may not be changed if a variable exists

[AKASHI Takahiro]

Changes in v2
* add patch#1 and patch#2 to avoid sefltest failure
* doesn't free a buffer returned by env_get() (patch#3)

AKASHI Takahiro (3):
  efi: selftest: APPEND_WRITE is not supported
  efi_loader: variable: return error for APPEND_WRITE
  efi_loader: variable: attributes may not be changed if a variable
    exists

 lib/efi_loader/efi_variable.c             | 16 ++++++++++---
 lib/efi_selftest/efi_selftest_variables.c | 28 +++++++++++------------
 2 files changed, 27 insertions(+), 17 deletions(-)

-- 
2.21.0

[U-Boot] [PATCH v2 1/3] efi: selftest: APPEND_WRITE is not supported

[AKASHI Takahiro]

The error here should be marked *todo*.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_selftest/efi_selftest_variables.c | 28 +++++++++++------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_variables.c b/lib/efi_selftest/efi_selftest_variables.c
index b028c64bbc85..06c1a032dd04 100644
--- a/lib/efi_selftest/efi_selftest_variables.c
+++ b/lib/efi_selftest/efi_selftest_variables.c
@@ -116,21 +116,21 @@ static int execute(void)
 				    EFI_VARIABLE_APPEND_WRITE,
 				    7, v + 8);
 	if (ret != EFI_SUCCESS) {
-		efi_st_error("SetVariable failed\n");
-		return EFI_ST_FAILURE;
-	}
-	len = EFI_ST_MAX_DATA_SIZE;
-	ret = runtime->get_variable(L"efi_st_var1", &guid_vendor1,
-				    &attr, &len, data);
-	if (ret != EFI_SUCCESS) {
-		efi_st_error("GetVariable failed\n");
-		return EFI_ST_FAILURE;
+		efi_st_todo("SetVariable(APPEND_WRITE) failed\n");
+	} else {
+		len = EFI_ST_MAX_DATA_SIZE;
+		ret = runtime->get_variable(L"efi_st_var1", &guid_vendor1,
+					    &attr, &len, data);
+		if (ret != EFI_SUCCESS) {
+			efi_st_error("GetVariable failed\n");
+			return EFI_ST_FAILURE;
+		}
+		if (len != 15)
+			efi_st_todo("GetVariable returned wrong length %u\n",
+				    (unsigned int)len);
+		if (memcmp(data, v, len))
+			efi_st_todo("GetVariable returned wrong value\n");
 	}
-	if (len != 15)
-		efi_st_todo("GetVariable returned wrong length %u\n",
-			    (unsigned int)len);
-	if (memcmp(data, v, len))
-		efi_st_todo("GetVariable returned wrong value\n");
 	/* Enumerate variables */
 	boottime->set_mem(&guid, 16, 0);
 	*varname = 0;
-- 
2.21.0

[U-Boot] [PATCH v2 2/3] efi_loader: variable: return error for APPEND_WRITE

[AKASHI Takahiro]

The current efi_et_variable() doesn't support EFI_VARIABLE_APPEND_WRITE
attiribut for now, and so should return an error.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_variable.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 28b1aa7505ae..e3ec502ffb45 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -427,7 +427,8 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
 		  data_size, data);
 
-	if (!variable_name || !vendor) {
+	if (!variable_name || !vendor ||
+	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
 		ret = EFI_INVALID_PARAMETER;
 		goto out;
 	}
-- 
2.21.0

[U-Boot] [PATCH v2 3/3] efi_loader: variable: attributes may not be changed if a variable exists

[AKASHI Takahiro]

If a variable already exists, efi_set_variable() should not change
the variable's attributes. This patch enforces it.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_variable.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index e3ec502ffb45..1bb3bbf3393e 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -450,12 +450,21 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 	if (val) {
 		parse_attr(val, &attr);
 
+		/* We should not free val */
+		val = NULL;
 		if (attr & READ_ONLY) {
-			/* We should not free val */
-			val = NULL;
 			ret = EFI_WRITE_PROTECTED;
 			goto out;
 		}
+
+		/*
+		 * attributes won't be changed
+		 * TODO: take care of APPEND_WRITE once supported
+		 */
+		if (attr != attributes) {
+			ret = EFI_INVALID_PARAMETER;
+			goto out;
+		}
 	}
 
 	val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1);
-- 
2.21.0

[U-Boot] [PATCH v2 2/3] efi_loader: variable: return error for APPEND_WRITE

[Heinrich Schuchardt]

On 5/24/19 8:59 AM, AKASHI Takahiro wrote:
> The current efi_et_variable() doesn't support EFI_VARIABLE_APPEND_WRITE
> attiribut for now, and so should return an error.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

I will correct the typos in the commit message and add a TODO comment in
the code.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>  lib/efi_loader/efi_variable.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 28b1aa7505ae..e3ec502ffb45 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -427,7 +427,8 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>  	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
>  		  data_size, data);
>
> -	if (!variable_name || !vendor) {
> +	if (!variable_name || !vendor ||
> +	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
>  		ret = EFI_INVALID_PARAMETER;
>  		goto out;
>  	}
>

[U-Boot] [PATCH v2 1/3] efi: selftest: APPEND_WRITE is not supported

[Heinrich Schuchardt]

On 5/24/19 8:59 AM, AKASHI Takahiro wrote:
> The error here should be marked *todo*.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[U-Boot] [PATCH v2 3/3] efi_loader: variable: attributes may not be changed if a variable exists

[Heinrich Schuchardt]

On 5/24/19 8:59 AM, AKASHI Takahiro wrote:
> If a variable already exists, efi_set_variable() should not change
> the variable's attributes. This patch enforces it.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>  lib/efi_loader/efi_variable.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)