efi_loader: Avoid emitting efi_var_buf to .GOT

Message ID 20210115160016.181511-1-ilias.apalodimas@linaro.org
State New
Headers show
Series
  • efi_loader: Avoid emitting efi_var_buf to .GOT
Related show

Commit Message

Ilias Apalodimas Jan. 15, 2021, 4 p.m.
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(-)

-- 
2.30.0.rc2

Comments

Heinrich Schuchardt Jan. 15, 2021, 6:53 p.m. | #1
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 */

>
Ilias Apalodimas Jan. 15, 2021, 7:11 p.m. | #2
Hi Heinrich, 

[...]
> > 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:


Well, I broke it to begin with so ...

> 

> 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/

> 


Ok

> > + *

> > + * 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."

> 


I'll replace it. Guess I was too concerned pointing out "Don't touch efi_var_buf"

> > + * 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()."

> 


Sure

> Otherwise


I'll wait for Atish to verify it fixes RISC-V, because it makes no difference
whatsoever in arm and send a v2. 

Thanks
/Ilias
> 

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


[...]
Atish Patra Jan. 15, 2021, 7:32 p.m. | #3
On Fri, Jan 15, 2021 at 11:11 AM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>

> Hi Heinrich,

>

> [...]

> > > 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:

>

> Well, I broke it to begin with so ...

>

> >

> > 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/

> >

>

> Ok

>

> > > + *

> > > + * 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."

> >

>

> I'll replace it. Guess I was too concerned pointing out "Don't touch efi_var_buf"

>

> > > + * 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()."

> >

>

> Sure

>

> > Otherwise

>

> I'll wait for Atish to verify it fixes RISC-V, because it makes no difference

> whatsoever in arm and send a v2.

>


Thanks for the quick fix. With this patch, I don't see the panic anymore.
Tested-by: Atish Patra <atish.patra@wdc.com>


> Thanks

> /Ilias

> >

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

>

> [...]




-- 
Regards,
Atish
Atish Patra Jan. 15, 2021, 7:33 p.m. | #4
On Fri, Jan 15, 2021 at 8:00 AM Ilias Apalodimas
<ilias.apalodimas@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 <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

> + *

> + * @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
Ilias Apalodimas Jan. 16, 2021, 3:08 p.m. | #5
On Fri, Jan 15, 2021 at 11:33:40AM -0800, Atish Patra wrote:
> On Fri, Jan 15, 2021 at 8:00 AM Ilias Apalodimas

> <ilias.apalodimas@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.

> 


I replied on the original thread regarding what happens in Arm and we get the 
feature working [1]

[1] https://lists.denx.de/pipermail/u-boot/2021-January/437484.html
Cheers
/Ilias

Patch

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 */