On Fri, Jan 15, 2021 at 8:00 AM Ilias Apalodimas
<ilias.apalodi...@linaro.org> 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.
>
Just curious to know: Is it because of linker script magic or compiler
optimization ?
I might have missed something but I did not find anything relevant in
the arm64 linker scripts.
> * 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
> + *
> + * @var_buf: Source buffer
> + *
> + * 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
> + * 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.
> + */
> +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 */
> --
> 2.30.0.rc2
>
--
Regards,
Atish
