On 15.01.21 17:00, Ilias Apalodimas wrote:

Atish reports than on RISC-V, accessing the EFI variables causes a kernel panic. An objdump of the file verifies that, since the global pointer for efi_var_buf ends up in .GOT section which is not mapped in virtual address space for Linux.

<snip of efi_var_mem_find>

0000000000000084 <efi_var_mem_find>: 84: 715d addi sp,sp,-80

• objdump -dr

0000000000000086 <.LCFI2>: 86: e0a2 sd s0,64(sp) 88: fc26 sd s1,56(sp) 8a: e486 sd ra,72(sp) 8c: f84a sd s2,48(sp) 8e: f44e sd s3,40(sp) 90: f052 sd s4,32(sp) 92: ec56 sd s5,24(sp) 94: 00000497 auipc s1,0x0 94: R_RISCV_GOT_HI20 efi_var_buf 98: 0004b483 ld s1,0(s1) # 94 <.LCFI2+0xe> 98: R_RISCV_PCREL_LO12_I .L0 98: R_RISCV_RELAX *ABS*

• objdump -t

0000000000000084 g F .text.efi_runtime 00000000000000b8 efi_var_mem_find

With the patch applied:

• objdump -dr

0000000000000086 <.LCFI2>: 86: e0a2 sd s0,64(sp) 88: fc26 sd s1,56(sp) 8a: e486 sd ra,72(sp) 8c: f84a sd s2,48(sp) 8e: f44e sd s3,40(sp) 90: f052 sd s4,32(sp) 92: ec56 sd s5,24(sp) 94: 00000497 auipc s1,0x0 94: R_RISCV_PCREL_HI20 .LANCHOR0 94: R_RISCV_RELAX *ABS* 98: 00048493 mv s1,s1 98: R_RISCV_PCREL_LO12_I .L0 98: R_RISCV_RELAX *ABS*

• objdump -t

0000000000000008 l O .data.efi_runtime 0000000000000008 efi_var_buf

On arm64 this works, because there's no .GOT entries for this and everything is converted to relative references.

• objdump -dr (identical pre-post patch, only the new function shows up)

00000000000000b4 <efi_var_mem_find>: b4: aa0003ee mov x14, x0 b8: 9000000a adrp x10, 0 <efi_var_mem_compare> b8: R_AARCH64_ADR_PREL_PG_HI21 .data.efi_runtime bc: 91000140 add x0, x10, #0x0 bc: R_AARCH64_ADD_ABS_LO12_NC .data.efi_runtime c0: aa0103ed mov x13, x1 c4: 79400021 ldrh w1, [x1] c8: aa0203eb mov x11, x2 cc: f9400400 ldr x0, [x0, #8] d0: b940100c ldr w12, [x0, #16] d4: 8b0c000c add x12, x0, x12

So let's switch efi_var_buf to static and create a helper function for anyone that needs to update it.

Fixes: e01aed47d6a0 ("efi_loader: Enable run-time variable support for tee based variables") Reported-by: Atish Patra atishp@atishpatra.org Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

Atish can you give it a spin and let me know if this fixes the issue for you? The objdump seems to be correct now, but I am not familiar with RISC-V. No regressions on Arm with TEE or memory backed variables. include/efi_variable.h | 12 ++++++++++++ lib/efi_loader/efi_var_mem.c | 12 +++++++++++- lib/efi_loader/efi_variable_tee.c | 2 +- 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/efi_variable.h b/include/efi_variable.h index 4704a3c16e65..b2317eb7bf1c 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -306,4 +306,16 @@ 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);

+/**

• • efi_var_buf_update() - Update the value of efi_var_buf in efi_var_mem.c

Dear Ilias,

thank you for addressing this problem. The code looks fine to me. Just some ideas concerning comment lines:

The value of efi_var_buf is the address it is pointing to. So i would prefer:

efi_var_buf_update() - udpate memory buffer for variables

• • @var_buf: Source buffer

%s/Source/source/

• • efi_var_buf is special since we use it on Runtime Services. We need
• • to keep it static in efi_var_mem.c and avoid having it pulled into
• • .GOT. Since it has to be static this function must be used to update

You already place a comment about .GOT where the declaration is. Here describing how the function is used would be of interest. E.g.

"This function copies to the memory buffer for UEFI variables. Call this function in ExitBootServices() if memory backed variables are only used at runtime to fill the buffer."

• • it
• */

+void efi_var_buf_update(struct efi_var_file *var_buf);

#endif diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c index d155f25f60e6..fcf0043b5d3b 100644 --- a/lib/efi_loader/efi_var_mem.c +++ b/lib/efi_loader/efi_var_mem.c @@ -10,7 +10,12 @@ #include <efi_variable.h> #include <u-boot/crc.h>

-struct efi_var_file __efi_runtime_data *efi_var_buf; +/*

• • keep efi_var_buf as static , moving it out might move it to .got
• • which is not mapped in virtual address for Linux. Whenever
• • we try to invoke get_variable service, it will panic.

Not everybody will know the abbreviation .got. How about:

"The variables efi_var_file and efi_var_entry must be static to avoid that they are referenced via the global offset table (section .got). The GOT is neither mapped as EfiRuntimeServicesData nor do we support its relocation during SetVirtualAddressMap()."

Otherwise

Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de

• */

+static struct efi_var_file __efi_runtime_data *efi_var_buf; static struct efi_var_entry __efi_runtime_data *efi_current_var;

/** @@ -339,3 +344,8 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,

return EFI_SUCCESS; }

+void efi_var_buf_update(struct efi_var_file *var_buf) +{

• memcpy(efi_var_buf, var_buf, EFI_VAR_BUF_SIZE);

+} diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c index be6f3dfad469..c69330443801 100644 --- a/lib/efi_loader/efi_variable_tee.c +++ b/lib/efi_loader/efi_variable_tee.c @@ -692,7 +692,7 @@ void efi_variables_boot_exit_notify(void) 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);
  

• efi_var_buf_update(var_buf);
  

  free(var_buf);

  /* Update runtime service table */