* Re: [PATCH v2 1/2] arm/build: Warn on orphan section placement [not found] ` <20200622204915.2987555-2-keescook@chromium.org> @ 2020-06-24 0:03 ` Nick Desaulniers 2020-06-24 19:43 ` Kees Cook 2020-06-26 21:36 ` Nick Desaulniers 0 siblings, 2 replies; 5+ messages in thread From: Nick Desaulniers @ 2020-06-24 0:03 UTC (permalink / raw) To: Kees Cook Cc: Arnd Bergmann, Masahiro Yamada, Eli Friedman, Russell King, LKML, Nathan Chancellor, Will Deacon, Ard Biesheuvel, Linux ARM 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 > -- Thanks, ~Nick Desaulniers _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] arm/build: Warn on orphan section placement 2020-06-24 0:03 ` [PATCH v2 1/2] arm/build: Warn on orphan section placement Nick Desaulniers @ 2020-06-24 19:43 ` Kees Cook 2020-06-26 21:36 ` Nick Desaulniers 1 sibling, 0 replies; 5+ messages in thread From: Kees Cook @ 2020-06-24 19:43 UTC (permalink / raw) To: Nick Desaulniers Cc: Arnd Bergmann, Masahiro Yamada, Eli Friedman, Russell King, LKML, Nathan Chancellor, Will Deacon, Ard Biesheuvel, Linux ARM 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! -- Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] arm/build: Warn on orphan section placement 2020-06-24 0:03 ` [PATCH v2 1/2] arm/build: Warn on orphan section placement Nick Desaulniers 2020-06-24 19:43 ` Kees Cook @ 2020-06-26 21:36 ` Nick Desaulniers 2020-06-26 21:55 ` Kees Cook 1 sibling, 1 reply; 5+ messages in thread From: Nick Desaulniers @ 2020-06-26 21:36 UTC (permalink / raw) To: Kees Cook Cc: Arnd Bergmann, Masahiro Yamada, Eli Friedman, Russell King, LKML, Nathan Chancellor, Will Deacon, Ard Biesheuvel, Linux ARM 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] arm/build: Warn on orphan section placement 2020-06-26 21:36 ` Nick Desaulniers @ 2020-06-26 21:55 ` Kees Cook 0 siblings, 0 replies; 5+ messages in thread From: Kees Cook @ 2020-06-26 21:55 UTC (permalink / raw) To: Nick Desaulniers Cc: Arnd Bergmann, Masahiro Yamada, Eli Friedman, Russell King, LKML, Nathan Chancellor, Will Deacon, Ard Biesheuvel, Linux ARM 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! -- Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20200622204915.2987555-3-keescook@chromium.org>]
* Re: [PATCH v2 2/2] arm/boot: Warn on orphan section placement [not found] ` <20200622204915.2987555-3-keescook@chromium.org> @ 2020-06-24 0:08 ` Nick Desaulniers 0 siblings, 0 replies; 5+ messages in thread From: Nick Desaulniers @ 2020-06-24 0:08 UTC (permalink / raw) To: Kees Cook Cc: Arnd Bergmann, Masahiro Yamada, Russell King, LKML, Nathan Chancellor, Will Deacon, Ard Biesheuvel, Linux ARM 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. > > 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.*) .rel.* is the only case I'm curious about. Why do we want it in vmlinux, but not the compressed image? Should `.rel.*` just be part of ARM_COMMON_DISCARD from the previous patch? > /* > * 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 > -- Thanks, ~Nick Desaulniers _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-26 21:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200622204915.2987555-1-keescook@chromium.org>
[not found] ` <20200622204915.2987555-2-keescook@chromium.org>
2020-06-24 0:03 ` [PATCH v2 1/2] arm/build: Warn on orphan section placement Nick Desaulniers
2020-06-24 19:43 ` Kees Cook
2020-06-26 21:36 ` Nick Desaulniers
2020-06-26 21:55 ` Kees Cook
[not found] ` <20200622204915.2987555-3-keescook@chromium.org>
2020-06-24 0:08 ` [PATCH v2 2/2] arm/boot: " Nick Desaulniers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox