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

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

[Arvind Sankar]

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.

Arvind Sankar (3):
  x86/boot/compressed/64: Make adjust_got easier to use repeatedly
  x86/boot/compressed/32: Allow adjust_got to be called repeatedly
  x86/boot: Perform GOT relocation before calling EFI stub

 arch/x86/boot/compressed/eboot.c   |  4 +-
 arch/x86/boot/compressed/head_32.S | 65 +++++++++++++++++++++----
 arch/x86/boot/compressed/head_64.S | 76 +++++++++++++++++-------------
 3 files changed, 99 insertions(+), 46 deletions(-)

-- 
2.24.1

[PATCH 1/3] x86/boot/compressed/64: Make adjust_got easier to use repeatedly

[Arvind Sankar]

Get the new relocation base address inside adjust_got, and save it so
that the caller doesn't have to figure it out.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_64.S | 58 +++++++++++++-----------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 1f1f6c8139b3..1464d8d0ec66 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -312,20 +312,7 @@ SYM_CODE_START(startup_64)
 	/*
 	 * paging_prepare() and cleanup_trampoline() below can have GOT
 	 * references. Adjust the table with address we are running at.
-	 *
-	 * Zero RAX for adjust_got: the GOT was not adjusted before;
-	 * there's no adjustment to undo.
 	 */
-	xorq	%rax, %rax
-
-	/*
-	 * Calculate the address the binary is loaded at and use it as
-	 * a GOT adjustment.
-	 */
-	call	1f
-1:	popq	%rdi
-	subq	$1b, %rdi
-
 	call	.Ladjust_got
 
 	/*
@@ -412,21 +399,6 @@ trampoline_return:
 	pushq	$0
 	popfq
 
-	/*
-	 * Previously we've adjusted the GOT with address the binary was
-	 * loaded at. Now we need to re-adjust for relocation address.
-	 *
-	 * Calculate the address the binary is loaded at, so that we can
-	 * undo the previous GOT adjustment.
-	 */
-	call	1f
-1:	popq	%rax
-	subq	$1b, %rax
-
-	/* The new adjustment is the relocation address */
-	movq	%rbx, %rdi
-	call	.Ladjust_got
-
 /*
  * Copy the compressed kernel to the end of our buffer
  * where decompression in place becomes safe.
@@ -475,6 +447,12 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
 	shrq	$3, %rcx
 	rep	stosq
 
+/*
+ * Previously we've adjusted the GOT with address the binary was
+ * loaded at. Now we need to re-adjust for relocation address.
+ */
+	call	.Ladjust_got
+
 /*
  * Do the extraction, and jump to the new kernel..
  */
@@ -497,23 +475,33 @@ SYM_FUNC_END(.Lrelocated)
 /*
  * Adjust the global offset table
  *
- * RAX is the previous adjustment of the table to undo (use 0 if it's the
- * first time we touch GOT).
- * RDI is the new adjustment to apply.
+ * The relocation base address calculation uses RIP-relative addressing, so if
+ * the kernel is being relocated to a new address, this function must be called
+ * after execution has been passed to the new location. We keep track of the
+ * relocation address so that it can be backed out if this function is called
+ * repeatedly.
  */
-.Ladjust_got:
+
+SYM_FUNC_START_LOCAL(.Ladjust_got)
+	/* Get the new relocation base address */
+	leaq	startup_32(%rip), %rax
+	/* Backout the previous relocation address if any */
+	subq	got_relocation_base(%rip), %rax
+	/* Store the relocation base address for future reference */
+	addq	%rax, got_relocation_base(%rip)
+
 	/* Walk through the GOT adding the address to the entries */
 	leaq	_got(%rip), %rdx
 	leaq	_egot(%rip), %rcx
 1:
 	cmpq	%rcx, %rdx
 	jae	2f
-	subq	%rax, (%rdx)	/* Undo previous adjustment */
-	addq	%rdi, (%rdx)	/* Apply the new adjustment */
+	addq	%rax, (%rdx)	/* Apply the incremental adjustment */
 	addq	$8, %rdx
 	jmp	1b
 2:
 	ret
+SYM_FUNC_END(.Ladjust_got)
 
 	.code32
 /*
@@ -628,6 +616,8 @@ SYM_DATA_START_LOCAL(gdt)
 	.quad   0x0000000000000000	/* TS continued */
 SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
