diff mbox series

[v1,3/4] efi_loader: add an EFI variable with the variable file contents

Message ID 20240406140203.248211-4-ilias.apalodimas@linaro.org
State Superseded
Headers show
Series Enable SetvariableRT | expand

Commit Message

Ilias Apalodimas April 6, 2024, 2:01 p.m. UTC 
Previous patches enabled SetVariableRT using a RAM backend.
Although EBBR [0] defines a variable format we can teach userspace tools
and write the altered variables, it's better if we skip the ABI
requirements completely.

So let's add a new variable, in its own namespace called "VarToFile"
which contains a binary dump of the updated RT, BS and, NV variables.

Some adjustments are needed to do that. Currently we discard BS-only
variables in EBS(). We need to preserve those on the OS RAM backend
that exposes the variables. Since BS-only variables can't appear at RT
we need to move the memory masking checks from efi_var_collect() to
efi_get_next_variable_name_mem()/efi_get_variable_mem() and do the
filtering at runtime. We also need to make efi_var_collect() available
at runtime, in order to construct the "VarToFile" buffer with BS, RT &
NV variables.

All users and applications (for linux) have to do when updating a variable
is dd that variable in the file described by "RTStorageVolatile".

Linux efivarfs uses a first 4 bytes of the output to represent attributes
in little-endian format. So, storing variables works like this:

$~ efibootmgr -n 0001
$~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1

