Skip to contentSkip to conversation

x86/boot: get rid of GOT entries and associated fixup code

PATCH In review Ard Biesheuvel 4 participants 16 messages May 23, 2020 – May 28, 2020

Subsystems (2)
x86 EFI
lore
Share & tools
lore

Ard Biesheuvel

author maintainer
#0 lore raw

v2 cover x86/boot: get rid of GOT entries and associated fixup code

Building position independent code using GCC by default results in references to symbols with external linkage to be resolved via GOT entries, which carry the absolute addresses of the symbols, and thus need to be corrected if the load time address of the executable != the link time address.

For fully linked binaries, such GOT indirected references are completely useless, and actually make the startup code more complicated than necessary, since these corrections may need to be applied more than once. In fact, we have been very careful to avoid such references in the EFI stub code, since it would require yet another [earlier] pass of GOT fixups which we currently don't implement.

Older GCCs were quirky when it came to overriding this behavior using symbol visibility, but now that we have increased the minimum GCC version to 4.6, we can actually start setting the symbol visibility to 'hidden' globally for all symbol references in the decompressor, getting rid of the GOT entirely. This means we can get rid of the GOT fixup code right away, and we can start using ordinary external symbol references in the EFI stub without running the risk of boot regressions. (v2 note: we have already started doing this)

CC'ing Linus and Maarten, who were involved in diagnosing an issue related to GOT entries emitted from the EFI stub ~5 years ago. [0] [1]

Many thanks to Arvind for the suggestions and the help in testing these changes. Tested on GCC 4.6 + binutils 2.24 (Ubuntu 14.04), and GCC 8 + binutils 2.31 (Debian Buster)

Changes since v1 [2]:
Rebase only - recent EFI changes have moved all the C code into drivers/firmware/efi/libstub/, which is also built with hidden visibility, and contains an additional objdump pass to detect (and reject) absolute symbol references.

Unless anyone objects, I'd like to incorporate these changes into my late EFI PR for v5.8, which will go out in a day or two.

Cc Maarten Lankhorst <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Arvind Sankar <[email protected]>

[0] https://lore.kernel.org/lkml/[email protected]/
[1] https://lore.kernel.org/lkml/CA+55aFxW9PmtjOf9nUQwpU8swsFqJOz8whZXcONo+XFmkSwezg@mail.gmail.com/
[2] https://lore.kernel.org/linux-efi/[email protected]/

Ard Biesheuvel (3):
x86/boot/compressed: move .got.plt entries out of the .got section x86/boot/compressed: force hidden visibility for all symbol references x86/boot/compressed: get rid of GOT fixup code

 arch/x86/boot/compressed/Makefile      |  1 +
 arch/x86/boot/compressed/head_32.S     | 22 ++------
 arch/x86/boot/compressed/head_64.S     | 57 --------------------
 arch/x86/boot/compressed/hidden.h      | 19 +++++++
 arch/x86/boot/compressed/vmlinux.lds.S | 16 ++++--
 5 files changed, 36 insertions(+), 79 deletions(-)
 create mode 100644 arch/x86/boot/compressed/hidden.h
Signature · 1 line
2.20.1
author maintainer
#1 lore raw

PATCH v2 1/3 x86/boot/compressed: move .got.plt entries out of the .got section

The .got.plt section contains the part of the GOT which is used by PLT entries, and which gets updated lazily by the dynamic loader when function calls are dispatched through those PLT entries.

On fully linked binaries such as the kernel proper or the decompressor, this never happens, and so in practice, the .got.plt section consists only of the first 3 magic entries that are meant to point at the _DYNAMIC section and at the fixup routine in the loader. However, since we don't use a dynamic loader, those entries are never populated or used.

This means that treating those entries like ordinary GOT entries, and updating their values based on the actual placement of the executable in memory is completely pointless, and we can just ignore the .got.plt section entirely, provided that it has no additional entries beyond the first 3 ones.

So add an assertion in the linker script to ensure that this assumption holds, and move the contents out of the [_got, _egot) memory range that is modified by the GOT fixup routines.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/vmlinux.lds.S | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
arch/x86/boot/compressed/vmlinux.lds.S +12 -1
index 0dc5c2b9614b..ce3fdfb93b57 100644
@@ -44,10 +44,13 @@ SECTIONS
 	}
 	.got : {
 		_got = .;
-		KEEP(*(.got.plt))
 		KEEP(*(.got))
 		_egot = .;
 	}
