[PATCH v3 0/9] Warn on orphan section placement

[PATCH v3 0/9] Warn on orphan section placement

[Kees Cook]

v3:
- merge series back together (I tried to make it separable, but no luck)
- remove unwanted sections in libstub
- remove unwanted .eh_frame sections for both .c and .S
- handle sections seen during allnoconfig builds
- handle synthetic and double-quoted sections reported by Clang
- add reviewed-bys
v2: https://lore.kernel.org/lkml/20200622205815.2988115-1-keescook@chromium.org/
v1: https://lore.kernel.org/lkml/20200228002244.15240-1-keescook@chromium.org/

A recent bug[1] was solved for builds linked with ld.lld, and tracking
it down took way longer than it needed to (a year). Ultimately, it
boiled down to differences between ld.bfd and ld.lld's handling of
orphan sections. Similarly, the recent FGKASLR series brough up orphan
section handling too[2]. In both cases, it would have been nice if the
linker was running with --orphan-handling=warn so that surprise sections
wouldn't silently get mapped into the kernel image at locations up to the
whim of the linker's orphan handling logic. Instead, all desired sections
should be explicitly identified in the linker script (to be either kept or
discarded) with any orphans throwing a warning. The powerpc architecture
actually already does this, so this series extends coverage to x86, arm,
and arm64.

All three architectures depend on the first two commits (to
vmlinux.lds.h), and x86 and arm64 depend on the third patch (to
libstub). As such, I'd like to land this series as a whole. Given that
two thirds of it is in the arm universe, perhaps this can land via the
arm64 tree? If x86 -tip is preferred, that works too. Or I could just
carry this myself in -next. In all cases, I would really appreciate
reviews/acks/etc. :)

Thanks!

-Kees

This series is here:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=linker/orphans/warn/v3

[1] https://github.com/ClangBuiltLinux/linux/issues/282
[2] https://lore.kernel.org/lkml/202002242122.AA4D1B8@keescook/

Kees Cook (9):
  vmlinux.lds.h: Add .gnu.version* to DISCARDS
  vmlinux.lds.h: Add .symtab, .strtab, and .shstrtab to STABS_DEBUG
  efi/libstub: Remove .note.gnu.property
  x86/build: Warn on orphan section placement
  x86/boot: Warn on orphan section placement
  arm/build: Warn on orphan section placement
  arm/boot: Warn on orphan section placement
  arm64/build: Use common DISCARDS in linker script
  arm64/build: Warn on orphan section placement

 arch/arm/Makefile                             |  4 ++++
 arch/arm/boot/compressed/Makefile             |  2 ++
 arch/arm/boot/compressed/vmlinux.lds.S        | 17 ++++++--------
 .../arm/{kernel => include/asm}/vmlinux.lds.h | 22 ++++++++++++++-----
 arch/arm/kernel/vmlinux-xip.lds.S             |  5 ++---
 arch/arm/kernel/vmlinux.lds.S                 |  5 ++---
 arch/arm64/Makefile                           |  9 +++++++-
 arch/arm64/kernel/smccc-call.S                |  2 --
 arch/arm64/kernel/vmlinux.lds.S               | 16 ++++++++++----
 arch/arm64/mm/mmu.c                           |  2 +-
 arch/x86/Makefile                             |  4 ++++
 arch/x86/boot/compressed/Makefile             |  3 ++-
 arch/x86/boot/compressed/vmlinux.lds.S        | 11 ++++++++++
 arch/x86/include/asm/asm.h                    |  6 ++++-
 arch/x86/kernel/vmlinux.lds.S                 |  6 +++++
 drivers/firmware/efi/libstub/Makefile         |  3 +++
 include/asm-generic/vmlinux.lds.h             |  7 +++++-
 17 files changed, 92 insertions(+), 32 deletions(-)
 rename arch/arm/{kernel => include/asm}/vmlinux.lds.h (92%)

-- 
2.25.1

[PATCH v3 1/9] vmlinux.lds.h: Add .gnu.version* to DISCARDS

[Kees Cook]

For vmlinux linking, no architecture uses the .gnu.version* sections,
so remove it via the common DISCARDS macro in preparation for adding
--orphan-handling=warn more widely. This is a work-around for what
appears to be a bug[1] in ld.bfd which warns for this synthetic section
even when none is found in input objects, and even when no section is
emitted for an output object[2].

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=26153
[2] https://lore.kernel.org/lkml/202006221524.CEB86E036B@keescook/

Reviewed-by: Fangrui Song <maskray@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/vmlinux.lds.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index db600ef218d7..1248a206be8d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -934,6 +934,8 @@
 	*(.discard)							\
 	*(.discard.*)							\
 	*(.modinfo)							\
+	/* ld.bfd warns about .gnu.version* even when not emitted */	\
+	*(.gnu.version*)						\
 	}
 
 /**
-- 
2.25.1

[PATCH v3 2/9] vmlinux.lds.h: Add .symtab, .strtab, and .shstrtab to STABS_DEBUG

[Kees Cook]

When linking vmlinux with LLD, the synthetic sections .symtab, .strtab,
and .shstrtab are listed as orphaned. Add them to the STABS_DEBUG section
so there will be no warnings when --orphan-handling=warn is used more
widely. (They are added above comment as it is the more common
order[1].)

ld.lld: warning: <internal>:(.symtab) is being placed in '.symtab'
ld.lld: warning: <internal>:(.shstrtab) is being placed in '.shstrtab'
ld.lld: warning: <internal>:(.strtab) is being placed in '.strtab'

[1] https://lore.kernel.org/lkml/20200622224928.o2a7jkq33guxfci4@google.com/

Reported-by: Fangrui Song <maskray@google.com>
Reviewed-by: Fangrui Song <maskray@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/vmlinux.lds.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 1248a206be8d..8e71757f485b 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -792,7 +792,10 @@
 		.stab.exclstr 0 : { *(.stab.exclstr) }			\
 		.stab.index 0 : { *(.stab.index) }			\
 		.stab.indexstr 0 : { *(.stab.indexstr) }		\
-		.comment 0 : { *(.comment) }
+		.comment 0 : { *(.comment) }				\
+		.symtab 0 : { *(.symtab) }				\
+		.strtab 0 : { *(.strtab) }				\
+		.shstrtab 0 : { *(.shstrtab) }
 
 #ifdef CONFIG_GENERIC_BUG
 #define BUG_TABLE							\
-- 
2.25.1

[PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Kees Cook]

In preparation for adding --orphan-handling=warn to more architectures,
make sure unwanted sections don't end up appearing under the .init
section prefix that libstub adds to itself during objcopy.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/firmware/efi/libstub/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 75daaf20374e..9d2d2e784bca 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -66,6 +66,9 @@ lib-$(CONFIG_X86)		+= x86-stub.o
 CFLAGS_arm32-stub.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
 CFLAGS_arm64-stub.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
 
+# Remove unwanted sections first.
+STUBCOPY_FLAGS-y		+= --remove-section=.note.gnu.property
+
 #
 # For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
 # .bss section, so the .bss section of the EFI stub needs to be included in the
-- 
2.25.1

[PATCH v3 4/9] x86/build: Warn on orphan section placement

[Kees Cook]

We don't want to depend on the linker's orphan section placement
heuristics as these can vary between linkers, and may change between
versions. All sections need to be explicitly named in the linker
script.

Discards the unused rela, plt, and got sections that are not needed
in the final vmlinux, stop emitting kprobe sections without kprobes,
and enable orphan section warnings.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/Makefile             | 4 ++++
 arch/x86/include/asm/asm.h    | 6 +++++-
 arch/x86/kernel/vmlinux.lds.S | 6 ++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 00e378de8bc0..f8a5b2333729 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -51,6 +51,10 @@ ifdef CONFIG_X86_NEED_RELOCS
         LDFLAGS_vmlinux := --emit-relocs --discard-none
 endif
 
+# We never want expected sections to be placed heuristically by the
+# linker. All sections should be explicitly named in the linker script.
+LDFLAGS_vmlinux += --orphan-handling=warn
+
 #
 # Prevent GCC from generating any FP code by mistake.
 #
diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 0f63585edf5f..92feec0f0a12 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -138,11 +138,15 @@
 # define _ASM_EXTABLE_FAULT(from, to)				\
 	_ASM_EXTABLE_HANDLE(from, to, ex_handler_fault)
 
-# define _ASM_NOKPROBE(entry)					\
+# ifdef CONFIG_KPROBES
+#  define _ASM_NOKPROBE(entry)					\
 	.pushsection "_kprobe_blacklist","aw" ;			\
 	_ASM_ALIGN ;						\
 	_ASM_PTR (entry);					\
 	.popsection
+# else
+#  define _ASM_NOKPROBE(entry)
+# endif
 
 #else
 # define _EXPAND_EXTABLE_HANDLE(x) #x
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 3bfc8dd8a43d..bb085ceeaaad 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -412,6 +412,12 @@ SECTIONS
 	DWARF_DEBUG
 
 	DISCARDS
+	/DISCARD/ : {
+		*(.rela.*) *(.rela_*)
+		*(.rel.*) *(.rel_*)
+		*(.got) *(.got.*)
+		*(.igot.*) *(.iplt)
+	}
 }
 
 
-- 
2.25.1

[PATCH v3 5/9] x86/boot: Warn on orphan section placement

[Kees Cook]

We don't want to depend on the linker's orphan section placement
heuristics as these can vary between linkers, and may change between
versions. All sections need to be explicitly named in the linker
script.

Add the common debugging sections. Discard the unused note, rel, plt,
dyn, and hash sections that are not needed in the compressed vmlinux.
Disable .eh_frame generation in the linker and enable orphan section
warnings.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/boot/compressed/Makefile      |  3 ++-
 arch/x86/boot/compressed/vmlinux.lds.S | 11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 7619742f91c9..646720a05f89 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -48,6 +48,7 @@ GCOV_PROFILE := n
 UBSAN_SANITIZE :=n
 
 KBUILD_LDFLAGS := -m elf_$(UTS_MACHINE)
+KBUILD_LDFLAGS += $(call ld-option,--no-ld-generated-unwind-info)
 # Compressed kernel should be built as PIE since it may be loaded at any
 # address by the bootloader.
 ifeq ($(CONFIG_X86_32),y)
@@ -59,7 +60,7 @@ else
 KBUILD_LDFLAGS += $(shell $(LD) --help 2>&1 | grep -q "\-z noreloc-overflow" \
 	&& echo "-z noreloc-overflow -pie --no-dynamic-linker")
 endif
-LDFLAGS_vmlinux := -T
+LDFLAGS_vmlinux := --orphan-handling=warn -T
 
 hostprogs	:= mkpiggy
 HOST_EXTRACFLAGS += -I$(srctree)/tools/include
diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
index 8f1025d1f681..6fe3ecdfd685 100644
--- a/arch/x86/boot/compressed/vmlinux.lds.S
+++ b/arch/x86/boot/compressed/vmlinux.lds.S
@@ -75,5 +75,16 @@ SECTIONS
 	. = ALIGN(PAGE_SIZE);	/* keep ZO size page aligned */
 	_end = .;
 