[0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage

Suggested-by:Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi_variable.h            |  15 +++-
 lib/efi_loader/efi_boottime.c     |   2 +
 lib/efi_loader/efi_var_common.c   |  43 +++++------
 lib/efi_loader/efi_var_file.c     |   1 -
 lib/efi_loader/efi_var_mem.c      |  90 ++++++++++-------------
 lib/efi_loader/efi_variable.c     | 118 ++++++++++++++++++++++++------
 lib/efi_loader/efi_variable_tee.c |   1 -
 7 files changed, 164 insertions(+), 106 deletions(-)

--
2.37.2

Comments

Heinrich Schuchardt April 8, 2024, 6:41 a.m. UTC | #1 
On 4/6/24 16:01, Ilias Apalodimas wrote:
> Previous patches enabled SetVariableRT using a RAM backend.
> Although EBBR [0] defines a variable format we can teach userspace tools
> and write the altered variables, it's better if we skip the ABI
> requirements completely.
>
> So let's add a new variable, in its own namespace called "VarToFile"
> which contains a binary dump of the updated RT, BS and, NV variables.
>
> Some adjustments are needed to do that. Currently we discard BS-only
> variables in EBS(). We need to preserve those on the OS RAM backend
> that exposes the variables. Since BS-only variables can't appear at RT
> we need to move the memory masking checks from efi_var_collect() to
> efi_get_next_variable_name_mem()/efi_get_variable_mem() and do the
> filtering at runtime. We also need to make efi_var_collect() available
> at runtime, in order to construct the "VarToFile" buffer with BS, RT &
> NV variables.
>
> All users and applications (for linux) have to do when updating a variable
> is dd that variable in the file described by "RTStorageVolatile".
>
> Linux efivarfs uses a first 4 bytes of the output to represent attributes
> in little-endian format. So, storing variables works like this:
>
> $~ efibootmgr -n 0001
> $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
>
> [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage

This change actually doubles the amount of memory needed to store a
variable at runtime. How do you reflect this in QueryVariableInfo()?

>
> Suggested-by:Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   include/efi_variable.h            |  15 +++-
>   lib/efi_loader/efi_boottime.c     |   2 +
>   lib/efi_loader/efi_var_common.c   |  43 +++++------
>   lib/efi_loader/efi_var_file.c     |   1 -
>   lib/efi_loader/efi_var_mem.c      |  90 ++++++++++-------------
>   lib/efi_loader/efi_variable.c     | 118 ++++++++++++++++++++++++------
>   lib/efi_loader/efi_variable_tee.c |   1 -
>   7 files changed, 164 insertions(+), 106 deletions(-)
>
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index 42a2b7c52bef..8963339b9bb6 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -271,13 +271,15 @@ const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
>    *
>    * @variable_name_size:	size of variable_name buffer in bytes
>    * @variable_name:	name of uefi variable's name in u16
> + * @mask:		bitmask with required attributes of variables to be collected.
> + *                      variables are only collected if all of the required
>    * @vendor:		vendor's guid
>    *
>    * Return: status code
>    */
>   efi_status_t __efi_runtime
>   efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
> -			       efi_guid_t *vendor);
> +			       efi_guid_t *vendor, u32 mask);
>   /**
>    * efi_get_variable_mem() - Runtime common code across efi variable
>    *                          implementations for GetVariable() from
> @@ -289,12 +291,14 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
>    * @data_size:		size of the buffer to which the variable value is copied
>    * @data:		buffer to which the variable value is copied
>    * @timep:		authentication time (seconds since start of epoch)
> + * @mask:		bitmask with required attributes of variables to be collected.
> + *                      variables are only collected if all of the required

This line looks incomplete.

>    * Return:		status code
>    */
>   efi_status_t __efi_runtime
>   efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>   		     u32 *attributes, efi_uintn_t *data_size, void *data,
> -		     u64 *timep);
> +		     u64 *timep, u32 mask);
>
>   /**
>    * efi_get_variable_runtime() - runtime implementation of GetVariable()
> @@ -334,4 +338,11 @@ efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>    */
>   void efi_var_buf_update(struct efi_var_file *var_buf);
>
> +/**
> + * efi_prealloced_rt_memory() - Get a pointer to preallocated EFI memory
> + *                              available at runtime
> + *
> + * Return: pointer to preallocated runtime usable buffer
> + */
> +void __efi_runtime *efi_prealloced_rt_memory(void);
>   #endif
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 1951291747cd..39481c89a688 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -97,6 +97,8 @@ const efi_guid_t efi_guid_load_file_protocol = EFI_LOAD_FILE_PROTOCOL_GUID;
>   const efi_guid_t efi_guid_load_file2_protocol = EFI_LOAD_FILE2_PROTOCOL_GUID;
>   /* GUID of the SMBIOS table */
>   const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
> +/* used by special U-Boot variables during SetVariableRT */
> +const efi_guid_t efi_guid_efi_rt_var_file = U_BOOT_EFI_RT_VAR_FILE_GUID;
>
>   static efi_status_t EFIAPI efi_disconnect_controller(
>   					efi_handle_t controller_handle,
> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> index 07b9603d49f3..4abc90e411e7 100644
> --- a/lib/efi_loader/efi_var_common.c
> +++ b/lib/efi_loader/efi_var_common.c
> @@ -182,7 +182,8 @@ efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
>   {
>   	efi_status_t ret;
>
> -	ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
> +	ret = efi_get_variable_mem(variable_name, guid, attributes, data_size,
> +				   data, NULL, EFI_VARIABLE_RUNTIME_ACCESS);
>
>   	/* Remove EFI_VARIABLE_READ_ONLY flag */
>   	if (attributes)
> @@ -195,7 +196,8 @@ efi_status_t __efi_runtime EFIAPI
>   efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>   				   u16 *variable_name, efi_guid_t *guid)
>   {
> -	return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
> +	return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> +					      guid, EFI_VARIABLE_RUNTIME_ACCESS);
>   }
>
>   /**
> @@ -427,18 +429,15 @@ void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
>    *
>    * Return:	Status code
>    */
> -efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp,
> -					    u32 check_attr_mask)
> +efi_status_t __efi_runtime
> +efi_var_collect(struct efi_var_file **bufp, loff_t *lenp, u32 check_attr_mask)
>   {
>   	size_t len = EFI_VAR_BUF_SIZE;
>   	struct efi_var_file *buf;
>   	struct efi_var_entry *var, *old_var;
>   	size_t old_var_name_length = 2;
>
> -	*bufp = NULL; /* Avoid double free() */
> -	buf = calloc(1, len);
> -	if (!buf)
> -		return EFI_OUT_OF_RESOURCES;
> +	buf = (struct efi_var_file *)efi_prealloced_rt_memory();
>   	var = buf->var;
>   	old_var = var;
>   	for (;;) {
> @@ -451,32 +450,26 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>   			return EFI_BUFFER_TOO_SMALL;
>
>   		var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
> -		memcpy(var->name, old_var->name, old_var_name_length);
> -		guidcpy(&var->guid, &old_var->guid);
> -		ret = efi_get_next_variable_name_int(
> -				&var_name_length, var->name, &var->guid);
> +		efi_memcpy_runtime(var->name, old_var->name, old_var_name_length);
> +		efi_memcpy_runtime(&var->guid, &old_var->guid, sizeof(efi_guid_t));
> +		ret = efi_get_next_variable_name_mem(&var_name_length, var->name,
> +						     &var->guid, check_attr_mask);
>   		if (ret == EFI_NOT_FOUND)
>   			break;
> -		if (ret != EFI_SUCCESS) {
> -			free(buf);
> +		if (ret != EFI_SUCCESS)
>   			return ret;
> -		}
>   		old_var_name_length = var_name_length;
>   		old_var = var;
>
>   		data = (u8 *)var->name + old_var_name_length;
>   		data_length = (uintptr_t)buf + len - (uintptr_t)data;
> -		ret = efi_get_variable_int(var->name, &var->guid,
> +		ret = efi_get_variable_mem(var->name, &var->guid,
>   					   &var->attr, &data_length, data,
> -					   &var->time);
> -		if (ret != EFI_SUCCESS) {
> -			free(buf);
> +					   &var->time, check_attr_mask);
> +		if (ret != EFI_SUCCESS)
>   			return ret;
> -		}
> -		if ((var->attr & check_attr_mask) == check_attr_mask) {
> -			var->length = data_length;
> -			var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8);
> -		}
> +		var->length = data_length;
> +		var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8);
>   	}
>
>   	buf->reserved = 0;
> @@ -490,5 +483,3 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>
>   	return EFI_SUCCESS;
>   }
> -
> -
> diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> index 413e1794e88c..8614e3d34706 100644
> --- a/lib/efi_loader/efi_var_file.c
> +++ b/lib/efi_loader/efi_var_file.c
> @@ -83,7 +83,6 @@ efi_status_t efi_var_to_file(void)
>   error:
>   	if (ret != EFI_SUCCESS)
>   		log_err("Failed to persist EFI variables\n");
> -	free(buf);
>   	return ret;
>   #else
>   	return EFI_SUCCESS;
> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> index 6c21cec5d457..a7af0604733e 100644
> --- a/lib/efi_loader/efi_var_mem.c
> +++ b/lib/efi_loader/efi_var_mem.c
> @@ -16,6 +16,7 @@
>    * relocation during SetVirtualAddressMap().
>    */
>   static struct efi_var_file __efi_runtime_data *efi_var_buf;
> +static void __efi_runtime_data *efi_rt_prealloced;
>   static struct efi_var_entry __efi_runtime_data *efi_current_var;
>
>   /**
> @@ -184,53 +185,6 @@ u64 __efi_runtime efi_var_mem_free(void)
>   	       sizeof(struct efi_var_entry);
>   }
>
> -/**
> - * efi_var_mem_bs_del() - delete boot service only variables
> - */
> -static void efi_var_mem_bs_del(void)
> -{
> -	struct efi_var_entry *var = efi_var_buf->var;
> -
> -	for (;;) {
> -		struct efi_var_entry *last;
> -
> -		last = (struct efi_var_entry *)
> -		       ((uintptr_t)efi_var_buf + efi_var_buf->length);
> -		if (var >= last)
> -			break;
> -		if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
> -			u16 *data;
> -
> -			/* skip variable */
> -			for (data = var->name; *data; ++data)
> -				;
> -			++data;
> -			var = (struct efi_var_entry *)
> -			      ALIGN((uintptr_t)data + var->length, 8);
> -		} else {
> -			/* delete variable */
> -			efi_var_mem_del(var);
> -		}
> -	}
> -}
> -
> -/**
> - * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
> - *
> - * @event:	callback event
> - * @context:	callback context
> - */
> -static void EFIAPI
> -efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
> -{
> -	EFI_ENTRY("%p, %p", event, context);
> -
> -	/* Delete boot service only variables */
> -	efi_var_mem_bs_del();
> -
> -	EFI_EXIT(EFI_SUCCESS);
> -}
> -
>   /**
>    * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
>    *
> @@ -241,6 +195,7 @@ static void EFIAPI __efi_runtime
>   efi_var_mem_notify_virtual_address_map(struct efi_event *event, void *context)
>   {
>   	efi_convert_pointer(0, (void **)&efi_var_buf);
> +	efi_convert_pointer(0, (void **)&efi_rt_prealloced);
>   	efi_current_var = NULL;
>   }
>
> @@ -261,13 +216,21 @@ efi_status_t efi_var_mem_init(void)
>   	efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
>   	efi_var_buf->length = (uintptr_t)efi_var_buf->var -
>   			      (uintptr_t)efi_var_buf;
> -	/* crc32 for 0 bytes = 0 */
>
> -	ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> -			       efi_var_mem_notify_exit_boot_services, NULL,
> -			       NULL, &event);
> +	/*
> +	 * efi_var_collect() needs to run at runtime and provide us
> +	 * copies of variables used for the VarToFile variable.
> +	 * Preallocate memory equal to the variable storage and
> +	 * preserve it to copy variables around
> +	 */
> +	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> +				 EFI_RUNTIME_SERVICES_DATA,
> +				 efi_size_in_pages(EFI_VAR_BUF_SIZE),
> +				 &memory);
>   	if (ret != EFI_SUCCESS)
>   		return ret;
> +	efi_rt_prealloced = (void *)(uintptr_t)memory;
> +
>   	ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
>   			       efi_var_mem_notify_virtual_address_map, NULL,
>   			       NULL, &event);
> @@ -279,7 +242,7 @@ efi_status_t efi_var_mem_init(void)
>   efi_status_t __efi_runtime
>   efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>   		     u32 *attributes, efi_uintn_t *data_size, void *data,
> -		     u64 *timep)
> +		     u64 *timep, u32 mask)
>   {
>   	efi_uintn_t old_size;
>   	struct efi_var_entry *var;
> @@ -291,6 +254,9 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>   	if (!var)
>   		return EFI_NOT_FOUND;
>
> +	if (mask && !((var->attr & mask) == mask))
> +		return EFI_NOT_FOUND;
> +
>   	if (attributes)
>   		*attributes = var->attr;
>   	if (timep)
> @@ -315,7 +281,8 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>
>   efi_status_t __efi_runtime
>   efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> -			       u16 *variable_name, efi_guid_t *vendor)
> +			       u16 *variable_name, efi_guid_t *vendor,
> +			       u32 mask)
>   {
>   	struct efi_var_entry *var;
>   	efi_uintn_t len, old_size;
> @@ -324,6 +291,7 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
>   	if (!variable_name_size || !variable_name || !vendor)
>   		return EFI_INVALID_PARAMETER;
>
> +skip:
>   	len = *variable_name_size >> 1;
>   	if (u16_strnlen(variable_name, len) == len)
>   		return EFI_INVALID_PARAMETER;
> @@ -347,6 +315,11 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
>   	efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
>   	efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
>
> +	if (mask && !((var->attr & mask) == mask)) {
> +		*variable_name_size = old_size;
> +		goto skip;
> +	}
> +
>   	return EFI_SUCCESS;
>   }
>
> @@ -354,3 +327,14 @@ void efi_var_buf_update(struct efi_var_file *var_buf)
>   {
>   	memcpy(efi_var_buf, var_buf, EFI_VAR_BUF_SIZE);
>   }
> +
> +void __efi_runtime *efi_prealloced_rt_memory(void)
> +{
> +	char *s;
> +	int count = EFI_VAR_BUF_SIZE;
> +
> +	s = (char *)efi_rt_prealloced;
> +	while (count--)
> +		*s++ = 0;
> +	return efi_rt_prealloced;
> +}
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index f97c8c57f75c..4f529169ea54 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -22,6 +22,8 @@
>   #include <u-boot/crc.h>
>   #include <asm/sections.h>
>
> +static const efi_guid_t __efi_runtime_data efi_guid_efi_rt_var_file =
> +						U_BOOT_EFI_RT_VAR_FILE_GUID;
>   #ifdef CONFIG_EFI_SECURE_BOOT
>
>   /**
> @@ -208,14 +210,16 @@ efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
>   		     u32 *attributes, efi_uintn_t *data_size, void *data,
>   		     u64 *timep)
>   {
> -	return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
> +	return efi_get_variable_mem(variable_name, vendor, attributes, data_size,
> +				    data, timep, 0);
>   }
>
>   efi_status_t __efi_runtime
>   efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
>   			       u16 *variable_name, efi_guid_t *vendor)
>   {
> -	return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
> +	return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> +					      vendor, 0);
>   }
>
>   /**
> @@ -479,6 +483,8 @@ if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
>   	efi_uintn_t ret;
>   	bool append, delete;
>   	u64 time = 0;
> +	struct efi_var_file *buf;
> +	loff_t len;
>
>   	/*
>   	 * Authenticated variables are not supported the rest of the checks
> @@ -520,30 +526,60 @@ if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
>   			return EFI_NOT_FOUND;
>   	}
>
> -	if (delete) {
> +	if (!delete) {
> +		/*
> +		 * We always insert new variabes and delete the old one when
> +		 * appending
> +		 */
> +		len = 2 * (u16_strlen(variable_name) + 1) + data_size +
> +			sizeof(struct efi_var_entry);
> +		if (var && append)
> +			 len += 2 * var->length;
> +		/*
> +		 * We will copy the variable update into VarToFile,
> +		 * account for it twice
> +		 */
> +		len *= 2;
> +		if (len > efi_var_mem_free())
> +			return EFI_OUT_OF_RESOURCES;
> +		if (append && var) {
> +			u16 *old_data = var->name;
> +
> +			for (; *old_data; ++old_data)
> +				;
> +			++old_data;
> +			ret = efi_var_mem_ins(variable_name, vendor, attributes,
> +					      var->length, old_data, data_size,
> +					      data, time);
> +		} else {
> +			ret = efi_var_mem_ins(variable_name, vendor, attributes,
> +					      data_size, data, 0, NULL, time);
> +		}
> +	} else {
>   		/* EFI_NOT_FOUND has been handled before */
>   		attributes = var->attr;
>   		ret = EFI_SUCCESS;
> -	} else if (append && var) {
> -		u16 *old_data = var->name;
> -
> -		for (; *old_data; ++old_data)
> -			;
> -		++old_data;
> -		ret = efi_var_mem_ins(variable_name, vendor, attributes,
> -				      var->length, old_data, data_size, data,
> -				      time);
> -	} else {
> -		ret = efi_var_mem_ins(variable_name, vendor, attributes,
> -				      data_size, data, 0, NULL, time);
>   	}
> -
>   	if (ret != EFI_SUCCESS)
>   		return ret;
>   	/* We are always inserting new variables, get rid of the old copy */
>   	efi_var_mem_del(var);
>
> -	return EFI_SUCCESS;
> +	/*
> +	 * Create a volatile variable that userspace apps can dd and
> +	 * update the file contents
> +	 */
> +	ret = efi_var_collect(&buf, &len, EFI_VARIABLE_NON_VOLATILE);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +	var = efi_var_mem_find(&efi_guid_efi_rt_var_file, u"VarToFile", NULL);
> +	if (var)
> +		efi_var_mem_del(var);
> +
> +	ret = efi_var_mem_ins(u"VarToFile", &efi_guid_efi_rt_var_file,
> +			      EFI_VARIABLE_RUNTIME_ACCESS, len, buf, 0,
> +			      NULL, time);
> +	return ret;
>   } else
>
>   	return EFI_UNSUPPORTED;
> @@ -557,11 +593,11 @@ void efi_variables_boot_exit_notify(void)
>   	const efi_guid_t efi_guid_efi_rt_var_file = U_BOOT_EFI_RT_VAR_FILE_GUID;
>   	const efi_guid_t rt_prop_guid = EFI_RT_PROPERTIES_TABLE_GUID;
>   	efi_status_t ret;
> +	struct efi_var_file *buf;
> +	loff_t len;
> +	bool fail = false;
>
>   	if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> -		struct efi_rt_properties_table *rt_prop =
> -			efi_get_configuration_table(&rt_prop_guid);
> -
>   		ret = efi_set_variable_int(u"RTStorageVolatile",
>   					   &efi_guid_efi_rt_var_file,
>   					   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> @@ -569,11 +605,47 @@ void efi_variables_boot_exit_notify(void)
>   					   EFI_VARIABLE_READ_ONLY,
>   					   sizeof(EFI_VAR_FILE_NAME),
>   					   EFI_VAR_FILE_NAME, false);
> +		if (ret != EFI_SUCCESS) {
> +			fail = true;
> +			goto out;
> +		}
> +
> +		ret = efi_var_collect(&buf, &len, EFI_VARIABLE_NON_VOLATILE);
> +		if (ret != EFI_SUCCESS) {
> +			fail = true;
> +			goto out;
> +		}
> +
> +		ret = efi_set_variable_int(u"VarToFile",
> +					   &efi_guid_efi_rt_var_file,
> +					   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +					   EFI_VARIABLE_RUNTIME_ACCESS,
> +					   len,
> +					   buf, false);
>   		if (ret != EFI_SUCCESS)
> -			rt_prop->runtime_services_supported |= ~EFI_RT_SUPPORTED_SET_VARIABLE;
> -		else
> -			log_err("Can't RTStorage. SetVariableRT won't be available\n");
> +			fail = true;
> +out:
> +		if (fail) {
> +			efi_set_variable_int(u"RTStorageVolatile",
> +					     &efi_guid_efi_rt_var_file,
> +					     EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +					     EFI_VARIABLE_RUNTIME_ACCESS |
> +					     EFI_VARIABLE_READ_ONLY, 0, 0,
> +					     false);

Not providing VarToFile because the memory buffer is more than half
filled before ExitBootServices() is unexpected behavior. This needs rework.

It is not necessary to create VarToFile in the internal memory buffer.
You could generate it when the user calls GetVariable() in the user
provided buffer.

Best regards

Heinrich

> +			efi_set_variable_int(u"VarToFile",
> +					     &efi_guid_efi_rt_var_file,
> +					     EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +					     EFI_VARIABLE_RUNTIME_ACCESS, 0, 0,
> +					     false);
> +		} else {
> +			struct efi_rt_properties_table *rt_prop =
> +				efi_get_configuration_table(&rt_prop_guid);
> +
> +			rt_prop->runtime_services_supported |=
> +					EFI_RT_SUPPORTED_SET_VARIABLE;
> +		}
>   	}
> +
>   	/* Switch variable services functions to runtime version */
>   	efi_runtime_services.get_variable = efi_get_variable_runtime;
>   	efi_runtime_services.get_next_variable_name =
> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> index dde135fd9f81..9d0e270591ea 100644
> --- a/lib/efi_loader/efi_variable_tee.c
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -969,7 +969,6 @@ void efi_variables_boot_exit_notify(void)
>   		log_err("Can't populate EFI variables. No runtime variables will be available\n");
>   	else
>   		efi_var_buf_update(var_buf);
> -	free(var_buf);
>
>   	/* Update runtime service table */
>   	efi_runtime_services.query_variable_info =
> --
> 2.37.2
>
Ilias Apalodimas April 8, 2024, 8:12 a.m. UTC | #2 
Hi Heinrich

