[U-Boot] [PATCH v2 0/2] efi_loader: support APPEND_WRITE

[U-Boot] [PATCH v2 0/2] efi_loader: support APPEND_WRITE

[AKASHI Takahiro]

Changes in v2 (Sept 6, 2019)
* add a check against read-only variable to delete with APPEND_WRITE
* add a check against non-existent variable to append
* add/modify APPEND_WRITE tests

AKASHI Takahiro (2):
  efi_loader: variable: support APPEND_WRITE
  efi_loader: selftest: enable APPEND_WRITE tests

 lib/efi_loader/efi_variable.c             | 70 ++++++++++++++---------
 lib/efi_selftest/efi_selftest_variables.c | 20 ++++++-
 2 files changed, 63 insertions(+), 27 deletions(-)

-- 
2.21.0

[U-Boot] [PATCH v2 1/2] efi_loader: variable: support APPEND_WRITE

[AKASHI Takahiro]

If EFI_VARIABLE_APPEND_WRITE is specified in attributes at
efi_set_variable(), specified data will be appended to the variable's
original value. Attributes other than APPEND_WRITE should not be
modified.

With this patch, APPEND_WRITE test in 'variables' selftest will pass.

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

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 6687b69a400d..48ee255f879b 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -424,17 +424,17 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 				     efi_uintn_t data_size, const void *data)
 {
 	char *native_name = NULL, *val = NULL, *s;
+	const char *old_val;
+	size_t old_size;
 	efi_status_t ret = EFI_SUCCESS;
 	u32 attr;
 
 	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
 		  data_size, data);
 
-	/* TODO: implement APPEND_WRITE */
 	if (!variable_name || !*variable_name || !vendor ||
 	    ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
-	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)) ||
-	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
+	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) {
 		ret = EFI_INVALID_PARAMETER;
 		goto out;
 	}
@@ -445,35 +445,51 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 
 #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
 
-	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
-		/* delete the variable: */
-		env_set(native_name, NULL);
-		ret = EFI_SUCCESS;
-		goto out;
-	}
+	old_val = env_get(native_name);
+	if (old_val) {
+		old_val = parse_attr(old_val, &attr);
 
-	val = env_get(native_name);
-	if (val) {
-		parse_attr(val, &attr);
-
-		/* We should not free val */
-		val = NULL;
+		/* check read-only first */
 		if (attr & READ_ONLY) {
 			ret = EFI_WRITE_PROTECTED;
 			goto out;
 		}
 
-		/*
-		 * attributes won't be changed
-		 * TODO: take care of APPEND_WRITE once supported
-		 */
-		if (attr != attributes) {
+		if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
+			/* delete the variable: */
+			env_set(native_name, NULL);
+			ret = EFI_SUCCESS;
+			goto out;
+		}
+
+		/* attributes won't be changed */
+		if (attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) {
 			ret = EFI_INVALID_PARAMETER;
 			goto out;
 		}
+
+		if (attributes & EFI_VARIABLE_APPEND_WRITE) {
+			if (!prefix(old_val, "(blob)")) {
+				return EFI_DEVICE_ERROR;
+				goto out;
+			}
+			old_size = strlen(old_val);
+		} else {
+			old_size = 0;
+		}
+	} else {
+		if ((data_size == 0) || !(attributes & ACCESS_ATTR) ||
+		    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
+			/* delete, but nothing to do */
+			ret = EFI_NOT_FOUND;
+			goto out;
+		}
+
+		old_size = 0;
 	}
 
-	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
+	val = malloc(old_size + 2 * data_size
+		     + strlen("{ro,run,boot,nv}(blob)") + 1);
 	if (!val) {
 		ret = EFI_OUT_OF_RESOURCES;
 		goto out;
@@ -481,10 +497,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 
 	s = val;
 
-	/*
-	 * store attributes
-	 * TODO: several attributes are not supported
-	 */
+	/* store attributes */
 	attributes &= (EFI_VARIABLE_NON_VOLATILE |
 		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
 		       EFI_VARIABLE_RUNTIME_ACCESS);
@@ -505,8 +518,13 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
 	}
 	s += sprintf(s, "}");
 
+	if (old_size)
+		/* APPEND_WRITE */
+		s += sprintf(s, old_val);
+	else
+		s += sprintf(s, "(blob)");
+
 	/* store payload: */
-	s += sprintf(s, "(blob)");
 	s = bin2hex(s, data, data_size);
 	*s = '\0';
 
-- 
2.21.0

[U-Boot] [PATCH v2 2/2] efi_loader: selftest: enable APPEND_WRITE tests

[AKASHI Takahiro]

Now that APPEND_WRITE is supported,
the result check for the only existing test case should be changed to
'todo' to 'error', while two more test cases are added.

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

diff --git a/lib/efi_selftest/efi_selftest_variables.c b/lib/efi_selftest/efi_selftest_variables.c
index 06c1a032dd04..a6b41d1f008d 100644
--- a/lib/efi_selftest/efi_selftest_variables.c
+++ b/lib/efi_selftest/efi_selftest_variables.c
@@ -21,6 +21,9 @@ static const efi_guid_t guid_vendor0 =
 static const efi_guid_t guid_vendor1 =
 	EFI_GUID(0xff629290, 0x1fc1, 0xd73f,
 		 0x8f, 0xb1, 0x32, 0xf9, 0x0c, 0xa0, 0x42, 0xea);
+static const efi_guid_t guid_global =
+	EFI_GUID(0x8be4df61, 0x93ca, 0x11d2,
+		 0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c);
 
 /*
  * Setup unit test.
@@ -116,7 +119,7 @@ static int execute(void)
 				    EFI_VARIABLE_APPEND_WRITE,
 				    7, v + 8);
 	if (ret != EFI_SUCCESS) {
-		efi_st_todo("SetVariable(APPEND_WRITE) failed\n");
+		efi_st_error("SetVariable(APPEND_WRITE) failed\n");
 	} else {
 		len = EFI_ST_MAX_DATA_SIZE;
 		ret = runtime->get_variable(L"efi_st_var1", &guid_vendor1,
@@ -131,6 +134,21 @@ static int execute(void)
 		if (memcmp(data, v, len))
 			efi_st_todo("GetVariable returned wrong value\n");
 	}
+	/* Append variable 2 */
+	ret = runtime->set_variable(L"efi_none", &guid_vendor1,
+				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				    EFI_VARIABLE_APPEND_WRITE,
+				    15, v);
+	if (ret != EFI_NOT_FOUND)
+		efi_st_error("SetVariable(APPEND_WRITE) with size 0 to non-existent variable returns wrong code\n");
+	/* Append variable 3 */
+	ret = runtime->set_variable(L"PlatformLangCodes", &guid_global,
+				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				    EFI_VARIABLE_RUNTIME_ACCESS |
+				    EFI_VARIABLE_APPEND_WRITE,
+				    15, v);
+	if (ret != EFI_WRITE_PROTECTED)
+		efi_st_todo("SetVariable(APPEND_WRITE) to read-only variable returns wrong code\n");
 	/* Enumerate variables */
 	boottime->set_mem(&guid, 16, 0);
 	*varname = 0;
-- 
2.21.0

[U-Boot] [PATCH v2 1/2] efi_loader: variable: support APPEND_WRITE

[Heinrich Schuchardt]

On 9/6/19 8:09 AM, AKASHI Takahiro wrote:
> If EFI_VARIABLE_APPEND_WRITE is specified in attributes at
> efi_set_variable(), specified data will be appended to the variable's
> original value. Attributes other than APPEND_WRITE should not be
> modified.
>
> With this patch, APPEND_WRITE test in 'variables' selftest will pass.

With both patches of the series applied I get the following warning in
bootefi selftest:

lib/efi_selftest/efi_selftest_variables.c(151):
TODO: SetVariable(APPEND_WRITE) to read-only variable returns wrong code

Best regards

Heinrich

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_variable.c | 70 ++++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 6687b69a400d..48ee255f879b 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -424,17 +424,17 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>  				     efi_uintn_t data_size, const void *data)
>  {
>  	char *native_name = NULL, *val = NULL, *s;
> +	const char *old_val;
> +	size_t old_size;
>  	efi_status_t ret = EFI_SUCCESS;
>  	u32 attr;
>
>  	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
>  		  data_size, data);
>
> -	/* TODO: implement APPEND_WRITE */
>  	if (!variable_name || !*variable_name || !vendor ||
>  	    ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
> -	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)) ||
> -	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> +	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) {
>  		ret = EFI_INVALID_PARAMETER;
>  		goto out;
>  	}
> @@ -445,35 +445,51 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>
>  #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
>
> -	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> -		/* delete the variable: */
> -		env_set(native_name, NULL);
> -		ret = EFI_SUCCESS;
> -		goto out;
> -	}
> +	old_val = env_get(native_name);
> +	if (old_val) {
> +		old_val = parse_attr(old_val, &attr);
>
> -	val = env_get(native_name);
> -	if (val) {
> -		parse_attr(val, &attr);
> -
> -		/* We should not free val */
> -		val = NULL;
> +		/* check read-only first */
>  		if (attr & READ_ONLY) {
>  			ret = EFI_WRITE_PROTECTED;
>  			goto out;
>  		}
>
> -		/*
> -		 * attributes won't be changed
> -		 * TODO: take care of APPEND_WRITE once supported
> -		 */
> -		if (attr != attributes) {
> +		if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> +			/* delete the variable: */
> +			env_set(native_name, NULL);
> +			ret = EFI_SUCCESS;
> +			goto out;
> +		}
> +
> +		/* attributes won't be changed */
> +		if (attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) {
>  			ret = EFI_INVALID_PARAMETER;
>  			goto out;
>  		}
> +
> +		if (attributes & EFI_VARIABLE_APPEND_WRITE) {
> +			if (!prefix(old_val, "(blob)")) {
> +				return EFI_DEVICE_ERROR;
> +				goto out;
> +			}
> +			old_size = strlen(old_val);
> +		} else {
> +			old_size = 0;
> +		}
> +	} else {
> +		if ((data_size == 0) || !(attributes & ACCESS_ATTR) ||
> +		    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> +			/* delete, but nothing to do */
> +			ret = EFI_NOT_FOUND;
> +			goto out;
> +		}
> +
> +		old_size = 0;
>  	}
>
> -	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
> +	val = malloc(old_size + 2 * data_size
> +		     + strlen("{ro,run,boot,nv}(blob)") + 1);
>  	if (!val) {
>  		ret = EFI_OUT_OF_RESOURCES;
>  		goto out;
> @@ -481,10 +497,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>
>  	s = val;
>
> -	/*
> -	 * store attributes
> -	 * TODO: several attributes are not supported
> -	 */
> +	/* store attributes */
>  	attributes &= (EFI_VARIABLE_NON_VOLATILE |
>  		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
>  		       EFI_VARIABLE_RUNTIME_ACCESS);
> @@ -505,8 +518,13 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>  	}
>  	s += sprintf(s, "}");
>
> +	if (old_size)
> +		/* APPEND_WRITE */
> +		s += sprintf(s, old_val);
> +	else
> +		s += sprintf(s, "(blob)");
> +
>  	/* store payload: */
> -	s += sprintf(s, "(blob)");
>  	s = bin2hex(s, data, data_size);
>  	*s = '\0';
>
>

[U-Boot] [PATCH v2 1/2] efi_loader: variable: support APPEND_WRITE

[AKASHI Takahiro]

On Mon, Sep 16, 2019 at 07:03:41PM +0200, Heinrich Schuchardt wrote:
> On 9/6/19 8:09 AM, AKASHI Takahiro wrote:
> > If EFI_VARIABLE_APPEND_WRITE is specified in attributes at
> > efi_set_variable(), specified data will be appended to the variable's
> > original value. Attributes other than APPEND_WRITE should not be
> > modified.
> >
> > With this patch, APPEND_WRITE test in 'variables' selftest will pass.
> 
> With both patches of the series applied I get the following warning in
> bootefi selftest:
> 
> lib/efi_selftest/efi_selftest_variables.c(151):
> TODO: SetVariable(APPEND_WRITE) to read-only variable returns wrong code

Yes, I know. That's why I used TODO here.
The reason that this test fails is that we don't support read-only
variables, more strictly, we do have read-only attribute, but don't
have any interface to specify read-only attribute for any variable.

Read-only is an implicit nature of variable only attributed to
some "global" variables UEFI specification defines. So we should
fix the issue in a separate patch.

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_variable.c | 70 ++++++++++++++++++++++-------------
> >  1 file changed, 44 insertions(+), 26 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index 6687b69a400d..48ee255f879b 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -424,17 +424,17 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >  				     efi_uintn_t data_size, const void *data)
> >  {
> >  	char *native_name = NULL, *val = NULL, *s;
> > +	const char *old_val;
> > +	size_t old_size;
> >  	efi_status_t ret = EFI_SUCCESS;
> >  	u32 attr;
> >
> >  	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> >  		  data_size, data);
> >
> > -	/* TODO: implement APPEND_WRITE */
> >  	if (!variable_name || !*variable_name || !vendor ||
> >  	    ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
> > -	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)) ||
> > -	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> > +	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) {
> >  		ret = EFI_INVALID_PARAMETER;
> >  		goto out;
> >  	}
> > @@ -445,35 +445,51 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >
> >  #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
> >
> > -	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> > -		/* delete the variable: */
> > -		env_set(native_name, NULL);
> > -		ret = EFI_SUCCESS;
> > -		goto out;
> > -	}
> > +	old_val = env_get(native_name);
> > +	if (old_val) {
> > +		old_val = parse_attr(old_val, &attr);
> >
> > -	val = env_get(native_name);
> > -	if (val) {
> > -		parse_attr(val, &attr);
> > -
> > -		/* We should not free val */
> > -		val = NULL;
> > +		/* check read-only first */
> >  		if (attr & READ_ONLY) {
> >  			ret = EFI_WRITE_PROTECTED;
> >  			goto out;
> >  		}
> >
> > -		/*
> > -		 * attributes won't be changed
> > -		 * TODO: take care of APPEND_WRITE once supported
> > -		 */
> > -		if (attr != attributes) {
> > +		if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> > +			/* delete the variable: */
> > +			env_set(native_name, NULL);
> > +			ret = EFI_SUCCESS;
> > +			goto out;
> > +		}
> > +
> > +		/* attributes won't be changed */
> > +		if (attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) {
> >  			ret = EFI_INVALID_PARAMETER;
> >  			goto out;
> >  		}
> > +
> > +		if (attributes & EFI_VARIABLE_APPEND_WRITE) {
> > +			if (!prefix(old_val, "(blob)")) {
> > +				return EFI_DEVICE_ERROR;
> > +				goto out;
> > +			}
> > +			old_size = strlen(old_val);
> > +		} else {
> > +			old_size = 0;
> > +		}
> > +	} else {
> > +		if ((data_size == 0) || !(attributes & ACCESS_ATTR) ||
> > +		    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> > +			/* delete, but nothing to do */
> > +			ret = EFI_NOT_FOUND;
> > +			goto out;
> > +		}
> > +
> > +		old_size = 0;
> >  	}
> >
> > -	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
> > +	val = malloc(old_size + 2 * data_size
> > +		     + strlen("{ro,run,boot,nv}(blob)") + 1);
> >  	if (!val) {
> >  		ret = EFI_OUT_OF_RESOURCES;
> >  		goto out;
> > @@ -481,10 +497,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >
> >  	s = val;
> >
> > -	/*
> > -	 * store attributes
> > -	 * TODO: several attributes are not supported
> > -	 */
> > +	/* store attributes */
> >  	attributes &= (EFI_VARIABLE_NON_VOLATILE |
> >  		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >  		       EFI_VARIABLE_RUNTIME_ACCESS);
> > @@ -505,8 +518,13 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >  	}
> >  	s += sprintf(s, "}");
> >
> > +	if (old_size)
> > +		/* APPEND_WRITE */
> > +		s += sprintf(s, old_val);
> > +	else
> > +		s += sprintf(s, "(blob)");
> > +
> >  	/* store payload: */
> > -	s += sprintf(s, "(blob)");
> >  	s = bin2hex(s, data, data_size);
> >  	*s = '\0';
> >
> >
> 

[U-Boot] [PATCH v2 1/2] efi_loader: variable: support APPEND_WRITE

[Heinrich Schuchardt]

On 9/6/19 8:09 AM, AKASHI Takahiro wrote:
> If EFI_VARIABLE_APPEND_WRITE is specified in attributes at
> efi_set_variable(), specified data will be appended to the variable's
> original value. Attributes other than APPEND_WRITE should not be
> modified.
>
> With this patch, APPEND_WRITE test in 'variables' selftest will pass.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   lib/efi_loader/efi_variable.c | 70 ++++++++++++++++++++++-------------
>   1 file changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 6687b69a400d..48ee255f879b 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -424,17 +424,17 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>   				     efi_uintn_t data_size, const void *data)
>   {
>   	char *native_name = NULL, *val = NULL, *s;
> +	const char *old_val;
> +	size_t old_size;
>   	efi_status_t ret = EFI_SUCCESS;
>   	u32 attr;
>
>   	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
>   		  data_size, data);
>
> -	/* TODO: implement APPEND_WRITE */
>   	if (!variable_name || !*variable_name || !vendor ||
>   	    ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
> -	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)) ||
> -	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> +	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) {
>   		ret = EFI_INVALID_PARAMETER;
>   		goto out;
>   	}
> @@ -445,35 +445,51 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>
>   #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
>
> -	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> -		/* delete the variable: */
> -		env_set(native_name, NULL);
> -		ret = EFI_SUCCESS;
> -		goto out;
> -	}
> +	old_val = env_get(native_name);
> +	if (old_val) {
> +		old_val = parse_attr(old_val, &attr);
>
> -	val = env_get(native_name);
> -	if (val) {
> -		parse_attr(val, &attr);
> -
> -		/* We should not free val */
> -		val = NULL;
> +		/* check read-only first */
>   		if (attr & READ_ONLY) {
>   			ret = EFI_WRITE_PROTECTED;
>   			goto out;
>   		}
>
> -		/*
> -		 * attributes won't be changed
> -		 * TODO: take care of APPEND_WRITE once supported
> -		 */
> -		if (attr != attributes) {
> +		if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {

The understanding of EDK2 is that no access attributes means 'attributes
== 0' (Function VariableServiceSetVariable() in
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c).

EFI_VARIABLE_APPEND_WRITE and data_size = 0 should not delete a variable
according to the UEFI spec:

"Unless the EFI_VARIABLE_APPEND_WRITE,
EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, or
EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS attribute is set (see below),
using SetVariable() with a DataSize of zero will cause the entire
variable to be deleted."

Best regards

Heinrich

> +			/* delete the variable: */
> +			env_set(native_name, NULL);
> +			ret = EFI_SUCCESS;
> +			goto out;
> +		}
> +
> +		/* attributes won't be changed */
> +		if (attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) {
>   			ret = EFI_INVALID_PARAMETER;
>   			goto out;
>   		}
> +
> +		if (attributes & EFI_VARIABLE_APPEND_WRITE) {
> +			if (!prefix(old_val, "(blob)")) {
> +				return EFI_DEVICE_ERROR;
> +				goto out;
> +			}
> +			old_size = strlen(old_val);
> +		} else {
> +			old_size = 0;
> +		}
> +	} else {
> +		if ((data_size == 0) || !(attributes & ACCESS_ATTR) ||
> +		    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> +			/* delete, but nothing to do */
> +			ret = EFI_NOT_FOUND;
> +			goto out;
> +		}
> +
> +		old_size = 0;
>   	}
>
> -	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
> +	val = malloc(old_size + 2 * data_size
> +		     + strlen("{ro,run,boot,nv}(blob)") + 1);
>   	if (!val) {
>   		ret = EFI_OUT_OF_RESOURCES;
>   		goto out;
> @@ -481,10 +497,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>
>   	s = val;
>
> -	/*
> -	 * store attributes
> -	 * TODO: several attributes are not supported
> -	 */
> +	/* store attributes */
>   	attributes &= (EFI_VARIABLE_NON_VOLATILE |
>   		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
>   		       EFI_VARIABLE_RUNTIME_ACCESS);
> @@ -505,8 +518,13 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>   	}
>   	s += sprintf(s, "}");
>
> +	if (old_size)
> +		/* APPEND_WRITE */
> +		s += sprintf(s, old_val);
> +	else
> +		s += sprintf(s, "(blob)");
> +
>   	/* store payload: */
> -	s += sprintf(s, "(blob)");
>   	s = bin2hex(s, data, data_size);
>   	*s = '\0';
>
>

[U-Boot] [PATCH v2 1/2] efi_loader: variable: support APPEND_WRITE

[AKASHI Takahiro]

On Tue, Sep 17, 2019 at 10:38:47PM +0200, Heinrich Schuchardt wrote:
> On 9/6/19 8:09 AM, AKASHI Takahiro wrote:
> >If EFI_VARIABLE_APPEND_WRITE is specified in attributes at
> >efi_set_variable(), specified data will be appended to the variable's
> >original value. Attributes other than APPEND_WRITE should not be
> >modified.
> >
> >With this patch, APPEND_WRITE test in 'variables' selftest will pass.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  lib/efi_loader/efi_variable.c | 70 ++++++++++++++++++++++-------------
> >  1 file changed, 44 insertions(+), 26 deletions(-)
> >
> >diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >index 6687b69a400d..48ee255f879b 100644
> >--- a/lib/efi_loader/efi_variable.c
> >+++ b/lib/efi_loader/efi_variable.c
> >@@ -424,17 +424,17 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >  				     efi_uintn_t data_size, const void *data)
> >  {
> >  	char *native_name = NULL, *val = NULL, *s;
> >+	const char *old_val;
> >+	size_t old_size;
> >  	efi_status_t ret = EFI_SUCCESS;
> >  	u32 attr;
> >
> >  	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> >  		  data_size, data);
> >
> >-	/* TODO: implement APPEND_WRITE */
> >  	if (!variable_name || !*variable_name || !vendor ||
> >  	    ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
> >-	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)) ||
> >-	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> >+	     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) {
> >  		ret = EFI_INVALID_PARAMETER;
> >  		goto out;
> >  	}
> >@@ -445,35 +445,51 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >
> >  #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
> >
> >-	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> >-		/* delete the variable: */
> >-		env_set(native_name, NULL);
> >-		ret = EFI_SUCCESS;
> >-		goto out;
> >-	}
> >+	old_val = env_get(native_name);
> >+	if (old_val) {
> >+		old_val = parse_attr(old_val, &attr);
> >
> >-	val = env_get(native_name);
> >-	if (val) {
> >-		parse_attr(val, &attr);
> >-
> >-		/* We should not free val */
> >-		val = NULL;
> >+		/* check read-only first */
> >  		if (attr & READ_ONLY) {
> >  			ret = EFI_WRITE_PROTECTED;
> >  			goto out;
> >  		}
> >
> >-		/*
> >-		 * attributes won't be changed
> >-		 * TODO: take care of APPEND_WRITE once supported
> >-		 */
> >-		if (attr != attributes) {
> >+		if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> 
> The understanding of EDK2 is that no access attributes means 'attributes
> == 0' (Function VariableServiceSetVariable() in
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c).

No, it's not correct. The right place you should refer to is:
UpdateVariable (
   [snip up to l.2279 or so]
    //
    // Setting a data variable with no access, or zero DataSize attributes
    // causes it to be deleted.
    // When the EFI_VARIABLE_APPEND_WRITE attribute is set, DataSize of zero will
    // not delete the variable.
    //
    if ((((Attributes & EFI_VARIABLE_APPEND_WRITE) == 0) && (DataSize == 0))|| ((Attributes & (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)) == 0)) {

> EFI_VARIABLE_APPEND_WRITE and data_size = 0 should not delete a variable
> according to the UEFI spec:

This is correct.

Thanks,
-Takahiro Akashi

> "Unless the EFI_VARIABLE_APPEND_WRITE,
> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, or
> EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS attribute is set (see below),
> using SetVariable() with a DataSize of zero will cause the entire
> variable to be deleted."
> 
> Best regards
> 
> Heinrich
> 
> >+			/* delete the variable: */
> >+			env_set(native_name, NULL);
> >+			ret = EFI_SUCCESS;
> >+			goto out;
> >+		}
> >+
> >+		/* attributes won't be changed */
> >+		if (attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) {
> >  			ret = EFI_INVALID_PARAMETER;
> >  			goto out;
> >  		}
> >+
> >+		if (attributes & EFI_VARIABLE_APPEND_WRITE) {
> >+			if (!prefix(old_val, "(blob)")) {
> >+				return EFI_DEVICE_ERROR;
> >+				goto out;
> >+			}
> >+			old_size = strlen(old_val);
> >+		} else {
> >+			old_size = 0;
> >+		}
> >+	} else {
> >+		if ((data_size == 0) || !(attributes & ACCESS_ATTR) ||
> >+		    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> >+			/* delete, but nothing to do */
> >+			ret = EFI_NOT_FOUND;
> >+			goto out;
> >+		}
> >+
> >+		old_size = 0;
> >  	}
> >
> >-	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
> >+	val = malloc(old_size + 2 * data_size
> >+		     + strlen("{ro,run,boot,nv}(blob)") + 1);
> >  	if (!val) {
> >  		ret = EFI_OUT_OF_RESOURCES;
> >  		goto out;
> >@@ -481,10 +497,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >
> >  	s = val;
> >
> >-	/*
> >-	 * store attributes
> >-	 * TODO: several attributes are not supported
> >-	 */
> >+	/* store attributes */
> >  	attributes &= (EFI_VARIABLE_NON_VOLATILE |
> >  		       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >  		       EFI_VARIABLE_RUNTIME_ACCESS);
> >@@ -505,8 +518,13 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >  	}
> >  	s += sprintf(s, "}");
> >
> >+	if (old_size)
> >+		/* APPEND_WRITE */
> >+		s += sprintf(s, old_val);
> >+	else
> >+		s += sprintf(s, "(blob)");
> >+
> >  	/* store payload: */
> >-	s += sprintf(s, "(blob)");
> >  	s = bin2hex(s, data, data_size);
> >  	*s = '\0';
> >
> >
>