+	STABS_DEBUG
+	DWARF_DEBUG
+
 	DISCARDS
+	/DISCARD/ : {
+		*(.note.*)
+		*(.rela.*) *(.rela_*)
+		*(.rel.*) *(.rel_*)
+		*(.plt) *(.plt.*)
+		*(.dyn*)
+		*(.hash) *(.gnu.hash)
+	}
 }
-- 
2.25.1

[PATCH v3 6/9] arm/build: Warn on orphan section placement

[Kees Cook]

We don't want to depend on the linker's orphan section placement
heuristics as these can vary between linkers, and may change between
versions. All sections need to be explicitly named in the linker
script.

Specifically, this would have made a recently fixed bug very obvious:

ld: warning: orphan section `.fixup' from `arch/arm/lib/copy_from_user.o' being placed in section `.fixup'

Refactor linker script include file for use in standard and XIP linker
scripts, as well as in the coming boot linker script changes. Add debug
sections explicitly. Create ARM_COMMON_DISCARD macro with unneeded
sections .ARM.attributes, .iplt, .rel.iplt, .igot.plt, and .modinfo.
Create ARM_STUBS_TEXT macro with missed text stub sections .vfp11_veneer,
and .v4_bx. Finally enable orphan section warning.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/Makefile                             |  4 ++++
 .../arm/{kernel => include/asm}/vmlinux.lds.h | 22 ++++++++++++++-----
 arch/arm/kernel/vmlinux-xip.lds.S             |  5 ++---
 arch/arm/kernel/vmlinux.lds.S                 |  5 ++---
 4 files changed, 25 insertions(+), 11 deletions(-)
 rename arch/arm/{kernel => include/asm}/vmlinux.lds.h (92%)

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 59fde2d598d8..e414e3732b3a 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -16,6 +16,10 @@ LDFLAGS_vmlinux	+= --be8
 KBUILD_LDFLAGS_MODULE	+= --be8
 endif
 
+# We never want expected sections to be placed heuristically by the
+# linker. All sections should be explicitly named in the linker script.
+LDFLAGS_vmlinux += --orphan-handling=warn
+
 ifeq ($(CONFIG_ARM_MODULE_PLTS),y)
 KBUILD_LDS_MODULE	+= $(srctree)/arch/arm/kernel/module.lds
 endif
diff --git a/arch/arm/kernel/vmlinux.lds.h b/arch/arm/include/asm/vmlinux.lds.h
similarity index 92%
rename from arch/arm/kernel/vmlinux.lds.h
rename to arch/arm/include/asm/vmlinux.lds.h
index 381a8e105fa5..3d88ea74f4cd 100644
--- a/arch/arm/kernel/vmlinux.lds.h
+++ b/arch/arm/include/asm/vmlinux.lds.h
@@ -1,4 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
+#include <asm-generic/vmlinux.lds.h>
 
 #ifdef CONFIG_HOTPLUG_CPU
 #define ARM_CPU_DISCARD(x)
@@ -37,6 +38,13 @@
 		*(.idmap.text)						\
 		__idmap_text_end = .;					\
 
+#define ARM_COMMON_DISCARD						\
+		*(.ARM.attributes)					\
+		*(.iplt) *(.rel.iplt) *(.igot.plt)			\
+		*(.modinfo)						\
+		*(.discard)						\
+		*(.discard.*)
+
 #define ARM_DISCARD							\
 		*(.ARM.exidx.exit.text)					\
 		*(.ARM.extab.exit.text)					\
@@ -49,8 +57,14 @@
 		EXIT_CALL						\
 		ARM_MMU_DISCARD(*(.text.fixup))				\
 		ARM_MMU_DISCARD(*(__ex_table))				\
-		*(.discard)						\
-		*(.discard.*)
+		ARM_COMMON_DISCARD
+
+#define ARM_STUBS_TEXT							\
+		*(.gnu.warning)						\
+		*(.glue_7t)						\
+		*(.glue_7)						\
+		*(.vfp11_veneer)					\
+		*(.v4_bx)
 
 #define ARM_TEXT							\
 		IDMAP_TEXT						\
@@ -64,9 +78,7 @@
 		CPUIDLE_TEXT						\
 		LOCK_TEXT						\
 		KPROBES_TEXT						\
-		*(.gnu.warning)						\
-		*(.glue_7)						\
-		*(.glue_7t)						\
+		ARM_STUBS_TEXT						\
 		. = ALIGN(4);						\
 		*(.got)			/* Global offset table */	\
 		ARM_CPU_KEEP(PROC_INFO)
diff --git a/arch/arm/kernel/vmlinux-xip.lds.S b/arch/arm/kernel/vmlinux-xip.lds.S
index 6d2be994ae58..0807f40844a2 100644
--- a/arch/arm/kernel/vmlinux-xip.lds.S
+++ b/arch/arm/kernel/vmlinux-xip.lds.S
@@ -9,15 +9,13 @@
 
 #include <linux/sizes.h>
 
-#include <asm-generic/vmlinux.lds.h>
+#include <asm/vmlinux.lds.h>
 #include <asm/cache.h>
 #include <asm/thread_info.h>
 #include <asm/memory.h>
 #include <asm/mpu.h>
 #include <asm/page.h>
 
-#include "vmlinux.lds.h"
-
 OUTPUT_ARCH(arm)
 ENTRY(stext)
 
@@ -152,6 +150,7 @@ SECTIONS
 	_end = .;
 
 	STABS_DEBUG
+	DWARF_DEBUG
 }
 
 /*
diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index 7f24bc08403e..969205f125ca 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -9,15 +9,13 @@
 #else
 
 #include <linux/pgtable.h>
-#include <asm-generic/vmlinux.lds.h>
+#include <asm/vmlinux.lds.h>
 #include <asm/cache.h>
 #include <asm/thread_info.h>
 #include <asm/memory.h>
 #include <asm/mpu.h>
 #include <asm/page.h>
 
-#include "vmlinux.lds.h"
-
 OUTPUT_ARCH(arm)
 ENTRY(stext)
 
@@ -151,6 +149,7 @@ SECTIONS
 	_end = .;
 
 	STABS_DEBUG
+	DWARF_DEBUG
 }
 
 #ifdef CONFIG_STRICT_KERNEL_RWX
-- 
2.25.1

[PATCH v3 7/9] arm/boot: Warn on orphan section placement

[Kees Cook]

We don't want to depend on the linker's orphan section placement
heuristics as these can vary between linkers, and may change between
versions. All sections need to be explicitly named in the linker
script.

Use common macros for debug sections, discards, and text stubs. Add
discards for unwanted .note, and .rel sections. Finally, enable orphan
section warning.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/boot/compressed/Makefile      |  2 ++
 arch/arm/boot/compressed/vmlinux.lds.S | 17 +++++++----------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index 00602a6fba04..b8a97d81662d 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -128,6 +128,8 @@ endif
 LDFLAGS_vmlinux += --no-undefined
 # Delete all temporary local symbols
 LDFLAGS_vmlinux += -X
+# Report orphan sections
+LDFLAGS_vmlinux += --orphan-handling=warn
 # Next argument is a linker script
 LDFLAGS_vmlinux += -T
 
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index 09ac33f52814..c2a8509f876f 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -2,6 +2,7 @@
 /*
  *  Copyright (C) 2000 Russell King
  */
+#include <asm/vmlinux.lds.h>
 
 #ifdef CONFIG_CPU_ENDIAN_BE8
 #define ZIMAGE_MAGIC(x) ( (((x) >> 24) & 0x000000ff) | \