+	.got.plt : {
+		KEEP(*(.got.plt))
+	}
+
 	.data :	{
 		_data = . ;
 		*(.data)

@@ -75,3 +78,11 @@ SECTIONS
 	. = ALIGN(PAGE_SIZE);	/* keep ZO size page aligned */
 	_end = .;
 }
+
+#ifdef CONFIG_X86_64
+ASSERT (SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18,
+	"Unexpected GOT/PLT entries detected!")
+#else
+ASSERT (SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc,
+	"Unexpected GOT/PLT entries detected!")
+#endif
Signature · 1 line
2.20.1
#2 lore raw

Re: [PATCH v2 1/3] x86/boot/compressed: move .got.plt entries out of the .got section

Quoted 46 lines · Ard Biesheuvel

The .got.plt section contains the part of the GOT which is used by PLT entries, and which gets updated lazily by the dynamic loader when function calls are dispatched through those PLT entries.

On fully linked binaries such as the kernel proper or the decompressor, this never happens, and so in practice, the .got.plt section consists only of the first 3 magic entries that are meant to point at the _DYNAMIC section and at the fixup routine in the loader. However, since we don't use a dynamic loader, those entries are never populated or used.

This means that treating those entries like ordinary GOT entries, and updating their values based on the actual placement of the executable in memory is completely pointless, and we can just ignore the .got.plt section entirely, provided that it has no additional entries beyond the first 3 ones.

So add an assertion in the linker script to ensure that this assumption holds, and move the contents out of the [_got, _egot) memory range that is modified by the GOT fixup routines.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/vmlinux.lds.S | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
arch/x86/boot/compressed/vmlinux.lds.S +4 -1
index 0dc5c2b9614b..ce3fdfb93b57 100644
@@ -44,10 +44,13 @@ SECTIONS
 	}
 	.got : {
 		_got = .;
-		KEEP(*(.got.plt))
 		KEEP(*(.got))
 		_egot = .;
 	}
+	.got.plt : {
+		KEEP(*(.got.plt))
+	}
+
 	.data :	{
 		_data = . ;
 		*(.data)

@@ -75,3 +78,11 @@ SECTIONS
 	. = ALIGN(PAGE_SIZE);	/* keep ZO size page aligned */

arch/x86/boot/compressed/vmlinux.lds.S SECTIONS · line 88

Quoted 10 lines
arch/x86/boot/compressed/vmlinux.lds.S +8 -0
 	_end = .;
 }
+
+#ifdef CONFIG_X86_64
+ASSERT (SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18,
+	"Unexpected GOT/PLT entries detected!")
+#else
+ASSERT (SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc,
+	"Unexpected GOT/PLT entries detected!")
+#endif

Cool debugging check!

Minor consistent-style nit:

s/ASSERT (
/ASSERT(

Optional: maybe even merge these on a single line, as a special exception to the col80 rule?

#ifdef CONFIG_X86_64
ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, "Unexpected GOT/PLT entries detected!") #else
ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc, "Unexpected GOT/PLT entries detected!") #endif

But fine either way.

Thanks,

Ingo

author maintainer
#3 lore raw

Re: [PATCH v2 1/3] x86/boot/compressed: move .got.plt entries out of the .got section

Quoted 63 lines · Ingo Molnar
Quoted 46 lines · Ard Biesheuvel

The .got.plt section contains the part of the GOT which is used by PLT entries, and which gets updated lazily by the dynamic loader when function calls are dispatched through those PLT entries.

On fully linked binaries such as the kernel proper or the decompressor, this never happens, and so in practice, the .got.plt section consists only of the first 3 magic entries that are meant to point at the _DYNAMIC section and at the fixup routine in the loader. However, since we don't use a dynamic loader, those entries are never populated or used.

This means that treating those entries like ordinary GOT entries, and updating their values based on the actual placement of the executable in memory is completely pointless, and we can just ignore the .got.plt section entirely, provided that it has no additional entries beyond the first 3 ones.

