Hi Ard, On Thu, Mar 7, 2024 at 12:15 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > 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@xxxxxxxxxx> 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? >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. 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. We need to address problems head-on, rather than avoid them. Thanks, Yunhui