On Mon, 8 Apr 2024 at 09:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/6/24 16:01, Ilias Apalodimas wrote:
> > Previous patches enabled SetVariableRT using a RAM backend.
> > Although EBBR [0] defines a variable format we can teach userspace tools
> > and write the altered variables, it's better if we skip the ABI
> > requirements completely.
> >
> > So let's add a new variable, in its own namespace called "VarToFile"
> > which contains a binary dump of the updated RT, BS and, NV variables.
> >
> > Some adjustments are needed to do that. Currently we discard BS-only
> > variables in EBS(). We need to preserve those on the OS RAM backend
> > that exposes the variables. Since BS-only variables can't appear at RT
> > we need to move the memory masking checks from efi_var_collect() to
> > efi_get_next_variable_name_mem()/efi_get_variable_mem() and do the
> > filtering at runtime. We also need to make efi_var_collect() available
> > at runtime, in order to construct the "VarToFile" buffer with BS, RT &
> > NV variables.
> >
> > All users and applications (for linux) have to do when updating a variable
> > is dd that variable in the file described by "RTStorageVolatile".
> >
> > Linux efivarfs uses a first 4 bytes of the output to represent attributes
> > in little-endian format. So, storing variables works like this:
> >
> > $~ efibootmgr -n 0001
> > $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
> >
> > [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
>
> This change actually doubles the amount of memory needed to store a
> variable at runtime. How do you reflect this in QueryVariableInfo()?
>

It doesn't double it. Only BS, RT, NV variables are added to VarToFile.
QueryVariableInfo isn't supported at runtime, but with the current
code we have at boot time the VarToFile would be included in the
calculations.

> >
> > Suggested-by:Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   include/efi_variable.h            |  15 +++-
> >   lib/efi_loader/efi_boottime.c     |   2 +
> >   lib/efi_loader/efi_var_common.c   |  43 +++++------
> >   lib/efi_loader/efi_var_file.c     |   1 -
> >   lib/efi_loader/efi_var_mem.c      |  90 ++++++++++-------------
> >   lib/efi_loader/efi_variable.c     | 118 ++++++++++++++++++++++++------

[...]

> >    * @data:           buffer to which the variable value is copied
> >    * @timep:          authentication time (seconds since start of epoch)
> > + * @mask:            bitmask with required attributes of variables to be collected.
> > + *                      variables are only collected if all of the required
>
> This line looks incomplete.

Yes, fixed

>
> >    * Return:          status code
> >    */
> >   efi_status_t __efi_runtime
> >   efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >                    u32 *attributes, efi_uintn_t *data_size, void *data,
> > -                  u64 *timep);
> > +                  u64 *timep, u32 mask);
> >
> >   /**

[...]

> >       return EFI_UNSUPPORTED;
> > @@ -557,11 +593,11 @@ void efi_variables_boot_exit_notify(void)
> >       const efi_guid_t efi_guid_efi_rt_var_file = U_BOOT_EFI_RT_VAR_FILE_GUID;
> >       const efi_guid_t rt_prop_guid = EFI_RT_PROPERTIES_TABLE_GUID;
> >       efi_status_t ret;
> > +     struct efi_var_file *buf;
> > +     loff_t len;
> > +     bool fail = false;
> >
> >       if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> > -             struct efi_rt_properties_table *rt_prop =
> > -                     efi_get_configuration_table(&rt_prop_guid);
> > -
> >               ret = efi_set_variable_int(u"RTStorageVolatile",
> >                                          &efi_guid_efi_rt_var_file,
> >                                          EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > @@ -569,11 +605,47 @@ void efi_variables_boot_exit_notify(void)
> >                                          EFI_VARIABLE_READ_ONLY,
> >                                          sizeof(EFI_VAR_FILE_NAME),
> >                                          EFI_VAR_FILE_NAME, false);
> > +             if (ret != EFI_SUCCESS) {
> > +                     fail = true;
> > +                     goto out;
> > +             }
> > +
> > +             ret = efi_var_collect(&buf, &len, EFI_VARIABLE_NON_VOLATILE);
> > +             if (ret != EFI_SUCCESS) {
> > +                     fail = true;
> > +                     goto out;
> > +             }
> > +
> > +             ret = efi_set_variable_int(u"VarToFile",
> > +                                        &efi_guid_efi_rt_var_file,
> > +                                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                        EFI_VARIABLE_RUNTIME_ACCESS,
> > +                                        len,
> > +                                        buf, false);
> >               if (ret != EFI_SUCCESS)
> > -                     rt_prop->runtime_services_supported |= ~EFI_RT_SUPPORTED_SET_VARIABLE;
> > -             else
> > -                     log_err("Can't RTStorage. SetVariableRT won't be available\n");
> > +                     fail = true;
> > +out:
> > +             if (fail) {
> > +                     efi_set_variable_int(u"RTStorageVolatile",
> > +                                          &efi_guid_efi_rt_var_file,
> > +                                          EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                          EFI_VARIABLE_RUNTIME_ACCESS |
> > +                                          EFI_VARIABLE_READ_ONLY, 0, 0,
> > +                                          false);
>
> Not providing VarToFile because the memory buffer is more than half
> filled before ExitBootServices() is unexpected behavior. This needs rework.

Why? We should add the SetVariable at runtime to the documentation.

>
> It is not necessary to create VarToFile in the internal memory buffer.
> You could generate it when the user calls GetVariable() in the user
> provided buffer.

Yes, but that would make some functions look a bit messier, since we
have to inject code specifically looking for VarToFile.
OTOH it would make SetVariable at runtime easier to read since all the
size calculations and variable creation would go away.
But in any case, we have to add the variable at runtime (perhaps with
a value of 0?) -- users need to know it's there to be able to read it.
But if you are fine with the special conditions in GetVariable at
runtime, I can easily move the logic to GetVariable.

Thanks
/Ilias

>
> Best regards
>
> Heinrich
>
> > +                     efi_set_variable_int(u"VarToFile",
> > +                                          &efi_guid_efi_rt_var_file,
> > +                                          EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                          EFI_VARIABLE_RUNTIME_ACCESS, 0, 0,
> > +                                          false);
> > +             } else {
> > +                     struct efi_rt_properties_table *rt_prop =
> > +                             efi_get_configuration_table(&rt_prop_guid);
> > +
> > +                     rt_prop->runtime_services_supported |=
> > +                                     EFI_RT_SUPPORTED_SET_VARIABLE;
> > +             }
> >       }
> > +
> >       /* Switch variable services functions to runtime version */
> >       efi_runtime_services.get_variable = efi_get_variable_runtime;
> >       efi_runtime_services.get_next_variable_name =
> > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> > index dde135fd9f81..9d0e270591ea 100644
> > --- a/lib/efi_loader/efi_variable_tee.c
> > +++ b/lib/efi_loader/efi_variable_tee.c
> > @@ -969,7 +969,6 @@ void efi_variables_boot_exit_notify(void)
> >               log_err("Can't populate EFI variables. No runtime variables will be available\n");
> >       else
> >               efi_var_buf_update(var_buf);
> > -     free(var_buf);
> >
> >       /* Update runtime service table */
> >       efi_runtime_services.query_variable_info =
> > --
> > 2.37.2
> >
>
Heinrich Schuchardt April 8, 2024, 8:29 a.m. UTC | #3 
On 4/8/24 10:12, Ilias Apalodimas wrote:
> Hi Heinrich
>
> On Mon, 8 Apr 2024 at 09:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 4/6/24 16:01, Ilias Apalodimas wrote:
>>> Previous patches enabled SetVariableRT using a RAM backend.
>>> Although EBBR [0] defines a variable format we can teach userspace tools
>>> and write the altered variables, it's better if we skip the ABI
>>> requirements completely.
>>>
>>> So let's add a new variable, in its own namespace called "VarToFile"
>>> which contains a binary dump of the updated RT, BS and, NV variables.
>>>
>>> Some adjustments are needed to do that. Currently we discard BS-only
>>> variables in EBS(). We need to preserve those on the OS RAM backend
>>> that exposes the variables. Since BS-only variables can't appear at RT
>>> we need to move the memory masking checks from efi_var_collect() to
>>> efi_get_next_variable_name_mem()/efi_get_variable_mem() and do the
>>> filtering at runtime. We also need to make efi_var_collect() available
>>> at runtime, in order to construct the "VarToFile" buffer with BS, RT &
>>> NV variables.
>>>
>>> All users and applications (for linux) have to do when updating a variable
>>> is dd that variable in the file described by "RTStorageVolatile".
>>>
>>> Linux efivarfs uses a first 4 bytes of the output to represent attributes
>>> in little-endian format. So, storing variables works like this:
>>>
>>> $~ efibootmgr -n 0001
>>> $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
>>>
>>> [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
>>
>> This change actually doubles the amount of memory needed to store a
>> variable at runtime. How do you reflect this in QueryVariableInfo()?
>>
>
> It doesn't double it. Only BS, RT, NV variables are added to VarToFile.
> QueryVariableInfo isn't supported at runtime, but with the current
> code we have at boot time the VarToFile would be included in the
> calculations.

VarToFile does not exist at boot time. Nothing stops the user from
writing NV variables to fill up the memory buffer. VarToFile will be as
large as the sum of NV variables and will not fit into the buffer when
efi_variables_boot_exit_notify() is invoked.

>
>>>
>>> Suggested-by:Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>>    include/efi_variable.h            |  15 +++-
>>>    lib/efi_loader/efi_boottime.c     |   2 +
>>>    lib/efi_loader/efi_var_common.c   |  43 +++++------
>>>    lib/efi_loader/efi_var_file.c     |   1 -
>>>    lib/efi_loader/efi_var_mem.c      |  90 ++++++++++-------------
>>>    lib/efi_loader/efi_variable.c     | 118 ++++++++++++++++++++++++------
>
> [...]
>
>>>     * @data:           buffer to which the variable value is copied
>>>     * @timep:          authentication time (seconds since start of epoch)
>>> + * @mask:            bitmask with required attributes of variables to be collected.
>>> + *                      variables are only collected if all of the required
>>
>> This line looks incomplete.
>
> Yes, fixed
>
>>
>>>     * Return:          status code
>>>     */
>>>    efi_status_t __efi_runtime
>>>    efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>>>                     u32 *attributes, efi_uintn_t *data_size, void *data,
>>> -                  u64 *timep);
>>> +                  u64 *timep, u32 mask);
>>>
>>>    /**
>
> [...]
>
>>>        return EFI_UNSUPPORTED;
>>> @@ -557,11 +593,11 @@ void efi_variables_boot_exit_notify(void)
>>>        const efi_guid_t efi_guid_efi_rt_var_file = U_BOOT_EFI_RT_VAR_FILE_GUID;
>>>        const efi_guid_t rt_prop_guid = EFI_RT_PROPERTIES_TABLE_GUID;
>>>        efi_status_t ret;
>>> +     struct efi_var_file *buf;
>>> +     loff_t len;
>>> +     bool fail = false;
>>>
>>>        if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
>>> -             struct efi_rt_properties_table *rt_prop =
>>> -                     efi_get_configuration_table(&rt_prop_guid);
>>> -
>>>                ret = efi_set_variable_int(u"RTStorageVolatile",
>>>                                           &efi_guid_efi_rt_var_file,
>>>                                           EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> @@ -569,11 +605,47 @@ void efi_variables_boot_exit_notify(void)
>>>                                           EFI_VARIABLE_READ_ONLY,
>>>                                           sizeof(EFI_VAR_FILE_NAME),
>>>                                           EFI_VAR_FILE_NAME, false);
>>> +             if (ret != EFI_SUCCESS) {
>>> +                     fail = true;
>>> +                     goto out;
>>> +             }
>>> +
>>> +             ret = efi_var_collect(&buf, &len, EFI_VARIABLE_NON_VOLATILE);
>>> +             if (ret != EFI_SUCCESS) {
>>> +                     fail = true;
>>> +                     goto out;
>>> +             }
>>> +
>>> +             ret = efi_set_variable_int(u"VarToFile",
>>> +                                        &efi_guid_efi_rt_var_file,
>>> +                                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> +                                        EFI_VARIABLE_RUNTIME_ACCESS,
>>> +                                        len,
>>> +                                        buf, false);
>>>                if (ret != EFI_SUCCESS)
>>> -                     rt_prop->runtime_services_supported |= ~EFI_RT_SUPPORTED_SET_VARIABLE;
>>> -             else
>>> -                     log_err("Can't RTStorage. SetVariableRT won't be available\n");
>>> +                     fail = true;
>>> +out:
>>> +             if (fail) {
>>> +                     efi_set_variable_int(u"RTStorageVolatile",
>>> +                                          &efi_guid_efi_rt_var_file,
>>> +                                          EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> +                                          EFI_VARIABLE_RUNTIME_ACCESS |
>>> +                                          EFI_VARIABLE_READ_ONLY, 0, 0,
>>> +                                          false);
>>
>> Not providing VarToFile because the memory buffer is more than half
>> filled before ExitBootServices() is unexpected behavior. This needs rework.
>
> Why? We should add the SetVariable at runtime to the documentation.

Depending on the number of variables existing at boot time support you
will be creating VarToFile or not. I can't see that users will fancy this.

Best regards

Heinrich

>
>>
>> It is not necessary to create VarToFile in the internal memory buffer.
>> You could generate it when the user calls GetVariable() in the user
>> provided buffer.
>
> Yes, but that would make some functions look a bit messier, since we
> have to inject code specifically looking for VarToFile.
> OTOH it would make SetVariable at runtime easier to read since all the
> size calculations and variable creation would go away.
> But in any case, we have to add the variable at runtime (perhaps with
> a value of 0?) -- users need to know it's there to be able to read it.
> But if you are fine with the special conditions in GetVariable at
> runtime, I can easily move the logic to GetVariable.
>
> Thanks
> /Ilias
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> +                     efi_set_variable_int(u"VarToFile",
>>> +                                          &efi_guid_efi_rt_var_file,
>>> +                                          EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> +                                          EFI_VARIABLE_RUNTIME_ACCESS, 0, 0,
>>> +                                          false);
>>> +             } else {
>>> +                     struct efi_rt_properties_table *rt_prop =
>>> +                             efi_get_configuration_table(&rt_prop_guid);
>>> +
>>> +                     rt_prop->runtime_services_supported |=
>>> +                                     EFI_RT_SUPPORTED_SET_VARIABLE;
>>> +             }
>>>        }
>>> +
>>>        /* Switch variable services functions to runtime version */
>>>        efi_runtime_services.get_variable = efi_get_variable_runtime;
>>>        efi_runtime_services.get_next_variable_name =
>>> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
>>> index dde135fd9f81..9d0e270591ea 100644
>>> --- a/lib/efi_loader/efi_variable_tee.c
>>> +++ b/lib/efi_loader/efi_variable_tee.c
>>> @@ -969,7 +969,6 @@ void efi_variables_boot_exit_notify(void)
>>>                log_err("Can't populate EFI variables. No runtime variables will be available\n");
>>>        else
>>>                efi_var_buf_update(var_buf);
>>> -     free(var_buf);
>>>
>>>        /* Update runtime service table */
>>>        efi_runtime_services.query_variable_info =
>>> --
>>> 2.37.2
>>>
>>
Ilias Apalodimas April 8, 2024, 9:40 a.m. UTC | #4 
On Mon, 8 Apr 2024 at 11:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/8/24 10:12, Ilias Apalodimas wrote:
> > Hi Heinrich
> >
> > On Mon, 8 Apr 2024 at 09:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 4/6/24 16:01, Ilias Apalodimas wrote:
> >>> Previous patches enabled SetVariableRT using a RAM backend.
> >>> Although EBBR [0] defines a variable format we can teach userspace tools
> >>> and write the altered variables, it's better if we skip the ABI
> >>> requirements completely.
> >>>
> >>> So let's add a new variable, in its own namespace called "VarToFile"
> >>> which contains a binary dump of the updated RT, BS and, NV variables.
> >>>
> >>> Some adjustments are needed to do that. Currently we discard BS-only
> >>> variables in EBS(). We need to preserve those on the OS RAM backend
> >>> that exposes the variables. Since BS-only variables can't appear at RT
> >>> we need to move the memory masking checks from efi_var_collect() to
> >>> efi_get_next_variable_name_mem()/efi_get_variable_mem() and do the
> >>> filtering at runtime. We also need to make efi_var_collect() available
> >>> at runtime, in order to construct the "VarToFile" buffer with BS, RT &
> >>> NV variables.
> >>>
> >>> All users and applications (for linux) have to do when updating a variable
> >>> is dd that variable in the file described by "RTStorageVolatile".
> >>>
> >>> Linux efivarfs uses a first 4 bytes of the output to represent attributes
> >>> in little-endian format. So, storing variables works like this:
> >>>
> >>> $~ efibootmgr -n 0001
> >>> $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
> >>>
> >>> [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> >>
> >> This change actually doubles the amount of memory needed to store a
> >> variable at runtime. How do you reflect this in QueryVariableInfo()?
> >>
> >
> > It doesn't double it. Only BS, RT, NV variables are added to VarToFile.
> > QueryVariableInfo isn't supported at runtime, but with the current
> > code we have at boot time the VarToFile would be included in the
> > calculations.
>
> VarToFile does not exist at boot time. Nothing stops the user from
> writing NV variables to fill up the memory buffer. VarToFile will be as
> large as the sum of NV variables and will not fit into the buffer when
> efi_variables_boot_exit_notify() is invoked.

Yes, tbh when I was initially writing this, I was allocating 2x the
variable size buffer to get away from this limitation. But I
eventually decided variables for embedded would be small enough to not
suffer from this that much.
Anyway, I can either allocate 2x size and adjust some code size checks
or move the variable filling in GetVariable as you suggested earlier.
Any preference?

Thanks
/Ilias


>
> >
> >>>
> >>> Suggested-by:Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> ---
> >>>    include/efi_variable.h            |  15 +++-
> >>>    lib/efi_loader/efi_boottime.c     |   2 +
> >>>    lib/efi_loader/efi_var_common.c   |  43 +++++------
> >>>    lib/efi_loader/efi_var_file.c     |   1 -
> >>>    lib/efi_loader/efi_var_mem.c      |  90 ++++++++++-------------
> >>>    lib/efi_loader/efi_variable.c     | 118 ++++++++++++++++++++++++------
> >
> > [...]
> >
> >>>     * @data:           buffer to which the variable value is copied
> >>>     * @timep:          authentication time (seconds since start of epoch)
> >>> + * @mask:            bitmask with required attributes of variables to be collected.
> >>> + *                      variables are only collected if all of the required
> >>
> >> This line looks incomplete.
> >
> > Yes, fixed
> >
> >>
> >>>     * Return:          status code
> >>>     */
> >>>    efi_status_t __efi_runtime
> >>>    efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >>>                     u32 *attributes, efi_uintn_t *data_size, void *data,
> >>> -                  u64 *timep);
> >>> +                  u64 *timep, u32 mask);
> >>>
> >>>    /**
> >
> > [...]
> >
> >>>        return EFI_UNSUPPORTED;
> >>> @@ -557,11 +593,11 @@ void efi_variables_boot_exit_notify(void)
> >>>        const efi_guid_t efi_guid_efi_rt_var_file = U_BOOT_EFI_RT_VAR_FILE_GUID;
> >>>        const efi_guid_t rt_prop_guid = EFI_RT_PROPERTIES_TABLE_GUID;
> >>>        efi_status_t ret;
> >>> +     struct efi_var_file *buf;
> >>> +     loff_t len;
> >>> +     bool fail = false;
> >>>
> >>>        if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> >>> -             struct efi_rt_properties_table *rt_prop =
> >>> -                     efi_get_configuration_table(&rt_prop_guid);
> >>> -
> >>>                ret = efi_set_variable_int(u"RTStorageVolatile",
> >>>                                           &efi_guid_efi_rt_var_file,
> >>>                                           EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> @@ -569,11 +605,47 @@ void efi_variables_boot_exit_notify(void)
> >>>                                           EFI_VARIABLE_READ_ONLY,
> >>>                                           sizeof(EFI_VAR_FILE_NAME),
> >>>                                           EFI_VAR_FILE_NAME, false);
> >>> +             if (ret != EFI_SUCCESS) {
> >>> +                     fail = true;
> >>> +                     goto out;
> >>> +             }
> >>> +
> >>> +             ret = efi_var_collect(&buf, &len, EFI_VARIABLE_NON_VOLATILE);
> >>> +             if (ret != EFI_SUCCESS) {
> >>> +                     fail = true;
> >>> +                     goto out;
> >>> +             }
> >>> +
> >>> +             ret = efi_set_variable_int(u"VarToFile",
> >>> +                                        &efi_guid_efi_rt_var_file,
> >>> +                                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> +                                        EFI_VARIABLE_RUNTIME_ACCESS,
> >>> +                                        len,
> >>> +                                        buf, false);
> >>>                if (ret != EFI_SUCCESS)
> >>> -                     rt_prop->runtime_services_supported |= ~EFI_RT_SUPPORTED_SET_VARIABLE;
> >>> -             else
> >>> -                     log_err("Can't RTStorage. SetVariableRT won't be available\n");
> >>> +                     fail = true;
> >>> +out:
> >>> +             if (fail) {
> >>> +                     efi_set_variable_int(u"RTStorageVolatile",
> >>> +                                          &efi_guid_efi_rt_var_file,
> >>> +                                          EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> +                                          EFI_VARIABLE_RUNTIME_ACCESS |
> >>> +                                          EFI_VARIABLE_READ_ONLY, 0, 0,
> >>> +                                          false);
> >>
> >> Not providing VarToFile because the memory buffer is more than half
> >> filled before ExitBootServices() is unexpected behavior. This needs rework.
> >
> > Why? We should add the SetVariable at runtime to the documentation.
>
> Depending on the number of variables existing at boot time support you
> will be creating VarToFile or not. I can't see that users will fancy this.
>
> Best regards
>
> Heinrich
>
> >
> >>
> >> It is not necessary to create VarToFile in the internal memory buffer.
> >> You could generate it when the user calls GetVariable() in the user
> >> provided buffer.
> >
> > Yes, but that would make some functions look a bit messier, since we
> > have to inject code specifically looking for VarToFile.
> > OTOH it would make SetVariable at runtime easier to read since all the
> > size calculations and variable creation would go away.
> > But in any case, we have to add the variable at runtime (perhaps with
> > a value of 0?) -- users need to know it's there to be able to read it.
> > But if you are fine with the special conditions in GetVariable at
> > runtime, I can easily move the logic to GetVariable.
> >
> > Thanks
> > /Ilias
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +                     efi_set_variable_int(u"VarToFile",
> >>> +                                          &efi_guid_efi_rt_var_file,
> >>> +                                          EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> +                                          EFI_VARIABLE_RUNTIME_ACCESS, 0, 0,
> >>> +                                          false);
> >>> +             } else {
> >>> +                     struct efi_rt_properties_table *rt_prop =
> >>> +                             efi_get_configuration_table(&rt_prop_guid);
> >>> +
> >>> +                     rt_prop->runtime_services_supported |=
> >>> +                                     EFI_RT_SUPPORTED_SET_VARIABLE;
> >>> +             }
> >>>        }
> >>> +
> >>>        /* Switch variable services functions to runtime version */
> >>>        efi_runtime_services.get_variable = efi_get_variable_runtime;
> >>>        efi_runtime_services.get_next_variable_name =
> >>> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> >>> index dde135fd9f81..9d0e270591ea 100644
> >>> --- a/lib/efi_loader/efi_variable_tee.c
> >>> +++ b/lib/efi_loader/efi_variable_tee.c
> >>> @@ -969,7 +969,6 @@ void efi_variables_boot_exit_notify(void)
> >>>                log_err("Can't populate EFI variables. No runtime variables will be available\n");
> >>>        else
> >>>                efi_var_buf_update(var_buf);
> >>> -     free(var_buf);
> >>>
> >>>        /* Update runtime service table */
> >>>        efi_runtime_services.query_variable_info =
> >>> --
> >>> 2.37.2
> >>>
> >>
>
diff mbox series