So add an assertion in the linker script to ensure that this assumption holds, and move the contents out of the [_got, _egot) memory range that is modified by the GOT fixup routines.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/vmlinux.lds.S | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
arch/x86/boot/compressed/vmlinux.lds.S +4 -1
index 0dc5c2b9614b..ce3fdfb93b57 100644
@@ -44,10 +44,13 @@ SECTIONS
      }
      .got : {
              _got = .;
-             KEEP(*(.got.plt))
              KEEP(*(.got))
              _egot = .;
      }
+     .got.plt : {
+             KEEP(*(.got.plt))
+     }
+
      .data : {
              _data = . ;
              *(.data)

@@ -75,3 +78,11 @@ SECTIONS
      . = ALIGN(PAGE_SIZE);   /* keep ZO size page aligned */

arch/x86/boot/compressed/vmlinux.lds.S SECTIONS · line 88

Quoted 10 lines
arch/x86/boot/compressed/vmlinux.lds.S +8 -0
      _end = .;
 }
+
+#ifdef CONFIG_X86_64
+ASSERT (SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18,
+     "Unexpected GOT/PLT entries detected!")
+#else
+ASSERT (SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc,
+     "Unexpected GOT/PLT entries detected!")
+#endif

Cool debugging check!

Quoted 5 lines

Minor consistent-style nit:

s/ASSERT (
/ASSERT(

ok

Quoted 10 lines

Optional: maybe even merge these on a single line, as a special exception to the col80 rule?

#ifdef CONFIG_X86_64
ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, "Unexpected GOT/PLT entries detected!") #else
ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc, "Unexpected GOT/PLT entries detected!") #endif

But fine either way.

SIZEOF(.got.plt) <= 0x18

could be used as well, given that the section is either empty, or has at least the 3 magic entries.

#4 lore raw

Re: [PATCH v2 1/3] x86/boot/compressed: move .got.plt entries out of the .got section

Quoted 80 lines · Ard Biesheuvel
Quoted 63 lines · Ingo Molnar
Quoted 46 lines · Ard Biesheuvel

The .got.plt section contains the part of the GOT which is used by PLT entries, and which gets updated lazily by the dynamic loader when function calls are dispatched through those PLT entries.

On fully linked binaries such as the kernel proper or the decompressor, this never happens, and so in practice, the .got.plt section consists only of the first 3 magic entries that are meant to point at the _DYNAMIC section and at the fixup routine in the loader. However, since we don't use a dynamic loader, those entries are never populated or used.

This means that treating those entries like ordinary GOT entries, and updating their values based on the actual placement of the executable in memory is completely pointless, and we can just ignore the .got.plt section entirely, provided that it has no additional entries beyond the first 3 ones.

So add an assertion in the linker script to ensure that this assumption holds, and move the contents out of the [_got, _egot) memory range that is modified by the GOT fixup routines.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/vmlinux.lds.S | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
arch/x86/boot/compressed/vmlinux.lds.S +4 -1
index 0dc5c2b9614b..ce3fdfb93b57 100644
@@ -44,10 +44,13 @@ SECTIONS
      }
      .got : {
              _got = .;
-             KEEP(*(.got.plt))
              KEEP(*(.got))
              _egot = .;
      }
+     .got.plt : {
+             KEEP(*(.got.plt))
+     }
+
      .data : {
              _data = . ;
              *(.data)

@@ -75,3 +78,11 @@ SECTIONS
      . = ALIGN(PAGE_SIZE);   /* keep ZO size page aligned */

arch/x86/boot/compressed/vmlinux.lds.S SECTIONS · line 88

Quoted 10 lines
arch/x86/boot/compressed/vmlinux.lds.S +8 -0
      _end = .;
 }
+
+#ifdef CONFIG_X86_64
+ASSERT (SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18,
+     "Unexpected GOT/PLT entries detected!")
+#else
+ASSERT (SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc,
+     "Unexpected GOT/PLT entries detected!")
+#endif

Cool debugging check!

Quoted 5 lines

Minor consistent-style nit:

