| Message ID | 20200723075317.731582-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: Enable run-time variable support for tee based variables | expand |
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@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; >
On Thu, Jul 23, 2020 at 12:32:01PM +0200, Heinrich Schuchardt wrote: > 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@linaro.org> > > Overall the changes look good. Some cleanup is needed. Thanks, I'll sent a v2 shortly. All of your remarks made sense Cheers /Ilias > > > --- > > 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; > > >
On Thu, Jul 23, 2020 at 12:53 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> 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@linaro.org> > --- > 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 > + * > + * 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 > + * @check_attr_mask: mask of variable attributes which will be included in the buffer > + * 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; I am a bit confused how this will work. This means it will reside in GOT which is not mapped in virtual address for Linux. Whenever we try to invoke get_variable service, it will panic. Did we miss a trick in RISC-V ? Here are the details of the issue we are seeing. http://lists.infradead.org/pipermail/linux-riscv/2021-January/004200.html > 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; > > *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; > } > > /** > @@ -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 */ > + ret = efi_var_mem_init(); > + if (ret != EFI_SUCCESS) > + return ret; > + > ret = get_max_payload(&max_payload_size); > if (ret != EFI_SUCCESS) > return ret; > -- > 2.28.0.rc1 > -- Regards, Atish
Hi Atish, > > > > 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; > > I am a bit confused how this will work. This means it will reside in GOT > which is not mapped in virtual address for Linux. Whenever we try to > invoke get_variable service, it will panic. > Did we miss a trick in RISC-V ? > > Here are the details of the issue we are seeing. > > http://lists.infradead.org/pipermail/linux-riscv/2021-January/004200.html > Thanks for reporting this. I can't make too much from the dump itself. Since there's a qemu config for riscv I'll reproduce it. Long shot but, during LTO, the whole executable is compiled in one go. I think that if in that phase it observes that GOT entries never change it converts them to relative references. I think we are either looking into some compiler differences here or maybe a linker script trick. In any case that's not the right way to go. FWIW I just tested on arm64 and my .got table is empty. Since this will work if we switch it back to a static pointer, that should be easy to fix and the correct way to do it since we'll be unaffected by compiler/linker changes. In U-Boot we support 2 ways for runtime variables. One is generic, by using a piece of runtime data memory for the variables and the other one is very arm specific. In both cases though the runtime memory backend is used to expose the variables to the kernel. So that variable can remain static and instead code a function internally to efi_var_mem.c and do the memcpy we need. I've never tested it on risc-v but apparently I should up more tests for cases like that. I'll send a patch tomorrow once I gather all the objdump info to make sure we aren't missing anything. Regards /Ilias
On Jan 14 2021, Atish Patra wrote: > I am a bit confused how this will work. This means it will reside in GOT > which is not mapped in virtual address for Linux. Whenever we try to > invoke get_variable service, it will panic. > Did we miss a trick in RISC-V ? I think the problem really is that RISC-V use -fpic for compiling. If I change that to -fpie, there is no longer a GOT reference. Andreas.
Hi Andreas, On Fri, Jan 15, 2021 at 05:34:04PM +0100, Andreas Schwab wrote: > On Jan 14 2021, Atish Patra wrote: > > > I am a bit confused how this will work. This means it will reside in GOT > > which is not mapped in virtual address for Linux. Whenever we try to > > invoke get_variable service, it will panic. > > Did we miss a trick in RISC-V ? > > I think the problem really is that RISC-V use -fpic for compiling. If I > change that to -fpie, there is no longer a GOT reference. The -fpic explains why the GOT is there to begin with, as you say. Keep in mind it's present in Arm as well. What I am trying to explain in the mail is that regardless of the -fpic, Arm gets rid of all GOT indirections. The section is actually empty and they all turn into relative references. That's why that works fine on Arm. The reason for that (I think, if I am wrong somebody shout please), is that you only need a GOT in shared libraries for symbol pre-emption. So if both the library and the executable define a global 'bar', the lib is supposed to switch the references to the executable exposed symbol. So on Arm the linker observes that's not the case, and uses relative references, while it gets rid of the GOT section entries (again shout if I am wrong :)). Anyway removing -fpic should work as well, but I'd rather do this [1], instead of relying on linker flags. [1] https://lists.denx.de/pipermail/u-boot/2021-January/437478.html Cheers /Ilias > > Andreas. > > -- > Andreas Schwab, SUSE Labs, schwab@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different."
On Jan 15 2021, Ilias Apalodimas wrote: > Anyway removing -fpic should work as well, but I'd rather do this [1], > instead of relying on linker flags. It's not the linker that breaks this, but the compiler, by forcing GOT addressing. And it can easily break again any time. Andreas.
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 + * + * 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 + * @check_attr_mask: mask of variable attributes which will be included in the buffer + * 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; *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; } /** @@ -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 */ + ret = efi_var_mem_init(); + if (ret != EFI_SUCCESS) + return ret; + ret = get_max_payload(&max_payload_size); if (ret != EFI_SUCCESS) return ret;
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@linaro.org> --- 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(-)