[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 at 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';
>
>