Re: [PATCH 0/3] Relocate GOT before calling EFI stub

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>
Subject: Re: [PATCH 0/3] Relocate GOT before calling EFI stub
Date: Tue, 7 Jan 2020 15:28:31 +0100	[thread overview]
Message-ID: <CAKv+Gu8v-5DXWh5ZaY0omrVNh46TEourpcf2Hv3TrsbCUtLFng@mail.gmail.com> (raw)
In-Reply-To: <20200107142732.GB652888@rani.riverdale.lan>

On Tue, 7 Jan 2020 at 15:27, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Tue, Jan 07, 2020 at 03:24:18PM +0100, Ard Biesheuvel wrote:
> > On Tue, 7 Jan 2020 at 15:21, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Tue, Jan 07, 2020 at 03:13:46PM +0100, Ard Biesheuvel wrote:
> > > > On Tue, 7 Jan 2020 at 15:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > > >
> > > > > On Tue, 7 Jan 2020 at 14:55, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > >
> > > > > > This series performs GOT relocation before calling into C code for the
> > > > > > EFI stub. While the stub does not currently require GOT relocation, it's
> > > > > > quite easy to introduce code that will use the GOT on old toolchains,
> > > > > > but not recent ones, which can lead to unexpected issues.
> > > > > >
> > > > > > This is based on
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next
> > > > > >
> > > > > > with commit 4169bb99cd70 ("efi/libstub/x86: use mandatory 16-byte stack
> > > > > > alignment in mixed mode") reverted, as it caused a crash in mixed mode.
> > > > > >
> > > > >
> > > > > Hi Arvind,
> > > > >
> > > > > I appreciate the effort, but I really don't think this is a good idea.
> > > > >
> > > > > A GOT is completely pointless in bare metal code, and fortunately,
> > > > > modern toolchains make it easier to suppress GOT entries from being
> > > > > emitted. So instead of adding back asm code that I just removed, I
> > > > > think it would be better to investigate whether we can get rid of the
> > > > > GOT entirely.
> > > > >
> > > >
> > > > With the following added to arch/x86/boot/compressed/vmlinux.lds.S,
> > > > the 64-bit kernel already links without error.
> > > >
> > > > ASSERT (_got == _egot, "GOT entries detected");
> > > >
> > > > The 32-bit kernel produces a GOT with 3 entries: I'm trying to narrow
> > > > down where they come from.
> > > >
> > > >
> > >
> > > With modern toolchain, 32-bit compressed kernel doesn't actually use the
> > > GOT, however unlike 64-bit, the linker still emits a GOT with the three
> > > reserved entries.
> > >
> > > The rest of the early boot code (after EFI stub) does generate
> > > R_386_GOT32(X) relocations, so we need to use a GOT anyway to cater for
> > > older linkers. Having a build-time check that the EFI stub code does not
> > > have any such relocations might be possible, but it seems easier to just
> > > do the GOT processing for it as well.
> >
> > Not necessarily. My tests with binutils 2.24 building 32-bit suggest
> > that using 'hidden' visibility is often sufficient to get rid of them.
>
> Ah. Could we just add -fvisibility=hidden to the boot/compressed cflags?
> If that works with old toolchain, we could just get rid of the whole
> adjust_got thing.

Unfortunately, the command line option implements a weaker form of
visibility than the pragma, so it probably comes down to setting the
pragma in a .h file that gets -include'd via the command line so it is
guaranteed to be seen first.