Re: [PATCH v2 13/21] efi/libstub/x86: drop __efi_early() export of efi_config struct

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Arvind Sankar <nivedita@alum.mit.edu>
Cc: Ard Biesheuvel <ardb@kernel.org>,
	linux-efi <linux-efi@vger.kernel.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Matthew Garrett <matthewgarrett@google.com>,
	Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v2 13/21] efi/libstub/x86: drop __efi_early() export of efi_config struct
Date: Thu, 2 Jan 2020 18:48:52 +0100	[thread overview]
Message-ID: <CAKv+Gu-NDps2EP865v45sUbKRwSc2fwvcfLt64BKX_D=ErmvMQ@mail.gmail.com> (raw)
In-Reply-To: <20200102174142.GA713602@rani.riverdale.lan>

On Thu, 2 Jan 2020 at 18:41, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Thu, Jan 02, 2020 at 06:30:46PM +0100, Ard Biesheuvel wrote:
> > On Thu, 2 Jan 2020 at 18:26, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Thu, Jan 02, 2020 at 05:59:27PM +0100, Ard Biesheuvel wrote:
> > > > On Thu, 2 Jan 2020 at 17:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > > >
> > > > > On Thu, 2 Jan 2020 at 16:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > > > >
> > > > > > On Thu, 2 Jan 2020 at 16:51, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > > >
> > > > > > > On Thu, Jan 02, 2020 at 04:20:46PM +0100, Ard Biesheuvel wrote:
> > > > > > > >
> > > > > > > > Yeah, good point.
> > > > > > > >
> > > > > > > > I pushed as branch here;
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-libstub-attr-const
> > > > > > > >
> > > > > > > > Could you please check if that fixes the issue for efi_is_64bit() ?
> > > > > > >
> > > > > > > Haven't built it yet -- but how does this handle the GOT issues you
> > > > > > > mentioned?
> > > > > >
> > > > > > It doesn't. The trick is to add __attribute__((visibility("hidden")))
> > > > > > to the extern declaration of efi_is64, but I am having trouble to
> > > > > > reproduce the original issue.
> > > > >
> > > > > Some background:
> > > > >
> > > > > https://lore.kernel.org/lkml/5405E186.2080406@canonical.com/
> > > > > https://lore.kernel.org/lkml/20140919104021.GA11552@gmail.com/
> > > >
> > > > OK, I have done a bit more digging, and it seems like recent
> > > > toolchains can optimize away GOT indirections using R_386_GOT32X
> > > > relocations, which can be converted into R_386_GOTOFF relocations by
> > > > the linker if it is building a fully linked binary, making the actual
> > > > contents of the GOT entries irrelevant.
> > > >
> > > > Note that even if the GOT entries are not fixed up, assigning a global
> > > > variable and then using it again may work by accident if the memory it
> > > > points to is writable, which is why it is not easy to reproduce
> > > > reliably.
> > > >
> > > > efi_is64 only exists on 64-bit, so annotating that as 'hidden' should
> > > > work. But efi_system_table() is also used on 32-bit, so I'll leave
> > > > that one alone for now.
> > >
> > > With hidden visibility, 32-bit compiler seems to generate GOTOFF
> > > relocations directly so it shouldn't depend on recent toolchain?
> > >
> > > https://godbolt.org/z/79iA_3
> >
> > Yes, that was my assumption at the time as well, but we still ended up
> > with /some/ absolute GOT dereferences for some reason in the 32-bit
> > kernel.
>
> Do you remember if we ever figured out what was wrong with Matt
> Fleming's patch to fixup the GOT for the EFI stub too?
>

No, I don't think we did. It is simply infeasible to diagnose this on
someone else's laptop who is on the other side of the world.

> For efi_system_table, if we don't want to risk creating GOT entries, one
> option would be to do what you did for is64, but define the wrapper
> function as an out-of-line function in the assembler code and declare it
> const.

Yeah, that would work. It would be for 32-bit only, so we could have a
__weak implementation in C, and an asm one for i386.

Currently, I am not managing to get the 64-bit compiler to emit any
GOT based references for efi_is64, though, even without the attribute,
so I am not sure what's going on there.