Patch

diff --git a/include/efi_variable.h b/include/efi_variable.h
index 42a2b7c52bef..8963339b9bb6 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -271,13 +271,15 @@  const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
  *
  * @variable_name_size:	size of variable_name buffer in bytes
  * @variable_name:	name of uefi variable's name in u16
+ * @mask:		bitmask with required attributes of variables to be collected.
+ *                      variables are only collected if all of the required
  * @vendor:		vendor's guid
  *
  * Return: status code
  */
 efi_status_t __efi_runtime
 efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
-			       efi_guid_t *vendor);
+			       efi_guid_t *vendor, u32 mask);
 /**
  * efi_get_variable_mem() - Runtime common code across efi variable
  *                          implementations for GetVariable() from
@@ -289,12 +291,14 @@  efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
  * @data_size:		size of the buffer to which the variable value is copied
  * @data:		buffer to which the variable value is copied
  * @timep:		authentication time (seconds since start of epoch)
+ * @mask:		bitmask with required attributes of variables to be collected.
+ *                      variables are only collected if all of the required
  * Return:		status code
  */
 efi_status_t __efi_runtime
 efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
 		     u32 *attributes, efi_uintn_t *data_size, void *data,
-		     u64 *timep);
+		     u64 *timep, u32 mask);

 /**
  * efi_get_variable_runtime() - runtime implementation of GetVariable()
@@ -334,4 +338,11 @@  efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
  */
 void efi_var_buf_update(struct efi_var_file *var_buf);

