Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp

[Ard Biesheuvel]

On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@xxxxxxxxxxx> wrote:
> >
> > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > >>
> > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@xxxxxxxxxxxxx> wrote:
> > >> >
> > >> > Hi Ard,
> > >> >
> > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernelorg> wrote:
> > >> > >
> > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@xxxxxxxxxxxxx> wrote:
> > >> > > >
> > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > >> > > > register for compilation optimization, but the efistub module
> > >> > > > does not initialize gp.
> > >> > > >
> > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@xxxxxxxxxxxxx>
> > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@xxxxxxxxxxxxx>
> > >> > >
> > >> > > This needs a sign-off, and your signoff needs to come after.
> > >> > >
> > >> > > > ---
> > >> > > >  arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > >> > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > >> > > >
> > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > >> > > >         .long   __pecoff_data_virt_end - __pecoff_text_end      // SizeOfInitializedData
> > >> > > >  #endif
> > >> > > >         .long   0                                       // SizeOfUninitializedData
> > >> > > > -       .long   __efistub_efi_pe_entry - _start         // AddressOfEntryPoint
> > >> > > > +       .long   _efistub_entry - _start         // AddressOfEntryPoint
> > >> > > >         .long   efi_header_end - _start                 // BaseOfCode
> > >> > > >  #ifdef CONFIG_32BIT
> > >> > > >         .long  __pecoff_text_end - _start               // BaseOfData
> > >> > > > @@ -121,4 +121,13 @@ section_table:
> > >> > > >
> > >> > > >         .balign 0x1000
> > >> > > >  efi_header_end:
> > >> > > > +
> > >> > > > +       .global _efistub_entry
> > >> > > > +_efistub_entry:
> > >> > >
> > >> > > This should go into .text or .init.text, not the header.
> > >> > >
> > >> > > > +       /* Reload the global pointer */
> > >> > > > +       load_global_pointer
> > >> > > > +
> > >> > >
> > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > >> > > without shadow call stack support, which I guess means that it might
> > >> > > use GP as a global pointer as usual?
> > >> > >
> > >> > > > +       call __efistub_efi_pe_entry
> > >> > > > +       ret
> > >> > > > +
> > >> > >
> > >> > > You are returning to the firmware here, but after modifying the GP
> > >> > > register. Shouldn't you restore it to its old value?
> > >> > There is no need to restore the value of the gp register. Where gp is
> > >> > needed, the gp register must first be initialized. And here is the
> > >> > entry.
> > >> >
> > >>
> > >> But how should the firmware know that GP was corrupted after calling
> > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > >> if it encounters any errors while still running in the EFI boot
> > >> services.
> > >>
> > >
> > > Actually, I wonder if GP can be modified at all before
> > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > point, and so the firmware is being called behind your back, and might
> > > rely on GP retaining its original value.
> >
> > [A few of us are talking on IRC as I'm writing this...]
> >
> > The UEFI spec says "UEFI firmware must neither trust the
> > values of tp and gp nor make an assumption of owning the write access to
> > these register in any circumstances".  It's kind of vague what "UEFI
> > firmware" means here, but I think it's reasonable to assume that the
> > kernel (and thus the EFI stub) is not included there.
> >
> > So under that interpretation, the kernel (including the EFI stub) would
> > be allowed to overwrite GP with whatever it wants.
> >
>
> OK, so even if the UEFI spec seems to suggest that using GP in EFI
> applications such as the Linux EFI stub should be safe, I'd still like
> to understand why this change is necessary. The patches you are
> reverting are supposed to ensure that a) the compiler does not
> generate references that can be relaxed to GP based ones, and b) no
> R_RISCV_RELAX relocations are present in any of the code that runs in
> the context of the EFI firmware.
>
> Are you still seeing GP based symbol references? Is there C code that
> gets pulled into the EFI stub that uses GP based relocations perhaps?
> (see list below). If any of those are implemented in C, they should
> not be used by the EFI stub directly unless they are guaranteed to be
> uninstrumented and callable at arbitrary offsets other than the one
> they were linked to run at.
>
>
> __efistub_memcmp         = memcmp;
> __efistub_memchr         = memchr;
> __efistub_memcpy         = memcpy;
> __efistub_memmove        = memmove;
> __efistub_memset         = memset;
> __efistub_strlen         = strlen;
> __efistub_strnlen        = strnlen;
> __efistub_strcmp         = strcmp;
> __efistub_strncmp        = strncmp;
> __efistub_strrchr        = strrchr;
> __efistub___memcpy       = memcpy;
> __efistub___memmove      = memmove;
> __efistub___memset       = memset;
> __efistub__start         = _start;
> __efistub__start_kernel  = _start_kernel;
>
> (from arch/riscv/kernel/image-vars.h)

Uhm never mind - these are all gone now, I was looking at a v6.1
kernel source tree.

So that means that, as far as I can tell, the only kernel C code that
executes in the context of the EFI firmware is built with -mno-relax
and is checked for the absence of R_RISCV_RELAX relocations. So I fail
to see why these changes are needed.

Yunhui, could you please explain the reason for this series?