@@ -17,8 +18,11 @@ ENTRY(_start)
 SECTIONS
 {
   /DISCARD/ : {
+    ARM_COMMON_DISCARD
     *(.ARM.exidx*)
     *(.ARM.extab*)
+    *(.note.*)
+    *(.rel.*)
     /*
      * Discard any r/w data - this produces a link error if we have any,
      * which is required for PIC decompression.  Local data generates
@@ -36,9 +40,7 @@ SECTIONS
     *(.start)
     *(.text)
     *(.text.*)
-    *(.gnu.warning)
-    *(.glue_7t)
-    *(.glue_7)
+    ARM_STUBS_TEXT
   }
   .table : ALIGN(4) {
     _table_start = .;
@@ -128,12 +130,7 @@ SECTIONS
   PROVIDE(__pecoff_data_size = ALIGN(512) - ADDR(.data));
   PROVIDE(__pecoff_end = ALIGN(512));
 
-  .stab 0		: { *(.stab) }
-  .stabstr 0		: { *(.stabstr) }
-  .stab.excl 0		: { *(.stab.excl) }
-  .stab.exclstr 0	: { *(.stab.exclstr) }
-  .stab.index 0		: { *(.stab.index) }
-  .stab.indexstr 0	: { *(.stab.indexstr) }
-  .comment 0		: { *(.comment) }
+  STABS_DEBUG
+  DWARF_DEBUG
 }
 ASSERT(_edata_real == _edata, "error: zImage file size is incorrect");
-- 
2.25.1

[PATCH v3 8/9] arm64/build: Use common DISCARDS in linker script

[Kees Cook]

Use the common DISCARDS rule for the linker script in an effort to
regularize the linker script to prepare for warning on orphaned
sections. Additionally clean up left-over no-op macros.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/vmlinux.lds.S | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 6827da7f3aa5..5427f502c3a6 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -6,6 +6,7 @@
  */
 
 #define RO_EXCEPTION_TABLE_ALIGN	8
+#define RUNTIME_DISCARD_EXIT
 
 #include <asm-generic/vmlinux.lds.h>
 #include <asm/cache.h>
@@ -89,10 +90,8 @@ SECTIONS
 	 * matching the same input section name.  There is no documented
 	 * order of matching.
 	 */
+	DISCARDS
 	/DISCARD/ : {
-		EXIT_CALL
-		*(.discard)
-		*(.discard.*)
 		*(.interp .dynamic)
 		*(.dynsym .dynstr .hash .gnu.hash)
 		*(.eh_frame)
-- 
2.25.1

[PATCH v3 9/9] arm64/build: Warn on orphan section placement

[Kees Cook]

We don't want to depend on the linker's orphan section placement
heuristics as these can vary between linkers, and may change between
versions. All sections need to be explicitly named in the linker
script.

Avoid .eh_frame* by making sure both -fno-asychronous-unwind-tables and
-fno-unwind-tables are present in both CFLAGS and AFLAGS. Remove one
last instance of .eh_frame by removing the needless Call Frame Information
annotations from arch/arm64/kernel/smccc-call.S.

Add .plt, .data.rel.ro, .igot.*, and .iplt to discards as they are not
actually used. While .got.plt is also not used, it must be included
otherwise ld.bfd will fail to link with the error:

    aarch64-linux-gnu-ld: discarded output section: `.got.plt'

However, as it'd be better to validate that it stays effectively empty,
add an assert.

Explicitly include debug sections when they're present.

Fix a case of needless quotes in __section(), which Clang doesn't like.

Finally, enable orphan section warnings.

Thanks to Ard Biesheuvel for many hints on correct ways to handle
mysterious sections. :)

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm64/Makefile             |  9 ++++++++-
 arch/arm64/kernel/smccc-call.S  |  2 --
 arch/arm64/kernel/vmlinux.lds.S | 11 ++++++++++-
 arch/arm64/mm/mmu.c             |  2 +-
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index a0d94d063fa8..fb3aa2d7de4d 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -29,6 +29,10 @@ LDFLAGS_vmlinux	+= --fix-cortex-a53-843419
   endif
 endif
 
+# We never want expected sections to be placed heuristically by the
+# linker. All sections should be explicitly named in the linker script.
+LDFLAGS_vmlinux += --orphan-handling=warn
+
 ifeq ($(CONFIG_ARM64_USE_LSE_ATOMICS), y)
   ifneq ($(CONFIG_ARM64_LSE_ATOMICS), y)
 $(warning LSE atomics not supported by binutils)
@@ -47,13 +51,16 @@ endif
 
 KBUILD_CFLAGS	+= -mgeneral-regs-only	\
 		   $(compat_vdso) $(cc_has_k_constraint)
-KBUILD_CFLAGS	+= -fno-asynchronous-unwind-tables
 KBUILD_CFLAGS	+= $(call cc-disable-warning, psabi)
 KBUILD_AFLAGS	+= $(compat_vdso)
 
 KBUILD_CFLAGS	+= $(call cc-option,-mabi=lp64)
 KBUILD_AFLAGS	+= $(call cc-option,-mabi=lp64)
 
+# Avoid generating .eh_frame* sections.
+KBUILD_CFLAGS	+= -fno-asynchronous-unwind-tables -fno-unwind-tables
+KBUILD_AFLAGS	+= -fno-asynchronous-unwind-tables -fno-unwind-tables
+
 ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
 prepare: stack_protector_prepare
 stack_protector_prepare: prepare0
diff --git a/arch/arm64/kernel/smccc-call.S b/arch/arm64/kernel/smccc-call.S
index 1f93809528a4..d62447964ed9 100644
--- a/arch/arm64/kernel/smccc-call.S
+++ b/arch/arm64/kernel/smccc-call.S
@@ -9,7 +9,6 @@
 #include <asm/assembler.h>
 
 	.macro SMCCC instr
-	.cfi_startproc
 	\instr	#0
 	ldr	x4, [sp]
 	stp	x0, x1, [x4, #ARM_SMCCC_RES_X0_OFFS]
@@ -21,7 +20,6 @@
 	b.ne	1f
 	str	x6, [x4, ARM_SMCCC_QUIRK_STATE_OFFS]
 1:	ret
-	.cfi_endproc
 	.endm
 
 /*
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 5427f502c3a6..f6c781768f83 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -94,7 +94,8 @@ SECTIONS
 	/DISCARD/ : {
 		*(.interp .dynamic)
 		*(.dynsym .dynstr .hash .gnu.hash)
-		*(.eh_frame)
+		*(.plt) *(.data.rel.ro)
+		*(.igot.*) *(.iplt)
 	}
 
 	. = KIMAGE_VADDR + TEXT_OFFSET;
@@ -244,8 +245,16 @@ SECTIONS
 	_end = .;
 
 	STABS_DEBUG
+	DWARF_DEBUG
 
 	HEAD_SYMBOLS
+
+	/*
+	 * Make sure that the .got.plt is either completely empty or it
+	 * contains only the lazy dispatch entries.
+	 */
+	.got.plt (INFO) : { *(.got.plt) }
+	ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, ".got.plt not empty")
 }
 
 #include "image-vars.h"
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 1df25f26571d..dce024ea6084 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -42,7 +42,7 @@
 u64 idmap_t0sz = TCR_T0SZ(VA_BITS);
 u64 idmap_ptrs_per_pgd = PTRS_PER_PGD;
 
-u64 __section(".mmuoff.data.write") vabits_actual;
+u64 __section(.mmuoff.data.write) vabits_actual;
 EXPORT_SYMBOL(vabits_actual);
 
 u64 kimage_voffset __ro_after_init;
-- 
2.25.1

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Fangrui Song]

On 2020-06-23, Kees Cook wrote:
>In preparation for adding --orphan-handling=warn to more architectures,
>make sure unwanted sections don't end up appearing under the .init
>section prefix that libstub adds to itself during objcopy.
>
>Signed-off-by: Kees Cook <keescook@chromium.org>
>---
> drivers/firmware/efi/libstub/Makefile | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
>index 75daaf20374e..9d2d2e784bca 100644
>--- a/drivers/firmware/efi/libstub/Makefile
>+++ b/drivers/firmware/efi/libstub/Makefile
>@@ -66,6 +66,9 @@ lib-$(CONFIG_X86)		+= x86-stub.o
> CFLAGS_arm32-stub.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
> CFLAGS_arm64-stub.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
>
>+# Remove unwanted sections first.
>+STUBCOPY_FLAGS-y		+= --remove-section=.note.gnu.property
>+
> #
> # For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
> # .bss section, so the .bss section of the EFI stub needs to be included in the

arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on

ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
else
branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
endif

This option creates .note.gnu.property:

% readelf -n drivers/firmware/efi/libstub/efi-stub.o

Displaying notes found in: .note.gnu.property
   Owner                Data size        Description
   GNU                  0x00000010       NT_GNU_PROPERTY_TYPE_0
       Properties: AArch64 feature: PAC

If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
-mbranch-protection=none can override -mbranch-protection=pac-ret+leaf

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Kees Cook]

On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> On 2020-06-23, Kees Cook wrote:
> > In preparation for adding --orphan-handling=warn to more architectures,
> > make sure unwanted sections don't end up appearing under the .init
> > section prefix that libstub adds to itself during objcopy.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > drivers/firmware/efi/libstub/Makefile | 3 +++
> > 1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index 75daaf20374e..9d2d2e784bca 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -66,6 +66,9 @@ lib-$(CONFIG_X86)		+= x86-stub.o
> > CFLAGS_arm32-stub.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
> > CFLAGS_arm64-stub.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
> > 
> > +# Remove unwanted sections first.
> > +STUBCOPY_FLAGS-y		+= --remove-section=.note.gnu.property
> > +
> > #
> > # For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
> > # .bss section, so the .bss section of the EFI stub needs to be included in the
> 
> arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
> 
> ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> else
> branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> endif
> 
> This option creates .note.gnu.property:
> 
> % readelf -n drivers/firmware/efi/libstub/efi-stub.o
> 
> Displaying notes found in: .note.gnu.property
>   Owner                Data size        Description
>   GNU                  0x00000010       NT_GNU_PROPERTY_TYPE_0
>       Properties: AArch64 feature: PAC
> 
> If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf

We want to keep the branch protection enabled. But since it's not a
"regular" ELF, we don't need to keep the property that identifies the
feature.

-- 
Kees Cook

Re: [PATCH v3 9/9] arm64/build: Warn on orphan section placement

[Will Deacon]

On Tue, Jun 23, 2020 at 06:49:40PM -0700, Kees Cook wrote:
> We don't want to depend on the linker's orphan section placement
> heuristics as these can vary between linkers, and may change between
> versions. All sections need to be explicitly named in the linker
> script.
> 
> Avoid .eh_frame* by making sure both -fno-asychronous-unwind-tables and
> -fno-unwind-tables are present in both CFLAGS and AFLAGS. Remove one
> last instance of .eh_frame by removing the needless Call Frame Information
> annotations from arch/arm64/kernel/smccc-call.S.
> 
> Add .plt, .data.rel.ro, .igot.*, and .iplt to discards as they are not
> actually used. While .got.plt is also not used, it must be included
> otherwise ld.bfd will fail to link with the error:
> 
>     aarch64-linux-gnu-ld: discarded output section: `.got.plt'
> 
> However, as it'd be better to validate that it stays effectively empty,
> add an assert.
> 
> Explicitly include debug sections when they're present.
> 
> Fix a case of needless quotes in __section(), which Clang doesn't like.
> 
> Finally, enable orphan section warnings.
> 
> Thanks to Ard Biesheuvel for many hints on correct ways to handle
> mysterious sections. :)

Sorry to be a pain, but this patch is doing 3 or 4 independent things at
once. Please could you split it up a bit?
e.g.

 - Removal of cfi directives from smccc macro
 - Removal of quotes around section name for clang
 - Avoid generating .eh_frame
 - Ensure all sections are accounted for in linker script and warn on orphans

That way it's a bit easier to manage, we can revert/backport bits later if
necessary and you get more patches in the kernel ;)

You can also add my Ack on all the patches:

Acked-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Will Deacon]

On Tue, Jun 23, 2020 at 09:44:11PM -0700, Kees Cook wrote:
> On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> > On 2020-06-23, Kees Cook wrote:
> > > In preparation for adding --orphan-handling=warn to more architectures,
> > > make sure unwanted sections don't end up appearing under the .init
> > > section prefix that libstub adds to itself during objcopy.
> > > 
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > > drivers/firmware/efi/libstub/Makefile | 3 +++
> > > 1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > index 75daaf20374e..9d2d2e784bca 100644
> > > --- a/drivers/firmware/efi/libstub/Makefile
> > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > @@ -66,6 +66,9 @@ lib-$(CONFIG_X86)		+= x86-stub.o
> > > CFLAGS_arm32-stub.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > CFLAGS_arm64-stub.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > 
> > > +# Remove unwanted sections first.
> > > +STUBCOPY_FLAGS-y		+= --remove-section=.note.gnu.property
> > > +
> > > #
> > > # For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
> > > # .bss section, so the .bss section of the EFI stub needs to be included in the
> > 
> > arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
> > 
> > ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> > else
> > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> > endif
> > 
> > This option creates .note.gnu.property:
> > 
> > % readelf -n drivers/firmware/efi/libstub/efi-stub.o
> > 
> > Displaying notes found in: .note.gnu.property
> >   Owner                Data size        Description
> >   GNU                  0x00000010       NT_GNU_PROPERTY_TYPE_0
> >       Properties: AArch64 feature: PAC
> > 
> > If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> > -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf
> 
> We want to keep the branch protection enabled. But since it's not a
> "regular" ELF, we don't need to keep the property that identifies the
> feature.

