[U-Boot] [PATCH] efi_loader: variable: support APPEND_WRITE

[AKASHI Takahiro]

On Tue, Sep 03, 2019 at 10:31:25PM +0200, Heinrich Schuchardt wrote:
> On 9/3/19 7:40 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 at linaro.org>
> >---
> >  lib/efi_loader/efi_variable.c | 50 +++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 20 deletions(-)
> >
> >diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >index 6687b69a400d..5d1ee50a606e 100644
> >--- a/lib/efi_loader/efi_variable.c
> >+++ b/lib/efi_loader/efi_variable.c
> >@@ -423,18 +423,17 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >  				     const efi_guid_t *vendor, u32 attributes,
> >  				     efi_uintn_t data_size, const void *data)
> >  {
> >-	char *native_name = NULL, *val = NULL, *s;
> >+	char *native_name = NULL, *oval, *val = NULL, *s;
> 
> Please, use old_val install of oval.

Hmm, okay.

> >+	size_t oval_size;
> 
> Please, use old_val_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;
> >  	}
> 
> Append with data_size = 0 should not delete the variable. See UEFI 2.8 spec.

Good point. Will add a check against it.

> If a variable does not exist, trying to delete it should return
> EFI_NOT_FOUND.
> 
> If a variable is READ_ONLY we have to return EFI_WRITE_PROTECTED and
> should not delete it.
> 
> I know these deficiencies are not caused by your patch. But as you are
> touching the code now, could you, please, consider them.

Okay.

> >@@ -452,28 +451,37 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >  		goto out;
> >  	}
> >
> >-	val = env_get(native_name);
> >-	if (val) {
> >-		parse_attr(val, &attr);
> >+	oval = env_get(native_name);
> >+	if (oval) {
> >+		oval = parse_attr(oval, &attr);
> >
> >-		/* We should not free val */
> >-		val = NULL;
> >  		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) {
> >+		/* 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(oval, "(blob)")) {
> >+				/* TODO: should support utf8? */
> 
> If the variable is read_only we should not append to it but return
> EFI_WRITE_PROTECTED.
> 
> We never stored any data as utf8. We can eliminate all references to the
> (utf8) prefix in a future patch.

Agree here.

> In lib/efi_selftest/efi_selftest_variables.c we already have a test for
> APPEND. We should change the efi_st_todo() into efi_st_error() in a
> follow-up patch.

Okay.
I know that my patch passes the test itself.

-Takahiro Akashi

> Thanks for taking care of this gap.
> 
> Best regards
> 
> Heinrich
> 
> >+				return EFI_DEVICE_ERROR;
> >+				goto out;
> >+			}
> >+			oval_size = strlen(oval);
> >+		} else {
> >+			oval_size = 0;
> >+		}
> >+	} else {
> >+		oval_size = 0;
> >  	}
> >
> >-	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
> >+	val = malloc(oval_size + 2 * data_size
> >+		     + strlen("{ro,run,boot,nv}(blob)") + 1);
> >  	if (!val) {
> >  		ret = EFI_OUT_OF_RESOURCES;
> >  		goto out;
> >@@ -481,10 +489,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 +510,13 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >  	}
> >  	s += sprintf(s, "}");
> >
> >+	if (oval_size)
> >+		/* APPEND_WRITE */
> >+		s += sprintf(s, oval);
> >+	else
> >+		s += sprintf(s, "(blob)");
> >+
> >  	/* store payload: */
> >-	s += sprintf(s, "(blob)");
> >  	s = bin2hex(s, data, data_size);
> >  	*s = '\0';
> >
> >
>