[PATCH] efi_loader: Avoid emitting efi_var_buf to .GOT

[Atish Patra]

On Fri, Jan 15, 2021 at 11:11 AM Ilias Apalodimas
<ilias.apalodimas at 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 at wdc.com>

> Thanks
> /Ilias
> >
> > Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>
> [...]



-- 
Regards,
Atish