For the kernel Image, how do we remove these sections? The objcopy flags
in arch/arm64/boot/Makefile look both insufficient and out of date. My
vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
segment.

Will

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Ard Biesheuvel]

On Wed, 24 Jun 2020 at 12:44, Will Deacon <will@kernel.org> wrote:
>
> On Tue, Jun 23, 2020 at 09:44:11PM -0700, Kees Cook wrote:
> > On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> > > On 2020-06-23, Kees Cook wrote:
> > > > In preparation for adding --orphan-handling=warn to more architectures,
> > > > make sure unwanted sections don't end up appearing under the .init
> > > > section prefix that libstub adds to itself during objcopy.
> > > >
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > ---
> > > > drivers/firmware/efi/libstub/Makefile | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > > index 75daaf20374e..9d2d2e784bca 100644
> > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > @@ -66,6 +66,9 @@ lib-$(CONFIG_X86)               += x86-stub.o
> > > > CFLAGS_arm32-stub.o               := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > CFLAGS_arm64-stub.o               := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > >
> > > > +# Remove unwanted sections first.
> > > > +STUBCOPY_FLAGS-y         += --remove-section=.note.gnu.property
> > > > +
> > > > #
> > > > # For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
> > > > # .bss section, so the .bss section of the EFI stub needs to be included in the
> > >
> > > arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
> > >
> > > ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> > > else
> > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> > > endif
> > >
> > > This option creates .note.gnu.property:
> > >
> > > % readelf -n drivers/firmware/efi/libstub/efi-stub.o
> > >
> > > Displaying notes found in: .note.gnu.property
> > >   Owner                Data size        Description
> > >   GNU                  0x00000010       NT_GNU_PROPERTY_TYPE_0
> > >       Properties: AArch64 feature: PAC
> > >
> > > If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> > > -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf
> >
> > We want to keep the branch protection enabled. But since it's not a
> > "regular" ELF, we don't need to keep the property that identifies the
> > feature.
>
> For the kernel Image, how do we remove these sections? The objcopy flags
> in arch/arm64/boot/Makefile look both insufficient and out of date. My
> vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> segment.
>

The latter is the fault of the libstub make rules, that prepend .init
to all section names.

I'm not sure if there is a point to having PAC and/or BTI in the EFI
stub, given that it runs under the control of the firmware, with its
memory mappings and PAC configuration etc.

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Will Deacon]

On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> On Wed, 24 Jun 2020 at 12:44, Will Deacon <will@kernel.org> wrote:
> > On Tue, Jun 23, 2020 at 09:44:11PM -0700, Kees Cook wrote:
> > > On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> > > > arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
> > > >
> > > > ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> > > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> > > > else
> > > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> > > > endif
> > > >
> > > > This option creates .note.gnu.property:
> > > >
> > > > % readelf -n drivers/firmware/efi/libstub/efi-stub.o
> > > >
> > > > Displaying notes found in: .note.gnu.property
> > > >   Owner                Data size        Description
> > > >   GNU                  0x00000010       NT_GNU_PROPERTY_TYPE_0
> > > >       Properties: AArch64 feature: PAC
> > > >
> > > > If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> > > > -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf
> > >
> > > We want to keep the branch protection enabled. But since it's not a
> > > "regular" ELF, we don't need to keep the property that identifies the
> > > feature.
> >
> > For the kernel Image, how do we remove these sections? The objcopy flags
> > in arch/arm64/boot/Makefile look both insufficient and out of date. My
> > vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> > segment.
> >
> 
> The latter is the fault of the libstub make rules, that prepend .init
> to all section names.

Hmm. I tried adding -mbranch-protection=none to arm64 cflags for the stub,
but I still see this note in vmlinux. It looks like it comes in via the
stub copy of lib-ctype.o, but I don't know why that would force the
note. The cflags look ok to me [1] and I confirmed that the note is
being generated by the compiler.

> I'm not sure if there is a point to having PAC and/or BTI in the EFI
> stub, given that it runs under the control of the firmware, with its
> memory mappings and PAC configuration etc.

Agreed, I just can't figure out how to get rid of the note.

Will

[1] -mlittle-endian -DKASAN_SHADOW_SCALE_SHIFT=3 -Qunused-arguments -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Wno-format-security -std=gnu89 --target=aarch64-linux-gnu --prefix=/usr/local/google/home/willdeacon/bin/ --gcc-toolchain=/usr/local/google/home/willdeacon -no-integrated-as -Werror=unknown-warning-option -mgeneral-regs-only -DCONFIG_CC_HAS_K_CONSTRAINT=1 -fno-asynchronous-unwind-tables -mbranch-protection=pac-ret+leaf+bti -Wa,-march=armv8.3-a -DKASAN_SHADOW_SCALE_SHIFT=3 -fno-delete-null-pointer-checks -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wno-format-invalid-specifier -Wno-gnu -mno-global-merge -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -g -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wno-array-bounds -fno-strict-overflow -fno-merge-all-constants -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -fmacro-prefix-map=./= -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-format-zero-length -Wno-tautological-constant-out-of-range-compare -fpie -mbranch-protection=none -I./scripts/dtc/libfdt -Os -DDISABLE_BRANCH_PROFILING -include ./drivers/firmware/efi/libstub/hidden.h -D__NO_FORTIFY -ffreestanding -fno-stack-protector -fno-addrsig -D__DISABLE_EXPORTS    -DKBUILD_MODFILE='"drivers/firmware/efi/libstub/lib-ctype"' -DKBUILD_BASENAME='"lib_ctype"' -DKBUILD_MODNAME='"lib_ctype"' -c -o drivers/firmware/efi/libstub/lib-ctype.o lib/ctype.c

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Dave Martin]

On Wed, Jun 24, 2020 at 12:26:47PM +0100, Will Deacon wrote:
> On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > On Wed, 24 Jun 2020 at 12:44, Will Deacon <will@kernel.org> wrote:
> > > On Tue, Jun 23, 2020 at 09:44:11PM -0700, Kees Cook wrote:
> > > > On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> > > > > arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
> > > > >
> > > > > ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> > > > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> > > > > else
> > > > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> > > > > endif
> > > > >
> > > > > This option creates .note.gnu.property:
> > > > >
> > > > > % readelf -n drivers/firmware/efi/libstub/efi-stub.o
> > > > >
> > > > > Displaying notes found in: .note.gnu.property
> > > > >   Owner                Data size        Description
> > > > >   GNU                  0x00000010       NT_GNU_PROPERTY_TYPE_0
> > > > >       Properties: AArch64 feature: PAC
> > > > >
> > > > > If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> > > > > -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf
> > > >
> > > > We want to keep the branch protection enabled. But since it's not a
> > > > "regular" ELF, we don't need to keep the property that identifies the
> > > > feature.
> > >
> > > For the kernel Image, how do we remove these sections? The objcopy flags
> > > in arch/arm64/boot/Makefile look both insufficient and out of date. My
> > > vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> > > segment.
> > >
> > 
> > The latter is the fault of the libstub make rules, that prepend .init
> > to all section names.
> 
> Hmm. I tried adding -mbranch-protection=none to arm64 cflags for the stub,
> but I still see this note in vmlinux. It looks like it comes in via the
> stub copy of lib-ctype.o, but I don't know why that would force the
> note. The cflags look ok to me [1] and I confirmed that the note is
> being generated by the compiler.
> 
> > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > stub, given that it runs under the control of the firmware, with its
> > memory mappings and PAC configuration etc.
> 
> Agreed, I just can't figure out how to get rid of the note.

Because this section is generated by the linker itself I think you might
have to send it to /DISCARD/ in the link, or strip it explicitly after
linking.

Cheers
---Dave

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Kees Cook]

On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> I'm not sure if there is a point to having PAC and/or BTI in the EFI
> stub, given that it runs under the control of the firmware, with its
> memory mappings and PAC configuration etc.

Is BTI being ignored when the firmware runs?

-- 
Kees Cook

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Will Deacon]

On Wed, Jun 24, 2020 at 02:48:55PM +0100, Dave Martin wrote:
> On Wed, Jun 24, 2020 at 12:26:47PM +0100, Will Deacon wrote:
> > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 24 Jun 2020 at 12:44, Will Deacon <will@kernel.org> wrote:
> > > > For the kernel Image, how do we remove these sections? The objcopy flags
> > > > in arch/arm64/boot/Makefile look both insufficient and out of date. My
> > > > vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> > > > segment.
> > > 
> > > The latter is the fault of the libstub make rules, that prepend .init
> > > to all section names.
> > 
> > Hmm. I tried adding -mbranch-protection=none to arm64 cflags for the stub,
> > but I still see this note in vmlinux. It looks like it comes in via the
> > stub copy of lib-ctype.o, but I don't know why that would force the
> > note. The cflags look ok to me [1] and I confirmed that the note is
> > being generated by the compiler.
> > 
> > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > stub, given that it runs under the control of the firmware, with its
> > > memory mappings and PAC configuration etc.
> > 
> > Agreed, I just can't figure out how to get rid of the note.
> 
> Because this section is generated by the linker itself I think you might
> have to send it to /DISCARD/ in the link, or strip it explicitly after
> linking.

Right, but why is the linker generating that section in the first place? I'm
compiling with -mbranch-protection=none and all the other objects linked
into the stub do not have the section.

I wonder if it's because lib/ctype.c doesn't have any executable code...

Will

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Ard Biesheuvel]

On Wed, 24 Jun 2020 at 17:21, Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > stub, given that it runs under the control of the firmware, with its
> > memory mappings and PAC configuration etc.
>
> Is BTI being ignored when the firmware runs?
>

Given that it requires the 'guarded' attribute to be set in the page
tables, and the fact that the UEFI spec does not require it for
executables that it invokes, nor describes any means of annotating
such executables as having been built with BTI annotations, I think we
can safely assume that the EFI stub will execute with BTI disabled in
the foreseeable future.

Re: [PATCH v3 9/9] arm64/build: Warn on orphan section placement

[Kees Cook]

