[PATCH] efi_loader: Enable run-time variable support for tee based variables
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu Jul 23 12:32:01 CEST 2020
On 23.07.20 09:53, Ilias Apalodimas wrote:
> We recently added functions for storing/restoring variables
> from a file to a memory backed buffer marked as __efi_runtime_data
> commit f1f990a8c958 ("efi_loader: memory buffer for variables")
> commit 5f7dcf079de8 ("efi_loader: UEFI variable persistence")
>
> Using the same idea we now can support GetVariable() and GetNextVariable()
> on the OP-TEE based variables as well.
>
> So let's re-arrange the code a bit and move the commmon code for
> accessing variables out of efi_variable.c. Create common functions for
> reading variables from memory that both implementations can use on
> run-time. Then just use those functions in the run-time variants of the
> OP-TEE based EFI variable implementation and initialize the memory
> buffer on ExitBootServices()
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
Overall the changes look good. Some cleanup is needed.
> ---
> include/efi_variable.h | 45 ++++++++++++++++++++
> lib/efi_loader/Makefile | 2 +-
> lib/efi_loader/efi_var_file.c | 25 ++++-------
> lib/efi_loader/efi_var_mem.c | 70 ++++++++++++++++++++++++++++++-
> lib/efi_loader/efi_variable.c | 58 +------------------------
> lib/efi_loader/efi_variable_tee.c | 55 ++++++++++++++++++++++--
> 6 files changed, 175 insertions(+), 80 deletions(-)
>
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index 2c629e4dca92..6ef24cd05feb 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -142,6 +142,20 @@ struct efi_var_file {
> */
> efi_status_t efi_var_to_file(void);
>
> +/**
> + * efi_var_collect() - collect non-volatile variables in buffer
Please, remove the reference to non-volatile here.
> + *
> + * A buffer is allocated and filled with all non-volatile variables in a
Same here.
> + * format ready to be written to disk.
Please, describe that the bits set in check_attr_mask must *all* be set
in the attributes of the variable to have the variable collected, e.g.
@check_attr_mask is a bitmask with required variable attributes.
Variables are only collected if all of the required attributes are set.
> + *
> + * @bufp: pointer to pointer of buffer with collected variables
> + * @lenp: pointer to length of buffer
> + * @check_attr_mask: mask of variable attributes which will be included in the buffer
bitmask with required attributes of variables to be collected
> + * Return: status code
> + */
> +efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *lenp,
> + u32 check_attr_mask);
> +
> /**
> * efi_var_restore() - restore EFI variables from buffer
> *
> @@ -233,4 +247,35 @@ efi_status_t efi_init_secure_state(void);
> */
> enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid);
>
> +/**
> + * efi_get_next_variable_name_mem() - Runtime common code across efi variable
> + * implementations for GetNextVariable()
> + * from the cached memory copy
> + * @variable_name_size: size of variable_name buffer in byte
> + * @variable_name: name of uefi variable's name in u16
> + * @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_get_variable_mem() - Runtime common code across efi variable
> + * implementations for GetVariable() from
> + * the cached memory copy
> + *
> + * @variable_name: name of the variable
> + * @vendor: vendor GUID
> + * @attributes: attributes of the variable
> + * @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)
> + * Return: status code
> +
> + */
> +efi_status_t __efi_runtime
> +efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes,
> + efi_uintn_t *data_size, void *data, u64 *timep);
> +
> #endif
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index 441ac9432e99..9bad1d159b03 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -37,11 +37,11 @@ obj-y += efi_setup.o
> obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> obj-y += efi_var_common.o
> obj-y += efi_var_mem.o
> +obj-y += efi_var_file.o
> ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
> obj-y += efi_variable_tee.o
> else
> obj-y += efi_variable.o
> -obj-y += efi_var_file.o
> obj-$(CONFIG_EFI_VARIABLES_PRESEED) += efi_var_seed.o
> endif
> obj-y += efi_watchdog.o
> diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> index 6f9d76f2a2d5..b171d2d1a8f7 100644
> --- a/lib/efi_loader/efi_var_file.c
> +++ b/lib/efi_loader/efi_var_file.c
> @@ -46,18 +46,8 @@ static efi_status_t __maybe_unused efi_set_blk_dev_to_system_partition(void)
> return EFI_SUCCESS;
> }
>
> -/**
> - * efi_var_collect() - collect non-volatile variables in buffer
> - *
> - * A buffer is allocated and filled with all non-volatile variables in a
> - * format ready to be written to disk.
> - *
> - * @bufp: pointer to pointer of buffer with collected variables
> - * @lenp: pointer to length of buffer
> - * Return: status code
> - */
> -static efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp,
> - loff_t *lenp)
> +efi_status_t __maybe_unused 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;
> @@ -102,11 +92,10 @@ static efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp,
> free(buf);
> return ret;
> }
> - if (!(var->attr & EFI_VARIABLE_NON_VOLATILE))
> - continue;
> - var->length = data_length;
> - var = (struct efi_var_entry *)
> - ALIGN((uintptr_t)data + data_length, 8);
> + 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);
> + }
> }
>
> buf->reserved = 0;
> @@ -137,7 +126,7 @@ efi_status_t efi_var_to_file(void)
> loff_t actlen;
> int r;
>
> - ret = efi_var_collect(&buf, &len);
> + ret = efi_var_collect(&buf, &len, EFI_VARIABLE_NON_VOLATILE);
> if (ret != EFI_SUCCESS)
> goto error;
>
> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> index 7a2dba7dc263..fd97d5b56300 100644
> --- a/lib/efi_loader/efi_var_mem.c
> +++ b/lib/efi_loader/efi_var_mem.c
> @@ -10,7 +10,7 @@
> #include <efi_variable.h>
> #include <u-boot/crc.h>
>
> -static struct efi_var_file __efi_runtime_data *efi_var_buf;
> +struct efi_var_file __efi_runtime_data *efi_var_buf;
> static struct efi_var_entry __efi_runtime_data *efi_current_var;
>
> /**
> @@ -264,3 +264,71 @@ efi_status_t efi_var_mem_init(void)
> return ret;
> return ret;
> }
> +
> +efi_status_t __efi_runtime
> +efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes,
> + efi_uintn_t *data_size, void *data, u64 *timep)
> +{
> + efi_uintn_t old_size;
> + struct efi_var_entry *var;
> + u16 *pdata;
> +
> + if (!variable_name || !vendor || !data_size)
> + return EFI_INVALID_PARAMETER;
> + var = efi_var_mem_find(vendor, variable_name, NULL);
> + if (!var)
> + return EFI_NOT_FOUND;
> +
> + if (attributes)
> + *attributes = var->attr;
> + if (timep)
> + *timep = var->time;
> +
> + old_size = *data_size;
> + *data_size = var->length;
> + if (old_size < var->length)
> + return EFI_BUFFER_TOO_SMALL;
> +
> + if (!data)
> + return EFI_INVALID_PARAMETER;
> +
> + for (pdata = var->name; *pdata; ++pdata)
> + ;
> + ++pdata;
> +
> + efi_memcpy_runtime(data, pdata, var->length);
> +
> + return EFI_SUCCESS;
> +}
> +
> +efi_status_t __efi_runtime
> +efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
> + efi_guid_t *vendor)
> +{
> + struct efi_var_entry *var;
> + efi_uintn_t old_size;
> + u16 *pdata;
> +
> + if (!variable_name_size || !variable_name || !vendor)
> + return EFI_INVALID_PARAMETER;
> +
> + efi_var_mem_find(vendor, variable_name, &var);
> +
> + if (!var)
> + return EFI_NOT_FOUND;
> +
> + for (pdata = var->name; *pdata; ++pdata)
> + ;
> + ++pdata;
> +
> + old_size = *variable_name_size;
> + *variable_name_size = (uintptr_t)pdata - (uintptr_t)var->name;
> +
> + if (old_size < *variable_name_size)
> + return EFI_BUFFER_TOO_SMALL;
> +
> + efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
> + efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
> +
> + return EFI_SUCCESS;
> +}
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 39a848290380..e684623aec44 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -282,68 +282,14 @@ efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> u32 *attributes, efi_uintn_t *data_size, void *data,
> u64 *timep)
> {
> - efi_uintn_t old_size;
> - struct efi_var_entry *var;
> - u16 *pdata;
> -
> - if (!variable_name || !vendor || !data_size)
> - return EFI_INVALID_PARAMETER;
> - var = efi_var_mem_find(vendor, variable_name, NULL);
> - if (!var)
> - return EFI_NOT_FOUND;
> -
> - if (attributes)
> - *attributes = var->attr;
> - if (timep)
> - *timep = var->time;
> -
> - old_size = *data_size;
> - *data_size = var->length;
> - if (old_size < var->length)
> - return EFI_BUFFER_TOO_SMALL;
> -
> - if (!data)
> - return EFI_INVALID_PARAMETER;
> -
> - for (pdata = var->name; *pdata; ++pdata)
> - ;
> - ++pdata;
> -
> - efi_memcpy_runtime(data, pdata, var->length);
> -
> - return EFI_SUCCESS;
> + return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
> }
>
> efi_status_t __efi_runtime
> efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
> u16 *variable_name, efi_guid_t *vendor)
> {
> - struct efi_var_entry *var;
> - efi_uintn_t old_size;
> - u16 *pdata;
> -
> - if (!variable_name_size || !variable_name || !vendor)
> - return EFI_INVALID_PARAMETER;
> -
> - efi_var_mem_find(vendor, variable_name, &var);
> -
> - if (!var)
> - return EFI_NOT_FOUND;
> -
> - for (pdata = var->name; *pdata; ++pdata)
> - ;
> - ++pdata;
> -
> - old_size = *variable_name_size;
> - *variable_name_size = (uintptr_t)pdata - (uintptr_t)var->name;
> -
> - if (old_size < *variable_name_size)
> - return EFI_BUFFER_TOO_SMALL;
> -
> - efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
> - efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
> -
> - return EFI_SUCCESS;
> + return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
> }
>
> efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> index 9920351a403e..a699cf414647 100644
> --- a/lib/efi_loader/efi_variable_tee.c
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -15,6 +15,8 @@
> #include <malloc.h>
> #include <mm_communication.h>
>
> +#define OPTEE_PAGE_SIZE BIT(12)
> +extern struct efi_var_file __efi_runtime_data *efi_var_buf;
> static efi_uintn_t max_buffer_size; /* comm + var + func + data */
> static efi_uintn_t max_payload_size; /* func + data */
>
> @@ -238,7 +240,25 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
> goto out;
>
We should check that we received a reasonable value for the payload size
here instead of the (size > 2) check below.
E.g.
if (var_payload->size < sizeof(struct smm_variable_access) + 0x20) {
ret = EFI_DEVICE_ERROR;
goto out:
}
0x20 is the size needed for setting PlatformLang to "en-US".
> *size = var_payload->size;
> -
> + /*
> + * Although the max payload is configurable on StMM, we only share a single
> + * page from OP-TEE for the non-secure buffer used to communicate with StMM.
> + * Since OP-TEE will reject to map anything bigger than that, make sure we are
> + * in bounds.
> + */
> + if (*size > OPTEE_PAGE_SIZE)
> + *size = OPTEE_PAGE_SIZE - MM_COMMUNICATE_HEADER_SIZE -
> + MM_VARIABLE_COMMUNICATE_SIZE;
> + /*
> + * There seems to be a bug in EDK2 miscalculating the boundaries and size
> + * checks, so deduct 2 more bytes to fulfill this requirement. Fix it up here
> + * to ensure backwards compatibility with older versions.
> + * Ref: StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
> + * sizeof (EFI_MM_COMMUNICATE_HEADER) instead the size minus the flexible
> + * array member
> + */
> + if (*size > 2)
> + *size -= 2;
> out:
> free(comm_buf);
> return ret;
> @@ -606,7 +626,15 @@ static efi_status_t __efi_runtime EFIAPI
> efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> u32 *attributes, efi_uintn_t *data_size, void *data)
> {
> - return EFI_UNSUPPORTED;
> + efi_status_t ret;
> +
> + ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
> +
> + /* Remove EFI_VARIABLE_READ_ONLY flag */
> + if (attributes)
> + *attributes &= EFI_VARIABLE_MASK;
> +
> + return ret;
> }
We now have the same runtime functions in lib/efi_loader/efi_variable.c
and lib/efi_loader/efi_variable_tee.c. Please, remove the code duplication.
>
> /**
> @@ -622,7 +650,7 @@ static 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_UNSUPPORTED;
> + return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
> }
>
> /**
> @@ -674,8 +702,10 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> */
> void efi_variables_boot_exit_notify(void)
> {
> - u8 *comm_buf;
> efi_status_t ret;
> + u8 *comm_buf;
> + loff_t len;
> + struct efi_var_file *var_buf;
>
> comm_buf = setup_mm_hdr(NULL, 0,
> SMM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE, &ret);
> @@ -688,6 +718,18 @@ void efi_variables_boot_exit_notify(void)
> log_err("Unable to notify StMM for ExitBootServices\n");
> free(comm_buf);
>
> + /*
> + * Populate the list for runtime variables.
> + * asking EFI_VARIABLE_RUNTIME_ACCESS is redundant, since
> + * efi_var_mem_notify_exit_boot_services will clean those, but that's fine
> + */
> + ret = efi_var_collect(&var_buf, &len, EFI_VARIABLE_RUNTIME_ACCESS);
> + if (ret != EFI_SUCCESS)
> + log_err("Can't populate EFI variables. No runtime variables will be available\n");
> + else
> + memcpy(efi_var_buf, var_buf, len);
> + free(var_buf);
> +
> /* Update runtime service table */
> efi_runtime_services.query_variable_info =
> efi_query_variable_info_runtime;
> @@ -707,6 +749,11 @@ efi_status_t efi_init_variables(void)
> {
> efi_status_t ret;
>
> + /* Create a cached copy of the variables that will be enabled on EBS */
I assume you mean ExitBootServices(). Could you, please, use the full name.
Best regards
Heinrich
> + ret = efi_var_mem_init();
> + if (ret != EFI_SUCCESS)
> + return ret;
> +
> ret = get_max_payload(&max_payload_size);
> if (ret != EFI_SUCCESS)
> return ret;
>
More information about the U-Boot
mailing list