s/ASSERT (
/ASSERT(

ok

Quoted 8 lines

Optional: maybe even merge these on a single line, as a special exception to the col80 rule?

#ifdef CONFIG_X86_64
ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, "Unexpected GOT/PLT entries detected!") #else
ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc, "Unexpected GOT/PLT entries detected!") #endif

Quoted 7 lines
Quoted 2 lines

But fine either way.

SIZEOF(.got.plt) <= 0x18

could be used as well, given that the section is either empty, or has at least the 3 magic entries.

This this has bitten us before, so I think the precise assert is better.

Two-line version is OK too.

Thanks,

Ingo

author maintainer
#5 lore raw

PATCH v2 2/3 x86/boot/compressed: force hidden visibility for all symbol references

In reply to Ard Biesheuvel

Eliminate all GOT entries in the decompressor binary, by forcing hidden visibility for all symbol references, which informs the compiler that such references will be resolved at link time without the need for allocating GOT entries.

To ensure that no GOT entries will creep back in, add an assertion to the decompressor linker script that will fire if the .got section has a non-zero size.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/Makefile      |  1 +
 arch/x86/boot/compressed/hidden.h      | 19 +++++++++++++++++++
 arch/x86/boot/compressed/vmlinux.lds.S |  1 +
 3 files changed, 21 insertions(+)
arch/x86/boot/compressed/Makefile +1 -0
index 5f7c262bcc99..aa9ed814e5fa 100644
@@ -40,6 +40,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
 KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
+KBUILD_CFLAGS += -include hidden.h
 
 KBUILD_AFLAGS  := $(KBUILD_CFLAGS) -D__ASSEMBLY__
 GCOV_PROFILE := n
arch/x86/boot/compressed/hidden.h new file +19 -0
new file mode 100644
index 000000000000..49a17b6b5962
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * When building position independent code with GCC using the -fPIC option,
+ * (or even the -fPIE one on older versions), it will assume that we are
+ * building a dynamic object (either a shared library or an executable) that
+ * may have symbol references that can only be resolved at load time. For a
+ * variety of reasons (ELF symbol preemption, the CoW footprint of the section
+ * that is modified by the loader), this results in all references to symbols
+ * with external linkage to go via entries in the Global Offset Table (GOT),
+ * which carries absolute addresses which need to be fixed up when the
+ * executable image is loaded at an offset which is different from its link
+ * time offset.
+ *
+ * Fortunately, there is a way to inform the compiler that such symbol
+ * references will be satisfied at link time rather than at load time, by
+ * giving them 'hidden' visibility.
+ */
+
+#pragma GCC visibility push(hidden)
arch/x86/boot/compressed/vmlinux.lds.S +1 -0
index ce3fdfb93b57..60a99dfb9d72 100644
@@ -79,6 +79,7 @@ SECTIONS
 	_end = .;
 }
 
+ASSERT (SIZEOF(.got) == 0, "Unexpected GOT entries detected!")
 #ifdef CONFIG_X86_64
 ASSERT (SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18,
 	"Unexpected GOT/PLT entries detected!")
Signature · 1 line
2.20.1
#6 lore raw

Re: [PATCH v2 2/3] x86/boot/compressed: force hidden visibility for all symbol references

Quoted 33 lines · Ard Biesheuvel

Eliminate all GOT entries in the decompressor binary, by forcing hidden visibility for all symbol references, which informs the compiler that such references will be resolved at link time without the need for allocating GOT entries.

To ensure that no GOT entries will creep back in, add an assertion to the decompressor linker script that will fire if the .got section has a non-zero size.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/Makefile      |  1 +
 arch/x86/boot/compressed/hidden.h      | 19 +++++++++++++++++++
 arch/x86/boot/compressed/vmlinux.lds.S |  1 +
 3 files changed, 21 insertions(+)
arch/x86/boot/compressed/Makefile +8 -0
index 5f7c262bcc99..aa9ed814e5fa 100644
@@ -40,6 +40,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
 KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
+KBUILD_CFLAGS += -include hidden.h
 
+ * When building position independent code with GCC using the -fPIC option,
+ * (or even the -fPIE one on older versions), it will assume that we are
+ * building a dynamic object (either a shared library or an executable) that
+ * may have symbol references that can only be resolved at load time. For a
+ * variety of reasons (ELF symbol preemption, the CoW footprint of the section
+ * that is modified by the loader), this results in all references to symbols
+ * with external linkage to go via entries in the Global Offset Table (GOT),