+SYM_DATA_LOCAL(got_relocation_base, .quad 0)
+
 #ifdef CONFIG_EFI_MIXED
 SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0)
 SYM_DATA(efi_is64, .byte 1)
-- 
2.24.1

[PATCH 2/3] x86/boot/compressed/32: Allow adjust_got to be called repeatedly

[Arvind Sankar]

Save the new relocation address so that this function can be called
safely multiple times.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_32.S | 42 +++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index e43ac17cb9fb..973ac0b51af5 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -173,15 +173,7 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
 /*
  * Adjust our own GOT
  */
-	leal	_got(%ebx), %edx
-	leal	_egot(%ebx), %ecx
-1:
-	cmpl	%ecx, %edx
-	jae	2f
-	addl	%ebx, (%edx)
-	addl	$4, %edx
-	jmp	1b
-2:
+	call	.Ladjust_got
 
 /*
  * Do the extraction, and jump to the new kernel..
@@ -211,6 +203,38 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
 	jmp	*%eax
 SYM_FUNC_END(.Lrelocated)
 
+/*
+ * Adjust the global offset table
+ *
+ * The relocation base address is passed in EBX. If the kernel is being
+ * relocated to a new address, this function must be called after the kernel
+ * has been copied to the new location. We keep track of the relocation address
+ * so that it can be backed out if this function is called repeatedly.
+ */
+
+SYM_FUNC_START_LOCAL(.Ladjust_got)
+	/* Get the new relocation base address */
+	movl	%ebx, %eax
+	/* Backout the previous relocation address if any */
+	subl	got_relocation_base(%ebx), %eax
+	/* Store the relocation base address for future reference */
+	addl	%eax, got_relocation_base(%ebx)
+
+	leal	_got(%ebx), %edx
+	leal	_egot(%ebx), %ecx
+1:
+	cmpl	%ecx, %edx
+	jae	2f
+	addl	%eax, (%edx)
+	addl	$4, %edx
+	jmp	1b
+2:
+	ret
+SYM_FUNC_END(.Ladjust_got)
+
+	.data
+SYM_DATA_LOCAL(got_relocation_base, .long 0)
+
 /*
  * Stack and heap for uncompression
  */
-- 
2.24.1

[PATCH 3/3] x86/boot: Perform GOT relocation before calling EFI stub

[Arvind Sankar]

While the EFI stub code does not generally require a GOT, it is very
easy to write code that would require a GOT with an old toolchain, but
that modern toolchains optimize to eliminate the GOT references. This
can lead to unexpected crashes if the kernel is built with an older
toolchain.

To avoid potential issues, move the efi_pe_entry back to assembler, and
relocate the GOT if present before calling any C code.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/eboot.c   |  4 ++--
 arch/x86/boot/compressed/head_32.S | 23 ++++++++++++++++++++++-
 arch/x86/boot/compressed/head_64.S | 18 ++++++++++++++++++
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index ab3a40283db7..b62f957274aa 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -356,8 +356,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
  * need to create one ourselves (usually the bootloader would create
  * one for us).
  */
-efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
-				   efi_system_table_t *sys_table_arg)
+efi_status_t efi_entry(efi_handle_t handle,
+		       efi_system_table_t *sys_table_arg)
 {
 	struct boot_params *boot_params;
 	struct apm_bios_info *bi;
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 973ac0b51af5..171947ae5b8c 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -145,9 +145,29 @@ SYM_FUNC_START(startup_32)
 SYM_FUNC_END(startup_32)
 
 #ifdef CONFIG_EFI_STUB
+
+SYM_FUNC_START(efi_pe_entry)
+	/* Adjust the GOT */
+	/* Don't clobber EBX as we may return to firmware in case of error */
+	pushl	%ebx
+	call	1f
+1:	popl	%ebx
+	subl	$1b, %ebx
+	call	.Ladjust_got
+	popl	%ebx
+	/* Transfer control to C entry */
+	jmp	efi_entry
+SYM_FUNC_END(efi_pe_entry)
+
 SYM_FUNC_START(efi32_stub_entry)
 SYM_FUNC_START_ALIAS(efi_stub_entry)
-	add	$0x4, %esp
+	/* We cannot return from here */
+	addl	$0x4, %esp
+	/* Adjust the GOT */
+	call	1f
+1:	popl	%ebx
+	subl	$1b, %ebx
+	call	.Ladjust_got
 	call	efi_main
 	movl	%eax, %esi
 	movl	BP_code32_start(%esi), %eax
@@ -155,6 +175,7 @@ SYM_FUNC_START_ALIAS(efi_stub_entry)
 	jmp	*%eax
 SYM_FUNC_END(efi32_stub_entry)
 SYM_FUNC_END_ALIAS(efi_stub_entry)
+
 #endif
 
 	.text
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 1464d8d0ec66..5a295d41a7a8 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -421,9 +421,26 @@ trampoline_return:
 SYM_CODE_END(startup_64)
 
 #ifdef CONFIG_EFI_STUB
+
+SYM_FUNC_START(efi_pe_entry)
+	/* Convert arguments to SysV ABI */
+	movq	%rcx, %rdi
+	movq	%rdx, %rsi
+	/* Adjust the GOT */
+	call	.Ladjust_got
+	/* Transfer control to C entry */
+	jmp	efi_entry
+SYM_FUNC_END(efi_pe_entry)
+
 	.org 0x390
 SYM_FUNC_START(efi64_stub_entry)
 SYM_FUNC_START_ALIAS(efi_stub_entry)
+	/* We cannot return from here */
+	addq	$8, %rsp
+	/* Adjust the GOT */
+	pushq	%rdx
+	call	.Ladjust_got
+	popq	%rdx
 	and	$~0xf, %rsp			/* realign the stack */
 	call	efi_main
 	movq	%rax,%rsi
@@ -432,6 +449,7 @@ SYM_FUNC_START_ALIAS(efi_stub_entry)
 	jmp	*%rax
 SYM_FUNC_END(efi64_stub_entry)
 SYM_FUNC_END_ALIAS(efi_stub_entry)
+
 #endif
 
 	.text
-- 
2.24.1

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

[Ard Biesheuvel]

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.




> Arvind Sankar (3):
>   x86/boot/compressed/64: Make adjust_got easier to use repeatedly
>   x86/boot/compressed/32: Allow adjust_got to be called repeatedly
>   x86/boot: Perform GOT relocation before calling EFI stub
>
>  arch/x86/boot/compressed/eboot.c   |  4 +-
>  arch/x86/boot/compressed/head_32.S | 65 +++++++++++++++++++++----
>  arch/x86/boot/compressed/head_64.S | 76 +++++++++++++++++-------------
>  3 files changed, 99 insertions(+), 46 deletions(-)
>
> --
> 2.24.1
>

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

[Ard Biesheuvel]

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.


>
>
>
> > Arvind Sankar (3):
> >   x86/boot/compressed/64: Make adjust_got easier to use repeatedly
> >   x86/boot/compressed/32: Allow adjust_got to be called repeatedly
> >   x86/boot: Perform GOT relocation before calling EFI stub
> >
> >  arch/x86/boot/compressed/eboot.c   |  4 +-
> >  arch/x86/boot/compressed/head_32.S | 65 +++++++++++++++++++++----
> >  arch/x86/boot/compressed/head_64.S | 76 +++++++++++++++++-------------
> >  3 files changed, 99 insertions(+), 46 deletions(-)
> >
> > --
> > 2.24.1
> >

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

[Arvind Sankar]

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.

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

[Ard Biesheuvel]

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.

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

[Arvind Sankar]

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.

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

[Ard Biesheuvel]

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.

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

[Arvind Sankar]

On Tue, Jan 07, 2020 at 03:28:31PM +0100, Ard Biesheuvel wrote:
> 
> 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.

Tried hacking that in and it works, tested with gcc 4.6.4.

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

[Ard Biesheuvel]

On Tue, 7 Jan 2020 at 18:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Tue, Jan 07, 2020 at 03:28:31PM +0100, Ard Biesheuvel wrote:
> >
> > 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.
>
> Tried hacking that in and it works, tested with gcc 4.6.4.

Excellent. But in my testing locally, I don't get any GOT entries in
the first place, strangely enough. So what changes in the output for
you with visibility hidden compared to before?

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

[Arvind Sankar]

On Tue, Jan 07, 2020 at 06:59:57PM +0100, Ard Biesheuvel wrote:
> On Tue, 7 Jan 2020 at 18:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Tue, Jan 07, 2020 at 03:28:31PM +0100, Ard Biesheuvel wrote:
> > >
> > > 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.
> >
> > Tried hacking that in and it works, tested with gcc 4.6.4.
> 
> Excellent. But in my testing locally, I don't get any GOT entries in
> the first place, strangely enough. So what changes in the output for
> you with visibility hidden compared to before?

Works with 32-bit as well.

Are you checking libstub or boot/compressed? Below is with gcc 4.6 (but
latest binutils). With gcc 9, there's only one left -- trampoline_32bit_src
in pgtable_64.

acpi.o
000000000000025b R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
cmdline.o
00000000000001d3 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
00000000000001f3 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
0000000000000223 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
cpuflags.o
00000000000000a3 R_X86_64_REX_GOTPCRELX  cpu_vendor-0x0000000000000004
00000000000000c5 R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
0000000000000127 R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
0000000000000144 R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
0000000000000153 R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
000000000000017b R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
early_serial_console.o
000000000000005a R_X86_64_REX_GOTPCRELX  early_serial_base-0x0000000000000004
0000000000000165 R_X86_64_REX_GOTPCRELX  early_serial_base-0x0000000000000004
eboot.o
efi_thunk_64.o
error.o
head_64.o
kaslr_64.o
00000000000001e7 R_X86_64_REX_GOTPCRELX  ptrs_per_p4d-0x0000000000000004
0000000000000372 R_X86_64_REX_GOTPCRELX  __default_kernel_pte_mask-0x0000000000000004
000000000000038b R_X86_64_REX_GOTPCRELX  pgdir_shift-0x0000000000000004
00000000000003ac R_X86_64_REX_GOTPCRELX  __pgtable_l5_enabled-0x0000000000000004
000000000000040a R_X86_64_REX_GOTPCRELX  __pgtable_l5_enabled-0x0000000000000004
00000000000004db R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
00000000000004f2 R_X86_64_REX_GOTPCRELX  __pgtable_l5_enabled-0x0000000000000004
0000000000000517 R_X86_64_REX_GOTPCRELX  _pgtable-0x0000000000000004
00000000000005f3 R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
kaslr.o
0000000000000281 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
00000000000003e3 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
0000000000000ba1 R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
0000000000000bb4 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
0000000000000cc8 R_X86_64_REX_GOTPCRELX  free_mem_ptr-0x0000000000000004
0000000000000cd2 R_X86_64_REX_GOTPCRELX  free_mem_end_ptr-0x0000000000000004
0000000000000d6e R_X86_64_REX_GOTPCRELX  free_mem_ptr-0x0000000000000004
0000000000000f63 R_X86_64_REX_GOTPCRELX  __pgtable_l5_enabled-0x0000000000000004
kernel_info.o
mem_encrypt.o
misc.o
0000000000000015 R_X86_64_REX_GOTPCRELX  free_mem_ptr-0x0000000000000004
000000000000001f R_X86_64_REX_GOTPCRELX  free_mem_end_ptr-0x0000000000000004
0000000000002965 R_X86_64_REX_GOTPCRELX  early_serial_base-0x0000000000000004
0000000000002a43 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
0000000000002b8e R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
0000000000002bec R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
0000000000002c10 R_X86_64_REX_GOTPCRELX  free_mem_end_ptr-0x0000000000000004
0000000000002c17 R_X86_64_REX_GOTPCRELX  free_mem_ptr-0x0000000000000004
0000000000002d16 R_X86_64_REX_GOTPCRELX  trampoline_32bit-0x0000000000000004
0000000000002f97 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
0000000000002fea R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
pgtable_64.o
000000000000000d R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
000000000000010a R_X86_64_REX_GOTPCRELX  trampoline_32bit_src-0x0000000000000004
0000000000000191 R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
0000000000000376 R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
piggy.o
string.o

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

[Ard Biesheuvel]

On Tue, 7 Jan 2020 at 19:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Tue, Jan 07, 2020 at 06:59:57PM +0100, Ard Biesheuvel wrote:
> > On Tue, 7 Jan 2020 at 18:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Tue, Jan 07, 2020 at 03:28:31PM +0100, Ard Biesheuvel wrote:
> > > >
> > > > 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.
> > >
> > > Tried hacking that in and it works, tested with gcc 4.6.4.
> >
> > Excellent. But in my testing locally, I don't get any GOT entries in
> > the first place, strangely enough. So what changes in the output for
> > you with visibility hidden compared to before?
>
> Works with 32-bit as well.
>
> Are you checking libstub or boot/compressed? Below is with gcc 4.6 (but
> latest binutils). With gcc 9, there's only one left -- trampoline_32bit_src
> in pgtable_64.
>

I am looking at the size of the .got section in
boot/compressed/vmlinux, and it is 0x0 on 64-bit, and 0xc (i.e., only
the .got.plt fixup code) on 32-bit.

Could you please check whether passing -Bsymbolic to the linker gives
the same result btw?


> acpi.o
> 000000000000025b R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> cmdline.o
> 00000000000001d3 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 00000000000001f3 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 0000000000000223 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> cpuflags.o
> 00000000000000a3 R_X86_64_REX_GOTPCRELX  cpu_vendor-0x0000000000000004
> 00000000000000c5 R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
> 0000000000000127 R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
> 0000000000000144 R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
> 0000000000000153 R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
> 000000000000017b R_X86_64_REX_GOTPCRELX  cpu-0x0000000000000004
> early_serial_console.o
> 000000000000005a R_X86_64_REX_GOTPCRELX  early_serial_base-0x0000000000000004
> 0000000000000165 R_X86_64_REX_GOTPCRELX  early_serial_base-0x0000000000000004
> eboot.o
> efi_thunk_64.o
> error.o
> head_64.o
> kaslr_64.o
> 00000000000001e7 R_X86_64_REX_GOTPCRELX  ptrs_per_p4d-0x0000000000000004
> 0000000000000372 R_X86_64_REX_GOTPCRELX  __default_kernel_pte_mask-0x0000000000000004
> 000000000000038b R_X86_64_REX_GOTPCRELX  pgdir_shift-0x0000000000000004
> 00000000000003ac R_X86_64_REX_GOTPCRELX  __pgtable_l5_enabled-0x0000000000000004
> 000000000000040a R_X86_64_REX_GOTPCRELX  __pgtable_l5_enabled-0x0000000000000004
> 00000000000004db R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
> 00000000000004f2 R_X86_64_REX_GOTPCRELX  __pgtable_l5_enabled-0x0000000000000004
> 0000000000000517 R_X86_64_REX_GOTPCRELX  _pgtable-0x0000000000000004
> 00000000000005f3 R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
> kaslr.o
> 0000000000000281 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 00000000000003e3 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 0000000000000ba1 R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
> 0000000000000bb4 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 0000000000000cc8 R_X86_64_REX_GOTPCRELX  free_mem_ptr-0x0000000000000004
> 0000000000000cd2 R_X86_64_REX_GOTPCRELX  free_mem_end_ptr-0x0000000000000004
> 0000000000000d6e R_X86_64_REX_GOTPCRELX  free_mem_ptr-0x0000000000000004
> 0000000000000f63 R_X86_64_REX_GOTPCRELX  __pgtable_l5_enabled-0x0000000000000004
> kernel_info.o
> mem_encrypt.o
> misc.o
> 0000000000000015 R_X86_64_REX_GOTPCRELX  free_mem_ptr-0x0000000000000004
> 000000000000001f R_X86_64_REX_GOTPCRELX  free_mem_end_ptr-0x0000000000000004
> 0000000000002965 R_X86_64_REX_GOTPCRELX  early_serial_base-0x0000000000000004
> 0000000000002a43 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 0000000000002b8e R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 0000000000002bec R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 0000000000002c10 R_X86_64_REX_GOTPCRELX  free_mem_end_ptr-0x0000000000000004
> 0000000000002c17 R_X86_64_REX_GOTPCRELX  free_mem_ptr-0x0000000000000004
> 0000000000002d16 R_X86_64_REX_GOTPCRELX  trampoline_32bit-0x0000000000000004
> 0000000000002f97 R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 0000000000002fea R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> pgtable_64.o
> 000000000000000d R_X86_64_REX_GOTPCRELX  boot_params-0x0000000000000004
> 000000000000010a R_X86_64_REX_GOTPCRELX  trampoline_32bit_src-0x0000000000000004
> 0000000000000191 R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
> 0000000000000376 R_X86_64_REX_GOTPCRELX  __force_order-0x0000000000000004
> piggy.o
> string.o

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

[Arvind Sankar]

On Tue, Jan 07, 2020 at 07:10:34PM +0100, Ard Biesheuvel wrote:
> On Tue, 7 Jan 2020 at 19:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Tue, Jan 07, 2020 at 06:59:57PM +0100, Ard Biesheuvel wrote:
> > > On Tue, 7 Jan 2020 at 18:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > On Tue, Jan 07, 2020 at 03:28:31PM +0100, Ard Biesheuvel wrote:
> > > > >
> > > > > 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.
> > > >
> > > > Tried hacking that in and it works, tested with gcc 4.6.4.
> > >
> > > Excellent. But in my testing locally, I don't get any GOT entries in
> > > the first place, strangely enough. So what changes in the output for
> > > you with visibility hidden compared to before?
> >
> > Works with 32-bit as well.
> >
> > Are you checking libstub or boot/compressed? Below is with gcc 4.6 (but
> > latest binutils). With gcc 9, there's only one left -- trampoline_32bit_src
> > in pgtable_64.
> >
> 
> I am looking at the size of the .got section in
> boot/compressed/vmlinux, and it is 0x0 on 64-bit, and 0xc (i.e., only
> the .got.plt fixup code) on 32-bit.
> 
> Could you please check whether passing -Bsymbolic to the linker gives
> the same result btw?
> 

With new ld all those GOTPCRELX's get eliminated. If you add --no-relax
you'll get them in the .got. I don't have an old version of binutils so
I can't check, but I think they will be assembled as GOTPCREL and remain
in the .got section after linking.

A linker option can't help I'd think, because once these relocations are
there in the object files, you need the new binutils to get rid of them.
I didn't get any difference with -Bsymbolic -- also that seems to be for
shared libraries, with executables the references should already be
bound within the executable, shared libraries can't override them.

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

[Ard Biesheuvel]

On Tue, 7 Jan 2020 at 19:32, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Tue, Jan 07, 2020 at 07:10:34PM +0100, Ard Biesheuvel wrote:
> > On Tue, 7 Jan 2020 at 19:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Tue, Jan 07, 2020 at 06:59:57PM +0100, Ard Biesheuvel wrote:
> > > > On Tue, 7 Jan 2020 at 18:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > >
> > > > > On Tue, Jan 07, 2020 at 03:28:31PM +0100, Ard Biesheuvel wrote:
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Tried hacking that in and it works, tested with gcc 4.6.4.
> > > >
> > > > Excellent. But in my testing locally, I don't get any GOT entries in
> > > > the first place, strangely enough. So what changes in the output for
> > > > you with visibility hidden compared to before?
> > >
> > > Works with 32-bit as well.
> > >
> > > Are you checking libstub or boot/compressed? Below is with gcc 4.6 (but
> > > latest binutils). With gcc 9, there's only one left -- trampoline_32bit_src
> > > in pgtable_64.
> > >
> >
> > I am looking at the size of the .got section in
> > boot/compressed/vmlinux, and it is 0x0 on 64-bit, and 0xc (i.e., only
> > the .got.plt fixup code) on 32-bit.
> >
> > Could you please check whether passing -Bsymbolic to the linker gives
> > the same result btw?
> >
>
> With new ld all those GOTPCRELX's get eliminated. If you add --no-relax
> you'll get them in the .got. I don't have an old version of binutils so
> I can't check, but I think they will be assembled as GOTPCREL and remain
> in the .got section after linking.
>

Right, unless you use hidden visibility, no?

> A linker option can't help I'd think, because once these relocations are
> there in the object files, you need the new binutils to get rid of them.
> I didn't get any difference with -Bsymbolic -- also that seems to be for
> shared libraries, with executables the references should already be
> bound within the executable, shared libraries can't override them.

PIE and .so support share a *lot* of code in binutils. In the arm64
kernel, we use Bsymbolic to get rid of a few absolute symbol
references, even though the -pie linker option should take care of
that, but it doesn't in all cases.

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

[Arvind Sankar]

On Tue, Jan 07, 2020 at 08:03:18PM +0100, Ard Biesheuvel wrote:
> On Tue, 7 Jan 2020 at 19:32, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Tue, Jan 07, 2020 at 07:10:34PM +0100, Ard Biesheuvel wrote:
> > > On Tue, 7 Jan 2020 at 19:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > On Tue, Jan 07, 2020 at 06:59:57PM +0100, Ard Biesheuvel wrote:
> > > > > On Tue, 7 Jan 2020 at 18:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > >
> > > > > > On Tue, Jan 07, 2020 at 03:28:31PM +0100, Ard Biesheuvel wrote:
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > Tried hacking that in and it works, tested with gcc 4.6.4.
> > > > >
> > > > > Excellent. But in my testing locally, I don't get any GOT entries in
> > > > > the first place, strangely enough. So what changes in the output for
> > > > > you with visibility hidden compared to before?
> > > >
> > > > Works with 32-bit as well.
> > > >
> > > > Are you checking libstub or boot/compressed? Below is with gcc 4.6 (but
> > > > latest binutils). With gcc 9, there's only one left -- trampoline_32bit_src
> > > > in pgtable_64.
> > > >
> > >
> > > I am looking at the size of the .got section in
> > > boot/compressed/vmlinux, and it is 0x0 on 64-bit, and 0xc (i.e., only
> > > the .got.plt fixup code) on 32-bit.
> > >
> > > Could you please check whether passing -Bsymbolic to the linker gives
> > > the same result btw?
> > >
> >
> > With new ld all those GOTPCRELX's get eliminated. If you add --no-relax
> > you'll get them in the .got. I don't have an old version of binutils so
> > I can't check, but I think they will be assembled as GOTPCREL and remain
> > in the .got section after linking.
> >
> 
> Right, unless you use hidden visibility, no?
> 

Right, that's what I said works before -- with hidden visibility the
compiler (even an old one) does not generate any GOT-using relocations.
We're trying to debug why you don't see any .got entries even before
turning on hidden visibility, while I do, no?

> > A linker option can't help I'd think, because once these relocations are
> > there in the object files, you need the new binutils to get rid of them.
> > I didn't get any difference with -Bsymbolic -- also that seems to be for
> > shared libraries, with executables the references should already be
> > bound within the executable, shared libraries can't override them.
> 
> PIE and .so support share a *lot* of code in binutils. In the arm64
> kernel, we use Bsymbolic to get rid of a few absolute symbol
> references, even though the -pie linker option should take care of
> that, but it doesn't in all cases.

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

[Ard Biesheuvel]

On Tue, 7 Jan 2020 at 20:14, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Tue, Jan 07, 2020 at 08:03:18PM +0100, Ard Biesheuvel wrote:
> > On Tue, 7 Jan 2020 at 19:32, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Tue, Jan 07, 2020 at 07:10:34PM +0100, Ard Biesheuvel wrote:
> > > > On Tue, 7 Jan 2020 at 19:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > >
> > > > > On Tue, Jan 07, 2020 at 06:59:57PM +0100, Ard Biesheuvel wrote:
> > > > > > On Tue, 7 Jan 2020 at 18:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > > >
> > > > > > > On Tue, Jan 07, 2020 at 03:28:31PM +0100, Ard Biesheuvel wrote:
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > Tried hacking that in and it works, tested with gcc 4.6.4.
> > > > > >
> > > > > > Excellent. But in my testing locally, I don't get any GOT entries in
> > > > > > the first place, strangely enough. So what changes in the output for
> > > > > > you with visibility hidden compared to before?
> > > > >
> > > > > Works with 32-bit as well.
> > > > >
> > > > > Are you checking libstub or boot/compressed? Below is with gcc 4.6 (but
> > > > > latest binutils). With gcc 9, there's only one left -- trampoline_32bit_src
> > > > > in pgtable_64.
> > > > >
> > > >
> > > > I am looking at the size of the .got section in
> > > > boot/compressed/vmlinux, and it is 0x0 on 64-bit, and 0xc (i.e., only
> > > > the .got.plt fixup code) on 32-bit.
> > > >
> > > > Could you please check whether passing -Bsymbolic to the linker gives
> > > > the same result btw?
> > > >
> > >
> > > With new ld all those GOTPCRELX's get eliminated. If you add --no-relax
> > > you'll get them in the .got. I don't have an old version of binutils so
> > > I can't check, but I think they will be assembled as GOTPCREL and remain
> > > in the .got section after linking.
> > >
> >
> > Right, unless you use hidden visibility, no?
> >
>
> Right, that's what I said works before -- with hidden visibility the
> compiler (even an old one) does not generate any GOT-using relocations.
> We're trying to debug why you don't see any .got entries even before
> turning on hidden visibility, while I do, no?
>

Yeah. I have just reinstalled Ubuntu Trusty in a VM, which has
binutils 2.4 and has a GCC 4.6 package available, but I haven't tried
building the 64-bit kernel yet.


> > > A linker option can't help I'd think, because once these relocations are
> > > there in the object files, you need the new binutils to get rid of them.
> > > I didn't get any difference with -Bsymbolic -- also that seems to be for
> > > shared libraries, with executables the references should already be
> > > bound within the executable, shared libraries can't override them.
> >
> > PIE and .so support share a *lot* of code in binutils. In the arm64
> > kernel, we use Bsymbolic to get rid of a few absolute symbol
> > references, even though the -pie linker option should take care of
> > that, but it doesn't in all cases.

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

[Ard Biesheuvel]

On Tue, 7 Jan 2020 at 20:23, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Tue, 7 Jan 2020 at 20:14, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Tue, Jan 07, 2020 at 08:03:18PM +0100, Ard Biesheuvel wrote:
> > > On Tue, 7 Jan 2020 at 19:32, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > On Tue, Jan 07, 2020 at 07:10:34PM +0100, Ard Biesheuvel wrote:
> > > > > On Tue, 7 Jan 2020 at 19:08, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > >
> > > > > > On Tue, Jan 07, 2020 at 06:59:57PM +0100, Ard Biesheuvel wrote:
> > > > > > > On Tue, 7 Jan 2020 at 18:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jan 07, 2020 at 03:28:31PM +0100, Ard Biesheuvel wrote:
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > > Tried hacking that in and it works, tested with gcc 4.6.4.
> > > > > > >
> > > > > > > Excellent. But in my testing locally, I don't get any GOT entries in
> > > > > > > the first place, strangely enough. So what changes in the output for
> > > > > > > you with visibility hidden compared to before?
> > > > > >
> > > > > > Works with 32-bit as well.
> > > > > >
> > > > > > Are you checking libstub or boot/compressed? Below is with gcc 4.6 (but
> > > > > > latest binutils). With gcc 9, there's only one left -- trampoline_32bit_src
> > > > > > in pgtable_64.
> > > > > >
> > > > >
> > > > > I am looking at the size of the .got section in
> > > > > boot/compressed/vmlinux, and it is 0x0 on 64-bit, and 0xc (i.e., only
> > > > > the .got.plt fixup code) on 32-bit.
> > > > >
> > > > > Could you please check whether passing -Bsymbolic to the linker gives
> > > > > the same result btw?
> > > > >
> > > >
> > > > With new ld all those GOTPCRELX's get eliminated. If you add --no-relax
> > > > you'll get them in the .got. I don't have an old version of binutils so
> > > > I can't check, but I think they will be assembled as GOTPCREL and remain
> > > > in the .got section after linking.
> > > >
> > >
> > > Right, unless you use hidden visibility, no?
> > >
> >
> > Right, that's what I said works before -- with hidden visibility the
> > compiler (even an old one) does not generate any GOT-using relocations.
> > We're trying to debug why you don't see any .got entries even before
> > turning on hidden visibility, while I do, no?
> >
>
> Yeah. I have just reinstalled Ubuntu Trusty in a VM, which has
> binutils 2.4 and has a GCC 4.6 package available, but I haven't tried
> building the 64-bit kernel yet.
>

I see the same thing: all GOT entries are gone when using -include to
set the pragma, with the exception of the first 0x18 bytes of the
.got.plt section, which we can ignore.

So in summary, we should be able ASSERT() in the linker script that
(_egot - _got) <= 3 * word size, and get rid of all the GOT fixup code
entirely. Or perhaps it is better to add a section marker, and assert
that the *(.got) part is really empty.

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

[Arvind Sankar]

On Tue, Jan 07, 2020 at 08:51:14PM +0100, Ard Biesheuvel wrote:
> >
> > Yeah. I have just reinstalled Ubuntu Trusty in a VM, which has
> > binutils 2.4 and has a GCC 4.6 package available, but I haven't tried
> > building the 64-bit kernel yet.
> >
> 
> I see the same thing: all GOT entries are gone when using -include to
> set the pragma, with the exception of the first 0x18 bytes of the
> .got.plt section, which we can ignore.
> 
> So in summary, we should be able ASSERT() in the linker script that
> (_egot - _got) <= 3 * word size, and get rid of all the GOT fixup code
> entirely. Or perhaps it is better to add a section marker, and assert
> that the *(.got) part is really empty.

Great!