| Message ID | 20200622204915.2987555-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: Warn on orphan section placement | expand |
On Mon, Jun 22, 2020 at 1:49 PM Kees Cook <keescook@chromium.org> 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. > > 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) \ I could have sworn that someone (Eli?) once told me that this section (.ARM.attributes) is used for disambiguating which ARM version or which optional extensions were used when compiling, and that without this section, one would not be able to disassemble 32b ARM precisely. If that's the case, we might not want to discard it? In fact, in LLVM, I can see quite a few tests under llvm/test/MC/ARM/directive-arch-armv*.s that reference .ARM.attributes. Looks like `{llvm|arm-linux-gnueabihf}-readelf --arch-specific` can be used to dump these sections. Though I also only see code in LLVM's tree for writing this, not necessarily reading it. Only did a cursory scan of llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp. Otherwise patch LGTM. > + *(.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) \ This changes the order of .glue_7t relative to .glue_7. Maybe that doesn't matter. > + *(.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 >
On Tue, Jun 23, 2020 at 05:03:46PM -0700, Nick Desaulniers wrote: > On Mon, Jun 22, 2020 at 1:49 PM Kees Cook <keescook@chromium.org> wrote: > > [...] > > @@ -37,6 +38,13 @@ > > *(.idmap.text) \ > > __idmap_text_end = .; \ > > > > +#define ARM_COMMON_DISCARD \ > > + *(.ARM.attributes) \ > > I could have sworn that someone (Eli?) once told me that this section > (.ARM.attributes) is used for disambiguating which ARM version or > which optional extensions were used when compiling, and that without > this section, one would not be able to disassemble 32b ARM precisely. > If that's the case, we might not want to discard it? Perhaps we want to treat it like .comment and include it in the ELF? > > +#define ARM_STUBS_TEXT \ > > + *(.gnu.warning) \ > > + *(.glue_7t) \ > > + *(.glue_7) \ > > This changes the order of .glue_7t relative to .glue_7. Maybe that > doesn't matter. Good point. I'll swap it just for consistency. Thanks!
On Tue, Jun 23, 2020 at 5:03 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Mon, Jun 22, 2020 at 1:49 PM Kees Cook <keescook@chromium.org> wrote: > > > > --- 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) \ > > I could have sworn that someone (Eli?) once told me that this section > (.ARM.attributes) is used for disambiguating which ARM version or > which optional extensions were used when compiling, and that without > this section, one would not be able to disassemble 32b ARM precisely. > If that's the case, we might not want to discard it? Yep, looks like ELFObjectFileBase::getARMFeatures() in llvm/lib/Object/ELFObjectFile.cpp does exactly that and more. https://github.com/llvm/llvm-project/blob/8808574e7438c8768b78ae7dd0f029385c6df01d/llvm/lib/Object/ELFObjectFile.cpp#L359-L441 https://github.com/llvm/llvm-project/blob/8808574e7438c8768b78ae7dd0f029385c6df01d/llvm/lib/Object/ELFObjectFile.cpp#L159-L287 As a test, let's do: $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make LLVM=1 -j71 defconfig (so armv7) $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make LLVM=1 -j71 (then pick any random object file) $ llvm-readelf -S arch/arm/kernel/bugs.o | grep attri [15] .ARM.attributes ARM_ATTRIBUTES 00000000 0000f7 000037 00 0 0 1 $ llvm-readelf --arch-specific arch/arm/kernel/bugs.o | grep -A 2 CPU_arch TagName: CPU_arch Description: ARM v7 } And let's see if this actually has a difference on the disassembly. $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make LLVM=1 -j71 (full build, since we're talking about linker script changes for vmlinux) $ llvm-objdump -d vmlinux > prepatch.txt (apply your patch) $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make LLVM=1 -j71 clean $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make LLVM=1 -j71 $ llvm-objdump -d vmlinux > postpatch.txt $ diff -u prepatch.txt postpatch.txt | less No difference. Eh. Checking again with arm-linux-gnueabihf-objdump, it seems some constants are slightly different for `movw`'s though. Not sure what's that about. If I enable CONFIG_THUMB2_KERNEL=y, is where things become interesting. llvm-objdump produces wildly different disassembly before vs after removing .ARM.attributes. There's also lots of decode errors in the disassembly. Repeating the thumb2 test with GNU objdump, I only see slight differences in constants values for operands to `movw`. So it looks like GNU objdump doesn't rely on .ARM.attributes to disambiguate between ARM vs THUMB2 instructions like llvm-objdump does. We can probably improve llvm-objdump, but I'd rather not discard this section for now. (also, I didn't test armv6, v5, etc, but those might be interesting tests, too, should we want to discard this section. Also, I think we can explicitly specify --triple=thumbv7-linux-gnueabihf to llvm-objdump, but I'd prefer it if my disassembler did the work for me, since I'm lazy) (oh man, the bytes are printed with different endianness between arm-linux-gnueabihf-objdump and llvm-objdump...guessing that's a bug in llvm). -- Thanks, ~Nick Desaulniers
On Fri, Jun 26, 2020 at 02:36:44PM -0700, Nick Desaulniers wrote: > If I enable CONFIG_THUMB2_KERNEL=y, is where things become > interesting. llvm-objdump produces wildly different disassembly before > vs after removing .ARM.attributes. There's also lots of decode errors > in the disassembly. Yeah, at your earlier recommendation my v4 series will be keeping .ARM.attributes. Thanks for verifying!
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
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%)