arch/x86/boot/compressed/Makefile KBUILD_CFLAGS += $(call cc-disable-warning, gnu)

Quoted 10 lines
arch/x86/boot/compressed/Makefile +10 -0
+ * which carries absolute addresses which need to be fixed up when the
+ * executable image is loaded at an offset which is different from its link
+ * time offset.
+ *
+ * Fortunately, there is a way to inform the compiler that such symbol
+ * references will be satisfied at link time rather than at load time, by
+ * giving them 'hidden' visibility.
+ */
+
+#pragma GCC visibility push(hidden)

BTW., how many such GOT entries did we have before this change, on a typical kernel?

Quoted 1 line

+ASSERT (SIZEOF(.got) == 0, "Unexpected GOT entries detected!")

s/ASSERT (
/ASSERT(

Thanks,

Ingo

author maintainer
#7 lore raw

Re: [PATCH v2 2/3] x86/boot/compressed: force hidden visibility for all symbol references

Quoted 41 lines · Ingo Molnar
Quoted 37 lines · Ard Biesheuvel

Eliminate all GOT entries in the decompressor binary, by forcing hidden visibility for all symbol references, which informs the compiler that such references will be resolved at link time without the need for allocating GOT entries.

To ensure that no GOT entries will creep back in, add an assertion to the decompressor linker script that will fire if the .got section has a non-zero size.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/Makefile      |  1 +
 arch/x86/boot/compressed/hidden.h      | 19 +++++++++++++++++++
 arch/x86/boot/compressed/vmlinux.lds.S |  1 +
 3 files changed, 21 insertions(+)
arch/x86/boot/compressed/Makefile +12 -0
index 5f7c262bcc99..aa9ed814e5fa 100644
@@ -40,6 +40,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
 KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
+KBUILD_CFLAGS += -include hidden.h
 
+ * When building position independent code with GCC using the -fPIC option,
+ * (or even the -fPIE one on older versions), it will assume that we are
+ * building a dynamic object (either a shared library or an executable) that
+ * may have symbol references that can only be resolved at load time. For a
+ * variety of reasons (ELF symbol preemption, the CoW footprint of the section
+ * that is modified by the loader), this results in all references to symbols
+ * with external linkage to go via entries in the Global Offset Table (GOT),
+ * which carries absolute addresses which need to be fixed up when the
+ * executable image is loaded at an offset which is different from its link
+ * time offset.
+ *
Quoted 9 lines

arch/x86/boot/compressed/Makefile

Quoted 6 lines

+ * Fortunately, there is a way to inform the compiler that such symbol
+ * references will be satisfied at link time rather than at load time, by
+ * giving them 'hidden' visibility.
+ */

arch/x86/boot/compressed/Makefile +2 -0
+
+#pragma GCC visibility push(hidden)

BTW., how many such GOT entries did we have before this change, on a typical kernel?

None if you are using a recent GCC/binutils that use a relocation type that can be relaxed into an ordinary relative reference by the linker. Older GCC/binutils may emit a couple for the decompressor.

Quoted 5 lines
Quoted 1 line

+ASSERT (SIZEOF(.got) == 0, "Unexpected GOT entries detected!")

s/ASSERT (
/ASSERT(

OK

Quoted 3 lines

Thanks,

Ingo

#8 lore raw

Re: [PATCH v2 2/3] x86/boot/compressed: force hidden visibility for all symbol references

In reply to Ard Biesheuvel
Quoted 21 lines · Ard Biesheuvel

Eliminate all GOT entries in the decompressor binary, by forcing hidden visibility for all symbol references, which informs the compiler that such references will be resolved at link time without the need for allocating GOT entries.

To ensure that no GOT entries will creep back in, add an assertion to the decompressor linker script that will fire if the .got section has a non-zero size.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/Makefile      |  1 +
 arch/x86/boot/compressed/hidden.h      | 19 +++++++++++++++++++
 arch/x86/boot/compressed/vmlinux.lds.S |  1 +
 3 files changed, 21 insertions(+)
arch/x86/boot/compressed/Makefile +0 -0
index 5f7c262bcc99..aa9ed814e5fa 100644

arch/x86/boot/compressed/Makefile KBUILD_CFLAGS += $(call cc-disable-warning, gnu) · line 44

Quoted 5 lines
arch/x86/boot/compressed/Makefile +1 -0
@@ -40,6 +40,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
 KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
+KBUILD_CFLAGS += -include hidden.h

Ard, from the other thread [1] in case you missed it -- the plain hidden.h fails to build in-tree. We need something like

	KBUILD_CFLAGS += -include $(srctree)/$(src)/hidden.h

instead.

[1] https://lore.kernel.org/lkml/[email protected]/

#9 lore raw

Re: [PATCH v2 2/3] x86/boot/compressed: force hidden visibility for all symbol references

Quoted 31 lines · Arvind Sankar
Quoted 21 lines · Ard Biesheuvel

Eliminate all GOT entries in the decompressor binary, by forcing hidden visibility for all symbol references, which informs the compiler that such references will be resolved at link time without the need for allocating GOT entries.

To ensure that no GOT entries will creep back in, add an assertion to the decompressor linker script that will fire if the .got section has a non-zero size.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/Makefile      |  1 +
 arch/x86/boot/compressed/hidden.h      | 19 +++++++++++++++++++
 arch/x86/boot/compressed/vmlinux.lds.S |  1 +
 3 files changed, 21 insertions(+)
arch/x86/boot/compressed/Makefile +0 -0
index 5f7c262bcc99..aa9ed814e5fa 100644

arch/x86/boot/compressed/Makefile KBUILD_CFLAGS += $(call cc-disable-warning, gnu) · line 44

Quoted 5 lines
arch/x86/boot/compressed/Makefile +1 -0
@@ -40,6 +40,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
 KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
+KBUILD_CFLAGS += -include hidden.h

Ard, from the other thread [1] in case you missed it -- the plain hidden.h fails to build in-tree. We need something like

Quoted 4 lines
        KBUILD_CFLAGS += -include $(srctree)/$(src)/hidden.h

instead.

[1] https://lore.kernel.org/lkml/[email protected]/

How about using -fvisibility=hidden instead of including this header?

--
Brian Gerst

author maintainer
#10 lore raw

Re: [PATCH v2 2/3] x86/boot/compressed: force hidden visibility for all symbol references

Quoted 36 lines · Brian Gerst
Quoted 34 lines · Arvind Sankar
Quoted 21 lines · Ard Biesheuvel

Eliminate all GOT entries in the decompressor binary, by forcing hidden visibility for all symbol references, which informs the compiler that such references will be resolved at link time without the need for allocating GOT entries.

To ensure that no GOT entries will creep back in, add an assertion to the decompressor linker script that will fire if the .got section has a non-zero size.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/Makefile      |  1 +
 arch/x86/boot/compressed/hidden.h      | 19 +++++++++++++++++++
 arch/x86/boot/compressed/vmlinux.lds.S |  1 +
 3 files changed, 21 insertions(+)
arch/x86/boot/compressed/Makefile +0 -0
index 5f7c262bcc99..aa9ed814e5fa 100644

arch/x86/boot/compressed/Makefile KBUILD_CFLAGS += $(call cc-disable-warning, gnu) · line 44

Quoted 5 lines
arch/x86/boot/compressed/Makefile +1 -0
@@ -40,6 +40,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
 KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
+KBUILD_CFLAGS += -include hidden.h

Ard, from the other thread [1] in case you missed it -- the plain hidden.h fails to build in-tree. We need something like

        KBUILD_CFLAGS += -include $(srctree)/$(src)/hidden.h

instead.

Quoted 4 lines
Quoted 1 line

[1] https://lore.kernel.org/lkml/[email protected]/

How about using -fvisibility=hidden instead of including this header?

That only works for definitions, not for extern declarations.

author maintainer
#11 lore raw

Re: [PATCH v2 2/3] x86/boot/compressed: force hidden visibility for all symbol references

In reply to Arvind Sankar
Quoted 31 lines · Arvind Sankar
Quoted 21 lines · Ard Biesheuvel

Eliminate all GOT entries in the decompressor binary, by forcing hidden visibility for all symbol references, which informs the compiler that such references will be resolved at link time without the need for allocating GOT entries.

To ensure that no GOT entries will creep back in, add an assertion to the decompressor linker script that will fire if the .got section has a non-zero size.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/Makefile      |  1 +
 arch/x86/boot/compressed/hidden.h      | 19 +++++++++++++++++++
 arch/x86/boot/compressed/vmlinux.lds.S |  1 +
 3 files changed, 21 insertions(+)
arch/x86/boot/compressed/Makefile +0 -0
index 5f7c262bcc99..aa9ed814e5fa 100644

arch/x86/boot/compressed/Makefile KBUILD_CFLAGS += $(call cc-disable-warning, gnu) · line 44

Quoted 5 lines
arch/x86/boot/compressed/Makefile +1 -0
@@ -40,6 +40,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
 KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
+KBUILD_CFLAGS += -include hidden.h

Ard, from the other thread [1] in case you missed it -- the plain hidden.h fails to build in-tree. We need something like

Quoted 4 lines
        KBUILD_CFLAGS += -include $(srctree)/$(src)/hidden.h

instead.

[1] https://lore.kernel.org/lkml/[email protected]/

Thanks for the reminder, but I was aware of this.

I'll send out a v2 but given how close we are to the v5.7 release, this is going to be v5.8 material anyway, so let's revisit this later.

author maintainer
#12 lore raw

PATCH v2 3/3 x86/boot/compressed: get rid of GOT fixup code

In reply to Ard Biesheuvel

In a previous patch, we have eliminated GOT entries from the decompressor binary and added an assertion that the .got section is empty. This means that the GOT fixup routines that exist in both the 32-bit and 64-bit startup routines have become dead code, and can be removed.

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/boot/compressed/head_32.S     | 22 ++------
 arch/x86/boot/compressed/head_64.S     | 57 --------------------
 arch/x86/boot/compressed/vmlinux.lds.S |  2 -
 3 files changed, 3 insertions(+), 78 deletions(-)
arch/x86/boot/compressed/head_32.S +3 -19
index ab3307036ba4..dfa4131c65df 100644
@@ -49,16 +49,13 @@
  * Position Independent Executable (PIE) so that linker won't optimize
  * R_386_GOT32X relocation to its fixed symbol address.  Older
  * linkers generate R_386_32 relocations against locally defined symbols,
- * _bss, _ebss, _got and _egot, in PIE.  It isn't wrong, just less
- * optimal than R_386_RELATIVE.  But the x86 kernel fails to properly handle
+ * _bss, _ebss, in PIE.  It isn't wrong, just suboptimal compared
+ * to R_386_RELATIVE.  But the x86 kernel fails to properly handle
  * R_386_32 relocations when relocating the kernel.  To generate
- * R_386_RELATIVE relocations, we mark _bss, _ebss, _got and _egot as
- * hidden:
+ * R_386_RELATIVE relocations, we mark _bss and _ebss as hidden:
  */
 	.hidden _bss
 	.hidden _ebss
-	.hidden _got
-	.hidden _egot
 
 	__HEAD
 SYM_FUNC_START(startup_32)

@@ -191,19 +188,6 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
 	shrl	$2, %ecx
 	rep	stosl
 
-/*
- * 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:
-
 /*
  * Do the extraction, and jump to the new kernel..
  */
arch/x86/boot/compressed/head_64.S +0 -57

81 lines not on this page · raw message

arch/x86/boot/compressed/vmlinux.lds.S +0 -2
index 60a99dfb9d72..d91fdda51aa8 100644
@@ -43,9 +43,7 @@ SECTIONS
 		_erodata = . ;
 	}
 	.got : {
-		_got = .;
 		KEEP(*(.got))
-		_egot = .;
 	}
 	.got.plt : {
 		KEEP(*(.got.plt))
Signature · 1 line
2.20.1
#13 lore raw

Re: [PATCH v2 3/3] x86/boot/compressed: get rid of GOT fixup code

Quoted 23 lines · Ard Biesheuvel

In a previous patch, we have eliminated GOT entries from the decompressor binary and added an assertion that the .got section is empty. This means that the GOT fixup routines that exist in both the 32-bit and 64-bit startup routines have become dead code, and can be removed.

Signed-off-by Ard Biesheuvel <[email protected]>
arch/x86/boot/compressed/vmlinux.lds.S +0 -2
index 60a99dfb9d72..d91fdda51aa8 100644
@@ -43,9 +43,7 @@ SECTIONS
 		_erodata = . ;
 	}
 	.got : {
-		_got = .;
 		KEEP(*(.got))
-		_egot = .;
 	}
 	.got.plt : {
 		KEEP(*(.got.plt))
Signature · 1 line
2.20.1

I think you can get rid of both the KEEP's here as well?

author maintainer
#14 lore raw

Re: [PATCH v2 3/3] x86/boot/compressed: get rid of GOT fixup code

Quoted 19 lines · Arvind Sankar
Quoted 17 lines · Ard Biesheuvel

In a previous patch, we have eliminated GOT entries from the decompressor binary and added an assertion that the .got section is empty. This means that the GOT fixup routines that exist in both the 32-bit and 64-bit startup routines have become dead code, and can be removed.

Signed-off-by Ard Biesheuvel <[email protected]>
arch/x86/boot/compressed/vmlinux.lds.S +0 -2
index 60a99dfb9d72..d91fdda51aa8 100644
@@ -43,9 +43,7 @@ SECTIONS
              _erodata = . ;
      }
      .got : {
-             _got = .;
              KEEP(*(.got))
-             _egot = .;
Quoted 8 lines
Quoted 6 lines
arch/x86/boot/compressed/vmlinux.lds.S +0 -1
      }
      .got.plt : {
              KEEP(*(.got.plt))
--

2.20.1

I think you can get rid of both the KEEP's here as well?

Yeah, they seem fairly pointless to me to begin with, given that these contents are created by the linker on the fly, rather than passed through from the input objects.

I'll drop them.

Acked-by on the v2 series
#15 lore raw
In reply to Ard Biesheuvel
Quoted 36 lines · Ard Biesheuvel

Building position independent code using GCC by default results in references to symbols with external linkage to be resolved via GOT entries, which carry the absolute addresses of the symbols, and thus need to be corrected if the load time address of the executable != the link time address.

For fully linked binaries, such GOT indirected references are completely useless, and actually make the startup code more complicated than necessary, since these corrections may need to be applied more than once. In fact, we have been very careful to avoid such references in the EFI stub code, since it would require yet another [earlier] pass of GOT fixups which we currently don't implement.

Older GCCs were quirky when it came to overriding this behavior using symbol visibility, but now that we have increased the minimum GCC version to 4.6, we can actually start setting the symbol visibility to 'hidden' globally for all symbol references in the decompressor, getting rid of the GOT entirely. This means we can get rid of the GOT fixup code right away, and we can start using ordinary external symbol references in the EFI stub without running the risk of boot regressions. (v2 note: we have already started doing this)

CC'ing Linus and Maarten, who were involved in diagnosing an issue related to GOT entries emitted from the EFI stub ~5 years ago. [0] [1]

Many thanks to Arvind for the suggestions and the help in testing these changes. Tested on GCC 4.6 + binutils 2.24 (Ubuntu 14.04), and GCC 8 + binutils 2.31 (Debian Buster)

Changes since v1 [2]:
Rebase only - recent EFI changes have moved all the C code into drivers/firmware/efi/libstub/, which is also built with hidden visibility, and contains an additional objdump pass to detect (and reject) absolute symbol references.

Unless anyone objects, I'd like to incorporate these changes into my late EFI PR for v5.8, which will go out in a day or two.

Quoted 8 lines
Cc Maarten Lankhorst <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Arvind Sankar <[email protected]>

[0] https://lore.kernel.org/lkml/[email protected]/
[1] https://lore.kernel.org/lkml/CA+55aFxW9PmtjOf9nUQwpU8swsFqJOz8whZXcONo+XFmkSwezg@mail.gmail.com/
[2] https://lore.kernel.org/linux-efi/[email protected]/

Haha, I was actually doing exactly the same thing as part of getting the compressed kernel linkable with LLD.

Acked-by Arvind Sankar <[email protected]>