Linux-EFI Archive mirror
 help / color / mirror / Atom feed
From: Palmer Dabbelt <palmer@dabbelt.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: cuiyunhui@bytedance.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
Subject: Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
Date: Wed, 06 Mar 2024 07:21:27 -0800 (PST)	[thread overview]
Message-ID: <mhng-c53211c1-4708-459a-bdc5-6e013c2adaee@palmer-ri-x1c9> (raw)
In-Reply-To: <CAMj1kXH1oMbONoHFMPaatfaqrHNE2ryfrG7kw-7J-eFsuXkK-Q@mail.gmail.com>

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> 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 PM Ard Biesheuvel <ardb@kernel.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 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/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.

[We're still talking on IRC, though]

  parent reply	other threads:[~2024-03-06 15:21 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-06  8:56 [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used" Yunhui Cui
2024-03-06  8:56 ` [PATCH 2/3] Revert "riscv/efistub: Tighten ELF relocation check" Yunhui Cui
2024-03-06  8:56 ` [PATCH 3/3] efistub: fix missed the initialization of gp Yunhui Cui
2024-03-06  9:36   ` Ard Biesheuvel
2024-03-06 12:34     ` [External] " yunhui cui
2024-03-06 13:02       ` Ard Biesheuvel
2024-03-06 13:09         ` Ard Biesheuvel
2024-03-06 13:30           ` yunhui cui
2024-03-06 15:21           ` Palmer Dabbelt [this message]
2024-03-06 15:44             ` Ard Biesheuvel
2024-03-06 16:15               ` Ard Biesheuvel
2024-03-06 17:11                 ` Ard Biesheuvel
2024-03-07  3:19                 ` yunhui cui
2024-03-07 16:48                   ` Ard Biesheuvel
2024-03-08  7:09                     ` yunhui cui
2024-03-08  8:16                       ` Ard Biesheuvel
2024-03-06  9:02 ` [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used" Conor Dooley
2024-03-06  9:38   ` Ard Biesheuvel
2024-03-06 12:37     ` [External] " yunhui cui
2024-03-06 12:52 ` Jan Kiszka
2024-03-06 13:07   ` [External] " yunhui cui
2024-03-06 13:11     ` Ard Biesheuvel
2024-03-06 13:26       ` yunhui cui
2024-03-06 13:46         ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=mhng-c53211c1-4708-459a-bdc5-6e013c2adaee@palmer-ri-x1c9 \
    --to=palmer@dabbelt.com \
    --cc=alexghiti@rivosinc.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=ardb@kernel.org \
    --cc=bp@alien8.de \
    --cc=cuiyunhui@bytedance.com \
    --cc=jan.kiszka@siemens.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=nathan@kernel.org \
    --cc=paul.walmsley@sifive.com \
    --cc=samitolvanen@google.com \
    --cc=xiao.w.wang@intel.com \
    --cc=xuzhipeng.1973@bytedance.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).