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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 6 Mar 2024 20:34:21 +0800
From: yunhui cui <cuiyunhui@...edance.com>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu, 
	xuzhipeng.1973@...edance.com, alexghiti@...osinc.com, samitolvanen@...gle.com, 
	bp@...en8.de, xiao.w.wang@...el.com, jan.kiszka@...mens.com, 
	kirill.shutemov@...ux.intel.com, nathan@...nel.org, 
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-efi@...r.kernel.org
Subject: Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization
 of gp

Hi Ard,

On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@...nel.org> wrote:
>
> On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@...edance.com> 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@...edance.com>
> > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@...edance.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/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.

Regarding your first two comments above, I plan to make the following
changes in v2,
efi_header_end:
+
+       __INIT
+       .global _efistub_entry
+_efistub_entry:
+       /* Reload the global pointer */
+.option push
+.option norelax
+       la gp, __global_pointer$
+.option pop
+
+       call __efistub_efi_pe_entry
+       ret
+       __HEAD
+
        .endm

what do you think?

Thanks,
Yunhui