+/**
+ * efi_prealloced_rt_memory() - Get a pointer to preallocated EFI memory
+ *                              available at runtime
+ *
+ * Return: pointer to preallocated runtime usable buffer
+ */
+void __efi_runtime *efi_prealloced_rt_memory(void);
 #endif
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 1951291747cd..39481c89a688 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -97,6 +97,8 @@  const efi_guid_t efi_guid_load_file_protocol = EFI_LOAD_FILE_PROTOCOL_GUID;
 const efi_guid_t efi_guid_load_file2_protocol = EFI_LOAD_FILE2_PROTOCOL_GUID;
 /* GUID of the SMBIOS table */
 const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
+/* used by special U-Boot variables during SetVariableRT */
+const efi_guid_t efi_guid_efi_rt_var_file = U_BOOT_EFI_RT_VAR_FILE_GUID;

 static efi_status_t EFIAPI efi_disconnect_controller(
 					efi_handle_t controller_handle,
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index 07b9603d49f3..4abc90e411e7 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -182,7 +182,8 @@  efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
 {
 	efi_status_t ret;

-	ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
+	ret = efi_get_variable_mem(variable_name, guid, attributes, data_size,
+				   data, NULL, EFI_VARIABLE_RUNTIME_ACCESS);

 	/* Remove EFI_VARIABLE_READ_ONLY flag */
 	if (attributes)
@@ -195,7 +196,8 @@  efi_status_t __efi_runtime EFIAPI
 efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
 				   u16 *variable_name, efi_guid_t *guid)
 {
-	return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
+	return efi_get_next_variable_name_mem(variable_name_size, variable_name,
+					      guid, EFI_VARIABLE_RUNTIME_ACCESS);
 }

 /**
@@ -427,18 +429,15 @@  void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
  *
  * Return:	Status code
  */
-efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp,
-					    u32 check_attr_mask)
+efi_status_t __efi_runtime
+efi_var_collect(struct efi_var_file **bufp, loff_t *lenp, u32 check_attr_mask)
 {
 	size_t len = EFI_VAR_BUF_SIZE;
 	struct efi_var_file *buf;
 	struct efi_var_entry *var, *old_var;
 	size_t old_var_name_length = 2;

-	*bufp = NULL; /* Avoid double free() */
-	buf = calloc(1, len);
-	if (!buf)
-		return EFI_OUT_OF_RESOURCES;
+	buf = (struct efi_var_file *)efi_prealloced_rt_memory();
 	var = buf->var;
 	old_var = var;
 	for (;;) {
@@ -451,32 +450,26 @@  efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
 			return EFI_BUFFER_TOO_SMALL;

 		var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
-		memcpy(var->name, old_var->name, old_var_name_length);
-		guidcpy(&var->guid, &old_var->guid);
-		ret = efi_get_next_variable_name_int(
-				&var_name_length, var->name, &var->guid);
+		efi_memcpy_runtime(var->name, old_var->name, old_var_name_length);
+		efi_memcpy_runtime(&var->guid, &old_var->guid, sizeof(efi_guid_t));
+		ret = efi_get_next_variable_name_mem(&var_name_length, var->name,
+						     &var->guid, check_attr_mask);
 		if (ret == EFI_NOT_FOUND)
 			break;
-		if (ret != EFI_SUCCESS) {
-			free(buf);
+		if (ret != EFI_SUCCESS)
 			return ret;
-		}
 		old_var_name_length = var_name_length;
 		old_var = var;

 		data = (u8 *)var->name + old_var_name_length;
 		data_length = (uintptr_t)buf + len - (uintptr_t)data;
-		ret = efi_get_variable_int(var->name, &var->guid,
+		ret = efi_get_variable_mem(var->name, &var->guid,
 					   &var->attr, &data_length, data,
-					   &var->time);
-		if (ret != EFI_SUCCESS) {
-			free(buf);
+					   &var->time, check_attr_mask);
+		if (ret != EFI_SUCCESS)
 			return ret;
-		}
-		if ((var->attr & check_attr_mask) == check_attr_mask) {
-			var->length = data_length;
-			var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8);
-		}
+		var->length = data_length;
+		var = (struct efi_var_entry *)ALIGN((uintptr_t)data + data_length, 8);
 	}

 	buf->reserved = 0;
