Received-SPF: pass (google.com: domain of linux-kernel+bounces-93977-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240306085622.87248-1-cuiyunhui@bytedance.com>
 <20240306085622.87248-3-cuiyunhui@bytedance.com> <CAMj1kXEjjFAeVAVwiDO22RJECSM=L=0q6J=zog7JR38rUZpLGQ@mail.gmail.com>
 <CAEEQ3w=2pX+pjwoz=hNFpR4thD+d6o9OmBob8LMzZ8BbKZ=pqg@mail.gmail.com>
 <CAMj1kXFG=y_XXiaBHZOXDApZiCb48i0U5pX+GBRO2KAUom52VA@mail.gmail.com> <CAMj1kXH1oMbONoHFMPaatfaqrHNE2ryfrG7kw-7J-eFsuXkK-Q@mail.gmail.com>
In-Reply-To: <CAMj1kXH1oMbONoHFMPaatfaqrHNE2ryfrG7kw-7J-eFsuXkK-Q@mail.gmail.com>
From: yunhui cui <cuiyunhui@bytedance.com>
Date: Wed, 6 Mar 2024 21:30:52 +0800
Message-ID: <CAEEQ3wnuqBYcrwaRhx46E2kbwcHR_miE4+JzK=CmYhF0zMA07Q@mail.gmail.com>
Subject: Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization
 of gp
To: Ard Biesheuvel <ardb@kernel.org>
Cc: paul.walmsley@sifive.com, palmer@dabbelt.com, aou@eecs.berkeley.edu, 
	xuzhipeng.1973@bytedance.com, alexghiti@rivosinc.com, samitolvanen@google.com, 
	bp@alien8.de, xiao.w.wang@intel.com, jan.kiszka@siemens.com, 
	kirill.shutemov@linux.intel.com, nathan@kernel.org, 
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, 
	linux-efi@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Ard,

On Wed, Mar 6, 2024 at 9:09=E2=80=AFPM Ard Biesheuvel <ardb@kernel.org> wro=
te:
>
> On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote=
:
> > >
> > > Hi Ard,
> > >
> > > On Wed, Mar 6, 2024 at 5:36=E2=80=AFPM Ard Biesheuvel <ardb@kernel.or=
g> wrote:
> > > >
> > > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> w=
rote:
> > > > >
> > > > > 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@bytedance.com>
> > > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > >
> > > > 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/e=
fi-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                                       // SizeOf=
UninitializedData
> > > > > -       .long   __efistub_efi_pe_entry - _start         // Addres=
sOfEntryPoint
> > > > > +       .long   _efistub_entry - _start         // AddressOfEntry=
Point
> > > > >         .long   efi_header_end - _start                 // BaseOf=
Code
> > > > >  #ifdef CONFIG_32BIT
> > > > >         .long  __pecoff_text_end - _start               // BaseOf=
Data
> > > > > @@ -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=3Dy? Th=
e EFI
> > > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > > without shadow call stack support, which I guess means that it migh=
t
> > > > 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.

OK, in v2 I will restore the value of gp as follows:

  efi_header_end:
+
+ __INIT
+ .global_efistub_entry
+_efistub_entry:
+ /* Reload the global pointer */
+.option push
+.option norelax
+ addi sp,sp,-8
+ sd gp,0(sp)
+ la gp, __global_pointer$
+.option pop
+ call __efistub_efi_pe_entry
+ld gp,0(sp)
+addi sp,sp,8
+ret
+ __HEAD
+
         .endm

what do you think?

Thanks,
Yunhui