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 <ati...@atishpatra.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodi...@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.g...@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 */
>