@@ -490,5 +483,3 @@  efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *

 	return EFI_SUCCESS;
 }
-
-
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
index 413e1794e88c..8614e3d34706 100644
--- a/lib/efi_loader/efi_var_file.c
+++ b/lib/efi_loader/efi_var_file.c
@@ -83,7 +83,6 @@  efi_status_t efi_var_to_file(void)
 error:
 	if (ret != EFI_SUCCESS)
 		log_err("Failed to persist EFI variables\n");
-	free(buf);
 	return ret;
 #else
 	return EFI_SUCCESS;
diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
index 6c21cec5d457..a7af0604733e 100644
--- a/lib/efi_loader/efi_var_mem.c
+++ b/lib/efi_loader/efi_var_mem.c
@@ -16,6 +16,7 @@ 
  * relocation during SetVirtualAddressMap().
  */
 static struct efi_var_file __efi_runtime_data *efi_var_buf;
+static void __efi_runtime_data *efi_rt_prealloced;
 static struct efi_var_entry __efi_runtime_data *efi_current_var;

 /**
@@ -184,53 +185,6 @@  u64 __efi_runtime efi_var_mem_free(void)
 	       sizeof(struct efi_var_entry);
 }

-/**
- * efi_var_mem_bs_del() - delete boot service only variables
- */
-static void efi_var_mem_bs_del(void)
-{
-	struct efi_var_entry *var = efi_var_buf->var;
-
-	for (;;) {
-		struct efi_var_entry *last;
-
-		last = (struct efi_var_entry *)
-		       ((uintptr_t)efi_var_buf + efi_var_buf->length);
-		if (var >= last)
-			break;
-		if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
-			u16 *data;
-
-			/* skip variable */
-			for (data = var->name; *data; ++data)
-				;
-			++data;
-			var = (struct efi_var_entry *)
-			      ALIGN((uintptr_t)data + var->length, 8);
-		} else {
-			/* delete variable */
-			efi_var_mem_del(var);
-		}
-	}
-}
-
-/**
- * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
- *
- * @event:	callback event
- * @context:	callback context
- */
-static void EFIAPI
-efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
-{
-	EFI_ENTRY("%p, %p", event, context);
-
-	/* Delete boot service only variables */
-	efi_var_mem_bs_del();
-
-	EFI_EXIT(EFI_SUCCESS);
-}
-
 /**
  * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
  *
@@ -241,6 +195,7 @@  static void EFIAPI __efi_runtime
 efi_var_mem_notify_virtual_address_map(struct efi_event *event, void *context)
 {
 	efi_convert_pointer(0, (void **)&efi_var_buf);
+	efi_convert_pointer(0, (void **)&efi_rt_prealloced);
 	efi_current_var = NULL;
 }

@@ -261,13 +216,21 @@  efi_status_t efi_var_mem_init(void)
 	efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
 	efi_var_buf->length = (uintptr_t)efi_var_buf->var -
 			      (uintptr_t)efi_var_buf;
-	/* crc32 for 0 bytes = 0 */

