Received-SPF: pass (google.com: domain of linux-kernel+bounces-95882-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <CAMj1kXH1oMbONoHFMPaatfaqrHNE2ryfrG7kw-7J-eFsuXkK-Q@mail.gmail.com>
 <mhng-c53211c1-4708-459a-bdc5-6e013c2adaee@palmer-ri-x1c9>
 <CAMj1kXEFqMx8qUHQEbg=OqbG-H0Zpj-nWu=a6qhhvNEZPO7f4Q@mail.gmail.com>
 <CAMj1kXG4SXsBfNqWMRUJ+AVv=6trWUAow-f8Mk5oKCpO=WueFg@mail.gmail.com> <CAEEQ3wkN3HDUuPDfWTn4kTxKH03OaRxBTFru3jJzZgW+BVhABg@mail.gmail.com>
In-Reply-To: <CAEEQ3wkN3HDUuPDfWTn4kTxKH03OaRxBTFru3jJzZgW+BVhABg@mail.gmail.com>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Thu, 7 Mar 2024 17:48:49 +0100
Message-ID: <CAMj1kXHuKbaXqWuFuMXhfL1_2w05CfJrk2uAPOW2HNHdpEnxXA@mail.gmail.com>
Subject: Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization
 of gp
To: yunhui cui <cuiyunhui@bytedance.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.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, Conor Dooley <conor@kernel.org>, 
	Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, 7 Mar 2024 at 04:19, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Thu, Mar 7, 2024 at 12:15=E2=80=AFAM Ard Biesheuvel <ardb@kernel.org> =
wrote:
> >
> > On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrot=
e:
> > > >
> > > > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wro=
te:
> > > > >>
> > > > >> 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@ke=
rnel.org> wrote:
> > > > >> > >
> > > > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance=
com> wrote:
> > > > >> > > >
> > > > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > >> > > > register for compilation optimization, but the efistub mod=
ule
> > > > >> > > > 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/k=
ernel/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         // Address=
OfEntryPoint
> > > > >> > > >         .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=
=3Dy? The EFI
> > > > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be bu=
ilt
> > > > >> > > 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. Wher=
e 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 cal=
ling
> > > > >> the kernel's EFI entrypoint? The EFI stub can return to the firm=
ware
> > > > >> 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 acce=
ss to
> > > > these register in any circumstances".  It's kind of vague what "UEF=
I
> > > > firmware" means here, but I think it's reasonable to assume that th=
e
> > > > kernel (and thus the EFI stub) is not included there.
> > > >
> > > > So under that interpretation, the kernel (including the EFI stub) w=
ould
> > > > 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 lik=
e
> > > 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         =3D memcmp;
> > > __efistub_memchr         =3D memchr;
> > > __efistub_memcpy         =3D memcpy;
> > > __efistub_memmove        =3D memmove;
> > > __efistub_memset         =3D memset;
> > > __efistub_strlen         =3D strlen;
> > > __efistub_strnlen        =3D strnlen;
> > > __efistub_strcmp         =3D strcmp;
> > > __efistub_strncmp        =3D strncmp;
> > > __efistub_strrchr        =3D strrchr;
> > > __efistub___memcpy       =3D memcpy;
> > > __efistub___memmove      =3D memmove;
> > > __efistub___memset       =3D memset;
> > > __efistub__start         =3D _start;
> > > __efistub__start_kernel  =3D _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?
>
> From the logic of binutils, if "__global_pointer$" exists, it is
> possible to use GP for optimization. For RISC-V, "__global_pointer$"
> was introduced in commit "fbe934d69eb7e". Therefore, for the system as
> a whole, we should keep using GP uniformly.

There is no 'system as a whole' that can use GP 'uniformly'

The EFI stub is a separate executable that runs from a different
mapping of memory, in an execution context managed by the firmware. It
happens to be linked into the same executable as the vmlinux kernel.

> The root cause of this
> problem is that GP is not loaded, rather than "On RISC-V, we also
> avoid GP based relocations..." as commit "d2baf8cc82c17" said.

GP is not loaded because in the EFI firmware context, there is no safe
way to rely on it.

> We need
> to address problems head-on, rather than avoid them.
>

So what solution are you proposing for the potential GP conflicts
between the boot loader, the Linux EFI stub and the firmware?