On Wed, Jun 24, 2020 at 08:57:12AM +0100, Will Deacon wrote:
> On Tue, Jun 23, 2020 at 06:49:40PM -0700, Kees Cook wrote:
> > We don't want to depend on the linker's orphan section placement
> > heuristics as these can vary between linkers, and may change between
> > versions. All sections need to be explicitly named in the linker
> > script.
> > 
> > Avoid .eh_frame* by making sure both -fno-asychronous-unwind-tables and
> > -fno-unwind-tables are present in both CFLAGS and AFLAGS. Remove one
> > last instance of .eh_frame by removing the needless Call Frame Information
> > annotations from arch/arm64/kernel/smccc-call.S.
> > 
> > Add .plt, .data.rel.ro, .igot.*, and .iplt to discards as they are not
> > actually used. While .got.plt is also not used, it must be included
> > otherwise ld.bfd will fail to link with the error:
> > 
> >     aarch64-linux-gnu-ld: discarded output section: `.got.plt'
> > 
> > However, as it'd be better to validate that it stays effectively empty,
> > add an assert.
> > 
> > Explicitly include debug sections when they're present.
> > 
> > Fix a case of needless quotes in __section(), which Clang doesn't like.
> > 
> > Finally, enable orphan section warnings.
> > 
> > Thanks to Ard Biesheuvel for many hints on correct ways to handle
> > mysterious sections. :)
> 
> Sorry to be a pain, but this patch is doing 3 or 4 independent things at
> once. Please could you split it up a bit?
> e.g.
> 
>  - Removal of cfi directives from smccc macro
>  - Removal of quotes around section name for clang
>  - Avoid generating .eh_frame
>  - Ensure all sections are accounted for in linker script and warn on orphans
> 
> That way it's a bit easier to manage, we can revert/backport bits later if
> necessary and you get more patches in the kernel ;)

Yeah, this one patch did grow a bit. ;) I've split it up now.

> You can also add my Ack on all the patches:
> 
> Acked-by: Will Deacon <will@kernel.org>

Thanks!

-- 
Kees Cook

Re: [PATCH v3 2/9] vmlinux.lds.h: Add .symtab, .strtab, and .shstrtab to STABS_DEBUG

[Arvind Sankar]

On Tue, Jun 23, 2020 at 06:49:33PM -0700, Kees Cook wrote:
> When linking vmlinux with LLD, the synthetic sections .symtab, .strtab,
> and .shstrtab are listed as orphaned. Add them to the STABS_DEBUG section
> so there will be no warnings when --orphan-handling=warn is used more
> widely. (They are added above comment as it is the more common

Nit 1: is "after .comment" better than "above comment"? It's above in the
sense of higher file offset, but it's below in readelf output.
Nit 2: These aren't actually debugging sections, no? Is it better to add
a new macro for it, and is there any plan to stop LLD from warning about
them?

> order[1].)
> 
> ld.lld: warning: <internal>:(.symtab) is being placed in '.symtab'
> ld.lld: warning: <internal>:(.shstrtab) is being placed in '.shstrtab'
> ld.lld: warning: <internal>:(.strtab) is being placed in '.strtab'
> 
> [1] https://lore.kernel.org/lkml/20200622224928.o2a7jkq33guxfci4@google.com/
> 
> Reported-by: Fangrui Song <maskray@google.com>
> Reviewed-by: Fangrui Song <maskray@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/asm-generic/vmlinux.lds.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 1248a206be8d..8e71757f485b 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -792,7 +792,10 @@
>  		.stab.exclstr 0 : { *(.stab.exclstr) }			\
>  		.stab.index 0 : { *(.stab.index) }			\
>  		.stab.indexstr 0 : { *(.stab.indexstr) }		\
> -		.comment 0 : { *(.comment) }
> +		.comment 0 : { *(.comment) }				\
> +		.symtab 0 : { *(.symtab) }				\
> +		.strtab 0 : { *(.strtab) }				\
> +		.shstrtab 0 : { *(.shstrtab) }
>  
>  #ifdef CONFIG_GENERIC_BUG
>  #define BUG_TABLE							\
> -- 
> 2.25.1
> 

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Kees Cook]

On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> On Wed, 24 Jun 2020 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > stub, given that it runs under the control of the firmware, with its
> > > memory mappings and PAC configuration etc.
> >
> > Is BTI being ignored when the firmware runs?
> 
> Given that it requires the 'guarded' attribute to be set in the page
> tables, and the fact that the UEFI spec does not require it for
> executables that it invokes, nor describes any means of annotating
> such executables as having been built with BTI annotations, I think we
> can safely assume that the EFI stub will execute with BTI disabled in
> the foreseeable future.

yaaaaaay. *sigh* How long until EFI catches up?

That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
it, we'll be ready?

-- 
Kees Cook

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Ard Biesheuvel]

On Wed, 24 Jun 2020 at 17:45, Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > On Wed, 24 Jun 2020 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > stub, given that it runs under the control of the firmware, with its
> > > > memory mappings and PAC configuration etc.
> > >
> > > Is BTI being ignored when the firmware runs?
> >
> > Given that it requires the 'guarded' attribute to be set in the page
> > tables, and the fact that the UEFI spec does not require it for
> > executables that it invokes, nor describes any means of annotating
> > such executables as having been built with BTI annotations, I think we
> > can safely assume that the EFI stub will execute with BTI disabled in
> > the foreseeable future.
>
> yaaaaaay. *sigh* How long until EFI catches up?
>
> That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> it, we'll be ready?
>

Sure. Although I anticipate that we'll need to set some flag in the
PE/COFF header to enable it, and so any BTI opcodes we emit without
that will never take effect in practice.

Re: [PATCH v3 2/9] vmlinux.lds.h: Add .symtab, .strtab, and .shstrtab to STABS_DEBUG

[Fangrui Song]

On 2020-06-24, Arvind Sankar wrote:
>On Tue, Jun 23, 2020 at 06:49:33PM -0700, Kees Cook wrote:
>> When linking vmlinux with LLD, the synthetic sections .symtab, .strtab,
>> and .shstrtab are listed as orphaned. Add them to the STABS_DEBUG section
>> so there will be no warnings when --orphan-handling=warn is used more
>> widely. (They are added above comment as it is the more common
>
>Nit 1: is "after .comment" better than "above comment"? It's above in the
>sense of higher file offset, but it's below in readelf output.

I mean this order:)

   .comment
   .symtab
   .shstrtab
   .strtab

This is the case in the absence of a linker script if at least one object file has .comment (mostly for GCC/clang version information) or the linker is LLD which adds a .comment

>Nit 2: These aren't actually debugging sections, no? Is it better to add
>a new macro for it, and is there any plan to stop LLD from warning about
>them?

https://reviews.llvm.org/D75149 "[ELF] --orphan-handling=: don't warn/error for unused synthesized sections"
described that .symtab .shstrtab .strtab are different in GNU ld.
Since many other GNU ld synthesized sections (.rela.dyn .plt ...) can be renamed or dropped
via output section descriptions, I don't understand why the 3 sections
can't be customized.

I created a feature request: https://sourceware.org/bugzilla/show_bug.cgi?id=26168
(If this is supported, it is a consistent behavior to warn for orphan
.symtab/.strtab/.shstrtab

There may be 50% chance that the maintainer decides that "LLD diverges"
I would disagree: there is no fundamental problems with .symtab/.strtab/.shstrtab which make them special in output section descriptions or orphan handling.)

>> order[1].)
>>
>> ld.lld: warning: <internal>:(.symtab) is being placed in '.symtab'
>> ld.lld: warning: <internal>:(.shstrtab) is being placed in '.shstrtab'
>> ld.lld: warning: <internal>:(.strtab) is being placed in '.strtab'
>>
>> [1] https://lore.kernel.org/lkml/20200622224928.o2a7jkq33guxfci4@google.com/
>>
>> Reported-by: Fangrui Song <maskray@google.com>
>> Reviewed-by: Fangrui Song <maskray@google.com>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  include/asm-generic/vmlinux.lds.h | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 1248a206be8d..8e71757f485b 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -792,7 +792,10 @@
>>  		.stab.exclstr 0 : { *(.stab.exclstr) }			\
>>  		.stab.index 0 : { *(.stab.index) }			\
>>  		.stab.indexstr 0 : { *(.stab.indexstr) }		\
>> -		.comment 0 : { *(.comment) }
>> +		.comment 0 : { *(.comment) }				\
>> +		.symtab 0 : { *(.symtab) }				\
>> +		.strtab 0 : { *(.strtab) }				\
>> +		.shstrtab 0 : { *(.shstrtab) }
>>
>>  #ifdef CONFIG_GENERIC_BUG
>>  #define BUG_TABLE							\
>> --
>> 2.25.1
>>

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Dave Martin]

On Wed, Jun 24, 2020 at 04:26:46PM +0100, Will Deacon wrote:
> On Wed, Jun 24, 2020 at 02:48:55PM +0100, Dave Martin wrote:
> > On Wed, Jun 24, 2020 at 12:26:47PM +0100, Will Deacon wrote:
> > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 24 Jun 2020 at 12:44, Will Deacon <will@kernel.org> wrote:
> > > > > For the kernel Image, how do we remove these sections? The objcopy flags
> > > > > in arch/arm64/boot/Makefile look both insufficient and out of date. My
> > > > > vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> > > > > segment.
> > > > 
> > > > The latter is the fault of the libstub make rules, that prepend .init
> > > > to all section names.
> > > 
> > > Hmm. I tried adding -mbranch-protection=none to arm64 cflags for the stub,
> > > but I still see this note in vmlinux. It looks like it comes in via the
> > > stub copy of lib-ctype.o, but I don't know why that would force the
> > > note. The cflags look ok to me [1] and I confirmed that the note is
> > > being generated by the compiler.
> > > 
> > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > stub, given that it runs under the control of the firmware, with its
> > > > memory mappings and PAC configuration etc.
> > > 
> > > Agreed, I just can't figure out how to get rid of the note.
> > 
> > Because this section is generated by the linker itself I think you might
> > have to send it to /DISCARD/ in the link, or strip it explicitly after
> > linking.
> 
> Right, but why is the linker generating that section in the first place? I'm
> compiling with -mbranch-protection=none and all the other objects linked
> into the stub do not have the section.
> 
> I wonder if it's because lib/ctype.c doesn't have any executable code...

What compiler and flags are you using for the affected object?  I don't
see this with gcc so far.

I wonder if this is a hole in the specs: the property could logically
be emitted in any codeless object, since turning on BTI will obviously
not break that object.

For different linkers and compilers to interoperate though, the specs
would need to say what to do in that situation.

Cheers
---Dave



> 
> Will
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Dave Martin]

On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> On Wed, 24 Jun 2020 at 17:45, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > stub, given that it runs under the control of the firmware, with its
> > > > > memory mappings and PAC configuration etc.
> > > >
> > > > Is BTI being ignored when the firmware runs?
> > >
> > > Given that it requires the 'guarded' attribute to be set in the page
> > > tables, and the fact that the UEFI spec does not require it for
> > > executables that it invokes, nor describes any means of annotating
> > > such executables as having been built with BTI annotations, I think we
> > > can safely assume that the EFI stub will execute with BTI disabled in
> > > the foreseeable future.
> >
> > yaaaaaay. *sigh* How long until EFI catches up?
> >
> > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > it, we'll be ready?
> >
> 
> Sure. Although I anticipate that we'll need to set some flag in the
> PE/COFF header to enable it, and so any BTI opcodes we emit without
> that will never take effect in practice.

In the meantime, it is possible to build all the in-tree parts of EFI
for BTI, and just turn it off for out-of-tree EFI binaries?

If there's no easy way to do this though, I guess we should wait for /
push for a PE/COFF flag to describe this properly.

Cheers
---Dave

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Ard Biesheuvel]

On Wed, 24 Jun 2020 at 18:29, Dave Martin <Dave.Martin@arm.com> wrote:
>
> On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> > On Wed, 24 Jun 2020 at 17:45, Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> > > > >
> > > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > > stub, given that it runs under the control of the firmware, with its
> > > > > > memory mappings and PAC configuration etc.
> > > > >
> > > > > Is BTI being ignored when the firmware runs?
> > > >
> > > > Given that it requires the 'guarded' attribute to be set in the page
> > > > tables, and the fact that the UEFI spec does not require it for
> > > > executables that it invokes, nor describes any means of annotating
> > > > such executables as having been built with BTI annotations, I think we
> > > > can safely assume that the EFI stub will execute with BTI disabled in
> > > > the foreseeable future.
> > >
> > > yaaaaaay. *sigh* How long until EFI catches up?
> > >
> > > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > > it, we'll be ready?
> > >
> >
> > Sure. Although I anticipate that we'll need to set some flag in the
> > PE/COFF header to enable it, and so any BTI opcodes we emit without
> > that will never take effect in practice.
>
> In the meantime, it is possible to build all the in-tree parts of EFI
> for BTI, and just turn it off for out-of-tree EFI binaries?
>

Not sure I understand the question. What do you mean by out-of-tree
EFI binaries? And how would the firmware (which is out of tree itself,
and is in charge of the page tables, vector table, timer interrupt etc
when the EFI stub executes) distinguish such binaries from the EFI
stub?


> If there's no easy way to do this though, I guess we should wait for /
> push for a PE/COFF flag to describe this properly.
>

Yeah good point. I will take this to the forum.

Re: [PATCH v3 2/9] vmlinux.lds.h: Add .symtab, .strtab, and .shstrtab to STABS_DEBUG

[Arvind Sankar]

On Wed, Jun 24, 2020 at 09:16:43AM -0700, Fangrui Song wrote:
> 
> On 2020-06-24, Arvind Sankar wrote:
> >On Tue, Jun 23, 2020 at 06:49:33PM -0700, Kees Cook wrote:
> >> When linking vmlinux with LLD, the synthetic sections .symtab, .strtab,
> >> and .shstrtab are listed as orphaned. Add them to the STABS_DEBUG section
> >> so there will be no warnings when --orphan-handling=warn is used more
> >> widely. (They are added above comment as it is the more common
> >
> >Nit 1: is "after .comment" better than "above comment"? It's above in the
> >sense of higher file offset, but it's below in readelf output.
> 
> I mean this order:)
> 
>    .comment
>    .symtab
>    .shstrtab
>    .strtab
> 
> This is the case in the absence of a linker script if at least one object file has .comment (mostly for GCC/clang version information) or the linker is LLD which adds a .comment
> 
> >Nit 2: These aren't actually debugging sections, no? Is it better to add
> >a new macro for it, and is there any plan to stop LLD from warning about
> >them?
> 
> https://reviews.llvm.org/D75149 "[ELF] --orphan-handling=: don't warn/error for unused synthesized sections"
> described that .symtab .shstrtab .strtab are different in GNU ld.
> Since many other GNU ld synthesized sections (.rela.dyn .plt ...) can be renamed or dropped
> via output section descriptions, I don't understand why the 3 sections
> can't be customized.

So IIUC, lld will now warn about .rela.dyn etc only if they're non-empty?

> 
> I created a feature request: https://sourceware.org/bugzilla/show_bug.cgi?id=26168
> (If this is supported, it is a consistent behavior to warn for orphan
> .symtab/.strtab/.shstrtab
> 
> There may be 50% chance that the maintainer decides that "LLD diverges"
> I would disagree: there is no fundamental problems with .symtab/.strtab/.shstrtab which make them special in output section descriptions or orphan handling.)
> 

.shstrtab is a little special in that it can't be discarded if the ELF
file contains any sections at all. But yeah, there's no reason they
can't be renamed or placed in a custom location in the file.

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Dave Martin]

On Wed, Jun 24, 2020 at 06:40:48PM +0200, Ard Biesheuvel wrote:
> On Wed, 24 Jun 2020 at 18:29, Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 24 Jun 2020 at 17:45, Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> > > > > >
> > > > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > > > stub, given that it runs under the control of the firmware, with its
> > > > > > > memory mappings and PAC configuration etc.
> > > > > >
> > > > > > Is BTI being ignored when the firmware runs?
> > > > >
> > > > > Given that it requires the 'guarded' attribute to be set in the page
> > > > > tables, and the fact that the UEFI spec does not require it for
> > > > > executables that it invokes, nor describes any means of annotating
> > > > > such executables as having been built with BTI annotations, I think we
> > > > > can safely assume that the EFI stub will execute with BTI disabled in
> > > > > the foreseeable future.
> > > >
> > > > yaaaaaay. *sigh* How long until EFI catches up?
> > > >
> > > > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > > > it, we'll be ready?
> > > >
> > >
> > > Sure. Although I anticipate that we'll need to set some flag in the
> > > PE/COFF header to enable it, and so any BTI opcodes we emit without
> > > that will never take effect in practice.
> >
> > In the meantime, it is possible to build all the in-tree parts of EFI
> > for BTI, and just turn it off for out-of-tree EFI binaries?
> >
> 
> Not sure I understand the question. What do you mean by out-of-tree
> EFI binaries? And how would the firmware (which is out of tree itself,
> and is in charge of the page tables, vector table, timer interrupt etc
> when the EFI stub executes) distinguish such binaries from the EFI
> stub?

I'm not an EFI expert, but I'm guessing that you configure EFI with
certain compiler flags and build it.  Possibly some standalone EFI
executables are built out of the same tree and shipped with the
firmware from the same build, but I'm speculating.  If not, we can just
run all EFI executables with BTI off.

> > If there's no easy way to do this though, I guess we should wait for /
> > push for a PE/COFF flag to describe this properly.
> >
> 
> Yeah good point. I will take this to the forum.

In the interim, we could set the GP bit in EFI's page tables for the
executable code from the firmware image if we want this protection, but
turn it off in pages mapping the executable code of EFI executables.
This is better than nothing.

Cheers
---Dave

Re: [PATCH v3 2/9] vmlinux.lds.h: Add .symtab, .strtab, and .shstrtab to STABS_DEBUG

[Fangrui Song]

On 2020-06-24, Arvind Sankar wrote:
>On Wed, Jun 24, 2020 at 09:16:43AM -0700, Fangrui Song wrote:
>>
>> On 2020-06-24, Arvind Sankar wrote:
>> >On Tue, Jun 23, 2020 at 06:49:33PM -0700, Kees Cook wrote:
>> >> When linking vmlinux with LLD, the synthetic sections .symtab, .strtab,
>> >> and .shstrtab are listed as orphaned. Add them to the STABS_DEBUG section
>> >> so there will be no warnings when --orphan-handling=warn is used more
>> >> widely. (They are added above comment as it is the more common
>> >
>> >Nit 1: is "after .comment" better than "above comment"? It's above in the
>> >sense of higher file offset, but it's below in readelf output.
>>
>> I mean this order:)
>>
>>    .comment
>>    .symtab
>>    .shstrtab
>>    .strtab
>>
>> This is the case in the absence of a linker script if at least one object file has .comment (mostly for GCC/clang version information) or the linker is LLD which adds a .comment
>>
>> >Nit 2: These aren't actually debugging sections, no? Is it better to add
>> >a new macro for it, and is there any plan to stop LLD from warning about
>> >them?
>>
>> https://reviews.llvm.org/D75149 "[ELF] --orphan-handling=: don't warn/error for unused synthesized sections"
>> described that .symtab .shstrtab .strtab are different in GNU ld.
>> Since many other GNU ld synthesized sections (.rela.dyn .plt ...) can be renamed or dropped
>> via output section descriptions, I don't understand why the 3 sections
>> can't be customized.
>
>So IIUC, lld will now warn about .rela.dyn etc only if they're non-empty?

HEAD and future 11.0.0 will not warn about unused synthesized sections
like .rela.dyn

For most synthesized sections, empty = unused.

>>
>> I created a feature request: https://sourceware.org/bugzilla/show_bug.cgi?id=26168
>> (If this is supported, it is a consistent behavior to warn for orphan
>> .symtab/.strtab/.shstrtab
>>
>> There may be 50% chance that the maintainer decides that "LLD diverges"
>> I would disagree: there is no fundamental problems with .symtab/.strtab/.shstrtab which make them special in output section descriptions or orphan handling.)
>>
>
>.shstrtab is a little special in that it can't be discarded if the ELF
>file contains any sections at all. But yeah, there's no reason they
>can't be renamed or placed in a custom location in the file.

https://sourceware.org/pipermail/binutils/2020-March/000179.html
proposes -z nosectionheader. With this option, I believe .shstrtab is
not needed. /DISCARD/ : { *(.shstrtab) }  should achieve a similar effect.

Re: [PATCH v3 2/9] vmlinux.lds.h: Add .symtab, .strtab, and .shstrtab to STABS_DEBUG

[Arvind Sankar]

On Wed, Jun 24, 2020 at 10:26:20AM -0700, Fangrui Song wrote:
> 
> On 2020-06-24, Arvind Sankar wrote:
> >On Wed, Jun 24, 2020 at 09:16:43AM -0700, Fangrui Song wrote:
> >>
> >> On 2020-06-24, Arvind Sankar wrote:
> >> >On Tue, Jun 23, 2020 at 06:49:33PM -0700, Kees Cook wrote:
> >> >> When linking vmlinux with LLD, the synthetic sections .symtab, .strtab,
> >> >> and .shstrtab are listed as orphaned. Add them to the STABS_DEBUG section
> >> >> so there will be no warnings when --orphan-handling=warn is used more
> >> >> widely. (They are added above comment as it is the more common
> >> >
> >> >Nit 1: is "after .comment" better than "above comment"? It's above in the
> >> >sense of higher file offset, but it's below in readelf output.
> >>
> >> I mean this order:)
> >>
> >>    .comment
> >>    .symtab
> >>    .shstrtab
> >>    .strtab
> >>
> >> This is the case in the absence of a linker script if at least one object file has .comment (mostly for GCC/clang version information) or the linker is LLD which adds a .comment
> >>
> >> >Nit 2: These aren't actually debugging sections, no? Is it better to add
> >> >a new macro for it, and is there any plan to stop LLD from warning about
> >> >them?
> >>
> >> https://reviews.llvm.org/D75149 "[ELF] --orphan-handling=: don't warn/error for unused synthesized sections"
> >> described that .symtab .shstrtab .strtab are different in GNU ld.
> >> Since many other GNU ld synthesized sections (.rela.dyn .plt ...) can be renamed or dropped
> >> via output section descriptions, I don't understand why the 3 sections
> >> can't be customized.
> >
> >So IIUC, lld will now warn about .rela.dyn etc only if they're non-empty?
> 
> HEAD and future 11.0.0 will not warn about unused synthesized sections
> like .rela.dyn
> 
> For most synthesized sections, empty = unused.
> 
> >>
> >> I created a feature request: https://sourceware.org/bugzilla/show_bug.cgi?id=26168
> >> (If this is supported, it is a consistent behavior to warn for orphan
> >> .symtab/.strtab/.shstrtab
> >>
> >> There may be 50% chance that the maintainer decides that "LLD diverges"
> >> I would disagree: there is no fundamental problems with .symtab/.strtab/.shstrtab which make them special in output section descriptions or orphan handling.)
> >>
> >
> >.shstrtab is a little special in that it can't be discarded if the ELF
> >file contains any sections at all. But yeah, there's no reason they
> >can't be renamed or placed in a custom location in the file.
> 
> https://sourceware.org/pipermail/binutils/2020-March/000179.html
> proposes -z nosectionheader. With this option, I believe .shstrtab is
> not needed. /DISCARD/ : { *(.shstrtab) }  should achieve a similar effect.

oh wow.

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Ard Biesheuvel]

On Wed, 24 Jun 2020 at 19:16, Dave Martin <Dave.Martin@arm.com> wrote:
>
> On Wed, Jun 24, 2020 at 06:40:48PM +0200, Ard Biesheuvel wrote:
> > On Wed, 24 Jun 2020 at 18:29, Dave Martin <Dave.Martin@arm.com> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 24 Jun 2020 at 17:45, Kees Cook <keescook@chromium.org> wrote:
> > > > >
> > > > > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > > > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> > > > > > >
> > > > > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > > > > stub, given that it runs under the control of the firmware, with its
> > > > > > > > memory mappings and PAC configuration etc.
> > > > > > >
> > > > > > > Is BTI being ignored when the firmware runs?
> > > > > >
> > > > > > Given that it requires the 'guarded' attribute to be set in the page
> > > > > > tables, and the fact that the UEFI spec does not require it for
> > > > > > executables that it invokes, nor describes any means of annotating
> > > > > > such executables as having been built with BTI annotations, I think we
> > > > > > can safely assume that the EFI stub will execute with BTI disabled in
> > > > > > the foreseeable future.
> > > > >
> > > > > yaaaaaay. *sigh* How long until EFI catches up?
> > > > >
> > > > > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > > > > it, we'll be ready?
> > > > >
> > > >
> > > > Sure. Although I anticipate that we'll need to set some flag in the
> > > > PE/COFF header to enable it, and so any BTI opcodes we emit without
> > > > that will never take effect in practice.
> > >
> > > In the meantime, it is possible to build all the in-tree parts of EFI
> > > for BTI, and just turn it off for out-of-tree EFI binaries?
> > >
> >
> > Not sure I understand the question. What do you mean by out-of-tree
> > EFI binaries? And how would the firmware (which is out of tree itself,
> > and is in charge of the page tables, vector table, timer interrupt etc
> > when the EFI stub executes) distinguish such binaries from the EFI
> > stub?
>
> I'm not an EFI expert, but I'm guessing that you configure EFI with
> certain compiler flags and build it.

'EFI' is not something you build. It is a specification that describes
how a conformant firmware implementation interfaces with a conformant
OS.

Sorry to be pedantic, but that is really quite relevant. By adhering
to the EFI spec rigorously, we no longer have to care about who
implements the opposite side, and how.

So yes, of course there are ways to build the opposite side with BTI
enabled, in a way that all its constituent pieces keep working as
expected. A typical EDK2 based implementation of EFI consists of
50-100 individual PE/COFF executables that all get loaded, relocated
and started like ordinary user space programs.

What we cannot do, though, is invent our own Linux specific way of
decorating the kernel's PE/COFF header with an annotation that
instructs a Linux specific EFI loader when to enable the GP bit for
the .text pages.

> Possibly some standalone EFI
> executables are built out of the same tree and shipped with the
> firmware from the same build, but I'm speculating.  If not, we can just
> run all EFI executables with BTI off.
>
> > > If there's no easy way to do this though, I guess we should wait for /
> > > push for a PE/COFF flag to describe this properly.
> > >
> >
> > Yeah good point. I will take this to the forum.
>
> In the interim, we could set the GP bit in EFI's page tables for the
> executable code from the firmware image if we want this protection, but
> turn it off in pages mapping the executable code of EFI executables.
> This is better than nothing.
>

We need to distinguish between the EFI stub and the EFI runtime services here.

The EFI stub consists of kernel code that executes in the context of
the firmware, at which point the loader has no control whatsoever over
page tables, vector tables, etc. This is the stage where the loading
and starting of PE/COFF images takes place. If we want to enable BTI
for code running in this context, we need PE/COFF annotations, as
discussed above.

The EFI runtime services are firmware code that gets invoked by the OS
at runtime. Whether or not such code is emitted with BTI annotations
is a separate matter (but should also be taken to the forum
nonetheless), and does not need any changes at the PE/COFF level.
However, for this code, I'd like the sandboxing to be much more
rigorous than it is today, to the point where the security it provides
doesn't even matter deeply to the OS itself. (I had some patches a
while ago that reused the KPTI infrastructure to unmap the entire
kernel while EFI runtime services are in progress. There was also an
intern in the team that implemented something similar on top of KVM)

Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

[Ard Biesheuvel]

On Wed, 24 Jun 2020 at 20:23, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 24 Jun 2020 at 19:16, Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Wed, Jun 24, 2020 at 06:40:48PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 24 Jun 2020 at 18:29, Dave Martin <Dave.Martin@arm.com> wrote:
> > > >
> > > > On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> > > > > On Wed, 24 Jun 2020 at 17:45, Kees Cook <keescook@chromium.org> wrote:
> > > > > >
> > > > > > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > > > > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> > > > > > > >
> > > > > > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > > > > > stub, given that it runs under the control of the firmware, with its
> > > > > > > > > memory mappings and PAC configuration etc.
> > > > > > > >
> > > > > > > > Is BTI being ignored when the firmware runs?
> > > > > > >
> > > > > > > Given that it requires the 'guarded' attribute to be set in the page
> > > > > > > tables, and the fact that the UEFI spec does not require it for
> > > > > > > executables that it invokes, nor describes any means of annotating
> > > > > > > such executables as having been built with BTI annotations, I think we
> > > > > > > can safely assume that the EFI stub will execute with BTI disabled in
> > > > > > > the foreseeable future.
> > > > > >
> > > > > > yaaaaaay. *sigh* How long until EFI catches up?
> > > > > >
> > > > > > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > > > > > it, we'll be ready?
> > > > > >
> > > > >
> > > > > Sure. Although I anticipate that we'll need to set some flag in the
> > > > > PE/COFF header to enable it, and so any BTI opcodes we emit without
> > > > > that will never take effect in practice.
> > > >
> > > > In the meantime, it is possible to build all the in-tree parts of EFI
> > > > for BTI, and just turn it off for out-of-tree EFI binaries?
> > > >
> > >
> > > Not sure I understand the question. What do you mean by out-of-tree
> > > EFI binaries? And how would the firmware (which is out of tree itself,
> > > and is in charge of the page tables, vector table, timer interrupt etc
> > > when the EFI stub executes) distinguish such binaries from the EFI
> > > stub?
> >
> > I'm not an EFI expert, but I'm guessing that you configure EFI with
> > certain compiler flags and build it.
>
> 'EFI' is not something you build. It is a specification that describes
> how a conformant firmware implementation interfaces with a conformant
> OS.
>
> Sorry to be pedantic, but that is really quite relevant. By adhering
> to the EFI spec rigorously, we no longer have to care about who
> implements the opposite side, and how.
>
> So yes, of course there are ways to build the opposite side with BTI
> enabled, in a way that all its constituent pieces keep working as
> expected. A typical EDK2 based implementation of EFI consists of
> 50-100 individual PE/COFF executables that all get loaded, relocated
> and started like ordinary user space programs.
>
> What we cannot do, though, is invent our own Linux specific way of
> decorating the kernel's PE/COFF header with an annotation that
> instructs a Linux specific EFI loader when to enable the GP bit for
> the .text pages.
>
> > Possibly some standalone EFI
> > executables are built out of the same tree and shipped with the
> > firmware from the same build, but I'm speculating.  If not, we can just
> > run all EFI executables with BTI off.
> >
> > > > If there's no easy way to do this though, I guess we should wait for /
> > > > push for a PE/COFF flag to describe this properly.
> > > >
> > >
> > > Yeah good point. I will take this to the forum.
> >
> > In the interim, we could set the GP bit in EFI's page tables for the
> > executable code from the firmware image if we want this protection, but
> > turn it off in pages mapping the executable code of EFI executables.
> > This is better than nothing.
> >
>
> We need to distinguish between the EFI stub and the EFI runtime services here.
>
> The EFI stub consists of kernel code that executes in the context of
> the firmware, at which point the loader has no control whatsoever over
> page tables, vector tables, etc. This is the stage where the loading
> and starting of PE/COFF images takes place. If we want to enable BTI
> for code running in this context, we need PE/COFF annotations, as
> discussed above.
>
> The EFI runtime services are firmware code that gets invoked by the OS
> at runtime. Whether or not such code is emitted with BTI annotations
> is a separate matter (but should also be taken to the forum
> nonetheless), and does not need any changes at the PE/COFF level.
> However, for this code, I'd like the sandboxing to be much more
> rigorous than it is today, to the point where the security it provides

...  the security *bti* provides ...

> doesn't even matter deeply to the OS itself. (I had some patches a
> while ago that reused the KPTI infrastructure to unmap the entire
> kernel while EFI runtime services are in progress. There was also an
> intern in the team that implemented something similar on top of KVM)

Re: [PATCH v3 4/9] x86/build: Warn on orphan section placement

[Kees Cook]

On Thu, Jun 25, 2020 at 02:36:27AM +0800, kernel test robot wrote:
> I love your patch! Perhaps something to improve:
> [...]
> config: x86_64-randconfig-a012-20200624 (attached as .config)

CONFIG_KCSAN=y

> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 1d4c87335d5236ea1f35937e1014980ba961ae34)
> [...]
> All warnings (new ones prefixed by >>):
> 
>    ld.lld: warning: drivers/built-in.a(mfd/mt6397-irq.o):(.init_array.0) is being placed in '.init_array.0'

As far as I can tell, this is a Clang bug. But I don't know the
internals here, so I've opened:
https://bugs.llvm.org/show_bug.cgi?id=46478

and created a work-around patch for the kernel:


commit 915f2c343e59a14f00c68f4d7afcfdc621de0674
Author: Kees Cook <keescook@chromium.org>
Date:   Sat Jun 27 08:07:54 2020 -0700

    vmlinux.lds.h: Avoid KCSAN's unwanted sections
    
    KCSAN (-fsanitize=thread) produces unwanted[1] .eh_frame and .init_array.*
    sections. Add them to DISCARDS, except with CONFIG_CONSTRUCTORS, which
    wants to keep .init_array.* sections.
    
    [1] https://bugs.llvm.org/show_bug.cgi?id=46478
    
    Signed-off-by: Kees Cook <keescook@chromium.org>

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index f8a5b2333729..41c8c73de6c4 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -195,7 +195,9 @@ endif
 # Workaround for a gcc prelease that unfortunately was shipped in a suse release
 KBUILD_CFLAGS += -Wno-sign-compare
 #
-KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
+KBUILD_AFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables
+KBUILD_CFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables
+KBUILD_LDFLAGS += $(call ld-option,--no-ld-generated-unwind-info)
 
 # Avoid indirect branches in kernel to deal with Spectre
 ifdef CONFIG_RETPOLINE
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b1dca0762fc5..a44ee16abc78 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -934,10 +934,28 @@
 	EXIT_DATA
 #endif
 
+/*
+ * Clang's -fsanitize=thread produces unwanted sections (.eh_frame
+ * and .init_array.*), but CONFIG_CONSTRUCTORS wants to keep any
+ * .init_array.* sections.
+ * https://bugs.llvm.org/show_bug.cgi?id=46478
+ */
+#if defined(CONFIG_KCSAN) && !defined(CONFIG_CONSTRUCTORS)
+#define KCSAN_DISCARDS	 						\
+	*(.init_array) *(.init_array.*)					\
+	*(.eh_frame)
+#elif defined(CONFIG_KCSAN) && defined(CONFIG_CONSTRUCTORS)
+#define KCSAN_DISCARDS	 						\
+	*(.eh_frame)
+#else
+#define KCSAN_DISCARDS
+#endif
+
 #define DISCARDS							\
 	/DISCARD/ : {							\
 	EXIT_DISCARDS							\
 	EXIT_CALL							\
+	KCSAN_DISCARDS							\
 	*(.discard)							\
 	*(.discard.*)							\
 	*(.modinfo)							\

-- 
Kees Cook

Re: [PATCH v3 4/9] x86/build: Warn on orphan section placement

[Marco Elver]

On Sat, 27 Jun 2020 at 17:44, Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Jun 25, 2020 at 02:36:27AM +0800, kernel test robot wrote:
> > I love your patch! Perhaps something to improve:
> > [...]
> > config: x86_64-randconfig-a012-20200624 (attached as .config)
>
> CONFIG_KCSAN=y
>
> > compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 1d4c87335d5236ea1f35937e1014980ba961ae34)
> > [...]
> > All warnings (new ones prefixed by >>):
> >
> >    ld.lld: warning: drivers/built-in.a(mfd/mt6397-irq.o):(.init_array.0) is being placed in '.init_array.0'
>
> As far as I can tell, this is a Clang bug. But I don't know the
> internals here, so I've opened:
> https://bugs.llvm.org/show_bug.cgi?id=46478
>
> and created a work-around patch for the kernel:

Thanks, minor comments below.

With KCSAN this is:

Tested-by: Marco Elver <elver@google.com>


> commit 915f2c343e59a14f00c68f4d7afcfdc621de0674
> Author: Kees Cook <keescook@chromium.org>
> Date:   Sat Jun 27 08:07:54 2020 -0700
>
>     vmlinux.lds.h: Avoid KCSAN's unwanted sections

Since you found that it's also KASAN, this probably wants updating.

>     KCSAN (-fsanitize=thread) produces unwanted[1] .eh_frame and .init_array.*
>     sections. Add them to DISCARDS, except with CONFIG_CONSTRUCTORS, which
>     wants to keep .init_array.* sections.
>
>     [1] https://bugs.llvm.org/show_bug.cgi?id=46478
>
>     Signed-off-by: Kees Cook <keescook@chromium.org>
>
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index f8a5b2333729..41c8c73de6c4 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -195,7 +195,9 @@ endif
>  # Workaround for a gcc prelease that unfortunately was shipped in a suse release
>  KBUILD_CFLAGS += -Wno-sign-compare
>  #
> -KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
> +KBUILD_AFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables
> +KBUILD_CFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables
> +KBUILD_LDFLAGS += $(call ld-option,--no-ld-generated-unwind-info)

Why are they needed? They are not mentioned in the commit message.

>  # Avoid indirect branches in kernel to deal with Spectre
>  ifdef CONFIG_RETPOLINE
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index b1dca0762fc5..a44ee16abc78 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -934,10 +934,28 @@
>         EXIT_DATA
>  #endif
>
> +/*
> + * Clang's -fsanitize=thread produces unwanted sections (.eh_frame
> + * and .init_array.*), but CONFIG_CONSTRUCTORS wants to keep any
> + * .init_array.* sections.
> + * https://bugs.llvm.org/show_bug.cgi?id=46478
> + */
> +#if defined(CONFIG_KCSAN) && !defined(CONFIG_CONSTRUCTORS)

CONFIG_KASAN as well?

> +#define KCSAN_DISCARDS                                                 \
> +       *(.init_array) *(.init_array.*)                                 \
> +       *(.eh_frame)
> +#elif defined(CONFIG_KCSAN) && defined(CONFIG_CONSTRUCTORS)
> +#define KCSAN_DISCARDS                                                 \
> +       *(.eh_frame)
> +#else
> +#define KCSAN_DISCARDS
> +#endif
> +
>  #define DISCARDS                                                       \
>         /DISCARD/ : {                                                   \
>         EXIT_DISCARDS                                                   \
>         EXIT_CALL                                                       \
> +       KCSAN_DISCARDS                                                  \

Maybe just 'SANITIZER_DISCARDS'?

>         *(.discard)                                                     \
>         *(.discard.*)                                                   \
>         *(.modinfo)                                                     \
>
> --
> Kees Cook

Re: [PATCH v3 4/9] x86/build: Warn on orphan section placement

[Kees Cook]

On Mon, Jun 29, 2020 at 04:54:13PM +0200, Marco Elver wrote:
> On Sat, 27 Jun 2020 at 17:44, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, Jun 25, 2020 at 02:36:27AM +0800, kernel test robot wrote:
> > > I love your patch! Perhaps something to improve:
> > > [...]
> > > config: x86_64-randconfig-a012-20200624 (attached as .config)
> >
> > CONFIG_KCSAN=y
> >
> > > compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 1d4c87335d5236ea1f35937e1014980ba961ae34)
> > > [...]
> > > All warnings (new ones prefixed by >>):
> > >
> > >    ld.lld: warning: drivers/built-in.a(mfd/mt6397-irq.o):(.init_array.0) is being placed in '.init_array.0'
> >
> > As far as I can tell, this is a Clang bug. But I don't know the
> > internals here, so I've opened:
> > https://bugs.llvm.org/show_bug.cgi?id=46478
> >
> > and created a work-around patch for the kernel:
> 
> Thanks, minor comments below.
> 
> With KCSAN this is:
> 
> Tested-by: Marco Elver <elver@google.com>

Thanks!

> 
> 
> > commit 915f2c343e59a14f00c68f4d7afcfdc621de0674
> > Author: Kees Cook <keescook@chromium.org>
> > Date:   Sat Jun 27 08:07:54 2020 -0700
> >
> >     vmlinux.lds.h: Avoid KCSAN's unwanted sections
> 
> Since you found that it's also KASAN, this probably wants updating.

Yeah, I found that while testing the v4 series and updated the patch
there.

> >     KCSAN (-fsanitize=thread) produces unwanted[1] .eh_frame and .init_array.*
> >     sections. Add them to DISCARDS, except with CONFIG_CONSTRUCTORS, which
> >     wants to keep .init_array.* sections.
> >
> >     [1] https://bugs.llvm.org/show_bug.cgi?id=46478
> >
> >     Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> > index f8a5b2333729..41c8c73de6c4 100644
> > --- a/arch/x86/Makefile
> > +++ b/arch/x86/Makefile
> > @@ -195,7 +195,9 @@ endif
> >  # Workaround for a gcc prelease that unfortunately was shipped in a suse release
> >  KBUILD_CFLAGS += -Wno-sign-compare
> >  #
> > -KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
> > +KBUILD_AFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables
> > +KBUILD_CFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables
> > +KBUILD_LDFLAGS += $(call ld-option,--no-ld-generated-unwind-info)
> 
> Why are they needed? They are not mentioned in the commit message.

This was a mis-applied chunk (I also noticed this in the v4).

> > +/*
> > + * Clang's -fsanitize=thread produces unwanted sections (.eh_frame
> > + * and .init_array.*), but CONFIG_CONSTRUCTORS wants to keep any
> > + * .init_array.* sections.
> > + * https://bugs.llvm.org/show_bug.cgi?id=46478
> > + */
> > +#if defined(CONFIG_KCSAN) && !defined(CONFIG_CONSTRUCTORS)
> 
> CONFIG_KASAN as well?
> 
> > +#define KCSAN_DISCARDS                                                 \
> > +       *(.init_array) *(.init_array.*)                                 \
> > +       *(.eh_frame)
> > +#elif defined(CONFIG_KCSAN) && defined(CONFIG_CONSTRUCTORS)
> > +#define KCSAN_DISCARDS                                                 \
> > +       *(.eh_frame)
> > +#else
> > +#define KCSAN_DISCARDS
> > +#endif
> > +
> >  #define DISCARDS                                                       \
> >         /DISCARD/ : {                                                   \
> >         EXIT_DISCARDS                                                   \
> >         EXIT_CALL                                                       \
> > +       KCSAN_DISCARDS                                                  \
> 
> Maybe just 'SANITIZER_DISCARDS'?

Sure! I will rename it.

> 
> >         *(.discard)                                                     \
> >         *(.discard.*)                                                   \
> >         *(.modinfo)                                                     \
> >
> > --
> > Kees Cook

-- 
Kees Cook