-	ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
-			       efi_var_mem_notify_exit_boot_services, NULL,
-			       NULL, &event);
+	/*
+	 * efi_var_collect() needs to run at runtime and provide us
+	 * copies of variables used for the VarToFile variable.
+	 * Preallocate memory equal to the variable storage and
+	 * preserve it to copy variables around
+	 */
+	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
+				 EFI_RUNTIME_SERVICES_DATA,
+				 efi_size_in_pages(EFI_VAR_BUF_SIZE),
+				 &memory);
 	if (ret != EFI_SUCCESS)
 		return ret;
+	efi_rt_prealloced = (void *)(uintptr_t)memory;
+
 	ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
 			       efi_var_mem_notify_virtual_address_map, NULL,
 			       NULL, &event);
@@ -279,7 +242,7 @@  efi_status_t efi_var_mem_init(void)
 efi_status_t __efi_runtime
 efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
 		     u32 *attributes, efi_uintn_t *data_size, void *data,
-		     u64 *timep)
+		     u64 *timep, u32 mask)
 {
 	efi_uintn_t old_size;
 	struct efi_var_entry *var;
@@ -291,6 +254,9 @@  efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
 	if (!var)
 		return EFI_NOT_FOUND;

+	if (mask && !((var->attr & mask) == mask))
+		return EFI_NOT_FOUND;
+
 	if (attributes)
 		*attributes = var->attr;
 	if (timep)
@@ -315,7 +281,8 @@  efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,

 efi_status_t __efi_runtime
 efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
-			       u16 *variable_name, efi_guid_t *vendor)
+			       u16 *variable_name, efi_guid_t *vendor,
+			       u32 mask)
 {
 	struct efi_var_entry *var;
 	efi_uintn_t len, old_size;
@@ -324,6 +291,7 @@  efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
 	if (!variable_name_size || !variable_name || !vendor)
 		return EFI_INVALID_PARAMETER;

+skip:
 	len = *variable_name_size >> 1;
 	if (u16_strnlen(variable_name, len) == len)
 		return EFI_INVALID_PARAMETER;
@@ -347,6 +315,11 @@  efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
 	efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
 	efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));

+	if (mask && !((var->attr & mask) == mask)) {
+		*variable_name_size = old_size;
+		goto skip;
+	}
+
 	return EFI_SUCCESS;
 }

@@ -354,3 +327,14 @@  void efi_var_buf_update(struct efi_var_file *var_buf)
 {
 	memcpy(efi_var_buf, var_buf, EFI_VAR_BUF_SIZE);
 }
+
+void __efi_runtime *efi_prealloced_rt_memory(void)
+{
+	char *s;
+	int count = EFI_VAR_BUF_SIZE;
+
+	s = (char *)efi_rt_prealloced;
+	while (count--)
+		*s++ = 0;
+	return efi_rt_prealloced;
+}
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index f97c8c57f75c..4f529169ea54 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -22,6 +22,8 @@ 
 #include <u-boot/crc.h>
 #include <asm/sections.h>

+static const efi_guid_t __efi_runtime_data efi_guid_efi_rt_var_file =
+						U_BOOT_EFI_RT_VAR_FILE_GUID;
 #ifdef CONFIG_EFI_SECURE_BOOT

 /**
@@ -208,14 +210,16 @@  efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
 		     u32 *attributes, efi_uintn_t *data_size, void *data,
 		     u64 *timep)
 {
-	return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
+	return efi_get_variable_mem(variable_name, vendor, attributes, data_size,
+				    data, timep, 0);
 }

 efi_status_t __efi_runtime
 efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
 			       u16 *variable_name, efi_guid_t *vendor)
 {
-	return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
+	return efi_get_next_variable_name_mem(variable_name_size, variable_name,
+					      vendor, 0);
 }

 /**
@@ -479,6 +483,8 @@  if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
 	efi_uintn_t ret;
 	bool append, delete;
 	u64 time = 0;
+	struct efi_var_file *buf;
+	loff_t len;

 	/*
 	 * Authenticated variables are not supported the rest of the checks
@@ -520,30 +526,60 @@  if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
 			return EFI_NOT_FOUND;
 	}

-	if (delete) {
+	if (!delete) {
+		/*
+		 * We always insert new variabes and delete the old one when
+		 * appending
+		 */
+		len = 2 * (u16_strlen(variable_name) + 1) + data_size +
+			sizeof(struct efi_var_entry);
+		if (var && append)
+			 len += 2 * var->length;
+		/*
+		 * We will copy the variable update into VarToFile,
+		 * account for it twice
+		 */
+		len *= 2;
+		if (len > efi_var_mem_free())
+			return EFI_OUT_OF_RESOURCES;
+		if (append && var) {
+			u16 *old_data = var->name;
+
+			for (; *old_data; ++old_data)
+				;
+			++old_data;
+			ret = efi_var_mem_ins(variable_name, vendor, attributes,
+					      var->length, old_data, data_size,
+					      data, time);
+		} else {
+			ret = efi_var_mem_ins(variable_name, vendor, attributes,
+					      data_size, data, 0, NULL, time);
+		}
+	} else {
 		/* EFI_NOT_FOUND has been handled before */
 		attributes = var->attr;
 		ret = EFI_SUCCESS;
-	} else if (append && var) {
-		u16 *old_data = var->name;
-
-		for (; *old_data; ++old_data)
-			;
-		++old_data;
-		ret = efi_var_mem_ins(variable_name, vendor, attributes,
-				      var->length, old_data, data_size, data,
-				      time);
-	} else {
-		ret = efi_var_mem_ins(variable_name, vendor, attributes,
-				      data_size, data, 0, NULL, time);
 	}
-
 	if (ret != EFI_SUCCESS)
 		return ret;
 	/* We are always inserting new variables, get rid of the old copy */
 	efi_var_mem_del(var);

-	return EFI_SUCCESS;
+	/*
+	 * Create a volatile variable that userspace apps can dd and
+	 * update the file contents
+	 */
+	ret = efi_var_collect(&buf, &len, EFI_VARIABLE_NON_VOLATILE);
+	if (ret != EFI_SUCCESS)
+		return ret;
+	var = efi_var_mem_find(&efi_guid_efi_rt_var_file, u"VarToFile", NULL);
+	if (var)
+		efi_var_mem_del(var);
+
+	ret = efi_var_mem_ins(u"VarToFile", &efi_guid_efi_rt_var_file,
+			      EFI_VARIABLE_RUNTIME_ACCESS, len, buf, 0,
+			      NULL, time);
+	return ret;
 } else

 	return EFI_UNSUPPORTED;
@@ -557,11 +593,11 @@  void efi_variables_boot_exit_notify(void)
 	const efi_guid_t efi_guid_efi_rt_var_file = U_BOOT_EFI_RT_VAR_FILE_GUID;
 	const efi_guid_t rt_prop_guid = EFI_RT_PROPERTIES_TABLE_GUID;
 	efi_status_t ret;
+	struct efi_var_file *buf;
+	loff_t len;
+	bool fail = false;

 	if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
-		struct efi_rt_properties_table *rt_prop =
-			efi_get_configuration_table(&rt_prop_guid);
-
 		ret = efi_set_variable_int(u"RTStorageVolatile",
 					   &efi_guid_efi_rt_var_file,
 					   EFI_VARIABLE_BOOTSERVICE_ACCESS |
@@ -569,11 +605,47 @@  void efi_variables_boot_exit_notify(void)
 					   EFI_VARIABLE_READ_ONLY,
 					   sizeof(EFI_VAR_FILE_NAME),
 					   EFI_VAR_FILE_NAME, false);
+		if (ret != EFI_SUCCESS) {
+			fail = true;
+			goto out;
+		}
+
+		ret = efi_var_collect(&buf, &len, EFI_VARIABLE_NON_VOLATILE);
+		if (ret != EFI_SUCCESS) {
+			fail = true;
+			goto out;
+		}
+
+		ret = efi_set_variable_int(u"VarToFile",
+					   &efi_guid_efi_rt_var_file,
+					   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					   EFI_VARIABLE_RUNTIME_ACCESS,
+					   len,
+					   buf, false);
 		if (ret != EFI_SUCCESS)
-			rt_prop->runtime_services_supported |= ~EFI_RT_SUPPORTED_SET_VARIABLE;
-		else
-			log_err("Can't RTStorage. SetVariableRT won't be available\n");
+			fail = true;
+out:
+		if (fail) {
+			efi_set_variable_int(u"RTStorageVolatile",
+					     &efi_guid_efi_rt_var_file,
+					     EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					     EFI_VARIABLE_RUNTIME_ACCESS |
+					     EFI_VARIABLE_READ_ONLY, 0, 0,
+					     false);
+			efi_set_variable_int(u"VarToFile",
+					     &efi_guid_efi_rt_var_file,
+					     EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					     EFI_VARIABLE_RUNTIME_ACCESS, 0, 0,
+					     false);
+		} else {
+			struct efi_rt_properties_table *rt_prop =
+				efi_get_configuration_table(&rt_prop_guid);
+
+			rt_prop->runtime_services_supported |=
+					EFI_RT_SUPPORTED_SET_VARIABLE;
+		}
 	}
+
 	/* Switch variable services functions to runtime version */
 	efi_runtime_services.get_variable = efi_get_variable_runtime;
 	efi_runtime_services.get_next_variable_name =
diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index dde135fd9f81..9d0e270591ea 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -969,7 +969,6 @@  void efi_variables_boot_exit_notify(void)
 		log_err("Can't populate EFI variables. No runtime variables will be available\n");
 	else
 		efi_var_buf_update(var_buf);
-	free(var_buf);

 	/* Update runtime service table */
 	efi_runtime_services.query_variable_info =