* Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement
[not found] ` <20200622205815.2988115-3-keescook@chromium.org>
@ 2020-06-23 14:52 ` Will Deacon
2020-06-23 14:59 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2020-06-23 14:52 UTC (permalink / raw)
To: Kees Cook
Cc: Mark Rutland, Arnd Bergmann, Catalin Marinas, Nick Desaulniers,
linux-kernel, clang-built-linux, James Morse, Nathan Chancellor,
Peter Collingbourne, Ard Biesheuvel, linux-arm-kernel
On Mon, Jun 22, 2020 at 01:58:15PM -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.
>
> Explicitly include debug sections when they're present. Add .eh_frame*
> to discard as it seems that these are still generated even though
> -fno-asynchronous-unwind-tables is being specified. Add .plt and
> .data.rel.ro to discards as they are not actually used. Add .got.plt
> to the image as it does appear to be mapped near .data. Finally enable
> orphan section warnings.
Can you elaborate a bit on what .got.plt is being used for, please? I
wonder if there's an interaction with an erratum workaround in the linker
or something.
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index a0d94d063fa8..3e628983445a 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)
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index 5427f502c3a6..c9ecb3b2007d 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)
> + *(.eh_frame) *(.init.eh_frame)
Do we need to include .eh_frame_hdr here too?
> }
>
> . = KIMAGE_VADDR + TEXT_OFFSET;
> @@ -209,6 +210,7 @@ SECTIONS
> _data = .;
> _sdata = .;
> RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_ALIGN)
> + .got.plt : ALIGN(8) { *(.got.plt) }
>
> /*
> * Data written with the MMU off but read with the MMU on requires
> @@ -244,6 +246,7 @@ SECTIONS
> _end = .;
>
> STABS_DEBUG
> + DWARF_DEBUG
I know you didn't add it, but do we _really_ care about stabs debug? Who
generates that for arm64?
Will
_______________________________________________
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] 6+ messages in thread
* Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement
2020-06-23 14:52 ` [PATCH v2 2/2] arm64/build: Warn on orphan section placement Will Deacon
@ 2020-06-23 14:59 ` Ard Biesheuvel
2020-06-23 19:18 ` Nick Desaulniers
2020-06-23 21:06 ` Kees Cook
0 siblings, 2 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2020-06-23 14:59 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Kees Cook, Arnd Bergmann, Catalin Marinas,
Nick Desaulniers, Linux Kernel Mailing List, clang-built-linux,
James Morse, Nathan Chancellor, Peter Collingbourne, Linux ARM
On Tue, 23 Jun 2020 at 16:52, Will Deacon <will@kernel.org> wrote:
>
> On Mon, Jun 22, 2020 at 01:58:15PM -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.
> >
> > Explicitly include debug sections when they're present. Add .eh_frame*
> > to discard as it seems that these are still generated even though
> > -fno-asynchronous-unwind-tables is being specified. Add .plt and
> > .data.rel.ro to discards as they are not actually used. Add .got.plt
> > to the image as it does appear to be mapped near .data. Finally enable
> > orphan section warnings.
>
> Can you elaborate a bit on what .got.plt is being used for, please? I
> wonder if there's an interaction with an erratum workaround in the linker
> or something.
>
.got.plt is not used at all, but it has three magic entries at the
start that the dynamic linker uses for lazy dispatch, so it turns up
as a non-empty section of 0x18 bytes.
We should be able to discard it afaict, but given that it does not
actually take up any space, it doesn't really matter either way.
> > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > index a0d94d063fa8..3e628983445a 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)
> > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > index 5427f502c3a6..c9ecb3b2007d 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)
> > + *(.eh_frame) *(.init.eh_frame)
>
> Do we need to include .eh_frame_hdr here too?
>
It would be better to build with -fno-unwind-tables, in which case
these sections should not even exist.
> > }
> >
> > . = KIMAGE_VADDR + TEXT_OFFSET;
> > @@ -209,6 +210,7 @@ SECTIONS
> > _data = .;
> > _sdata = .;
> > RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_ALIGN)
> > + .got.plt : ALIGN(8) { *(.got.plt) }
> >
> > /*
> > * Data written with the MMU off but read with the MMU on requires
> > @@ -244,6 +246,7 @@ SECTIONS
> > _end = .;
> >
> > STABS_DEBUG
> > + DWARF_DEBUG
>
> I know you didn't add it, but do we _really_ care about stabs debug? Who
> generates that for arm64?
>
> Will
_______________________________________________
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] 6+ messages in thread
* Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement
2020-06-23 14:59 ` Ard Biesheuvel
@ 2020-06-23 19:18 ` Nick Desaulniers
2020-06-23 21:06 ` Kees Cook
1 sibling, 0 replies; 6+ messages in thread
From: Nick Desaulniers @ 2020-06-23 19:18 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Mark Rutland, Kees Cook, Arnd Bergmann, Peter Collingbourne,
Catalin Marinas, Linux Kernel Mailing List, clang-built-linux,
James Morse, Nathan Chancellor, Will Deacon, Linux ARM
On Tue, Jun 23, 2020 at 7:59 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 23 Jun 2020 at 16:52, Will Deacon <will@kernel.org> wrote:
> >
> > On Mon, Jun 22, 2020 at 01:58:15PM -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.
> > >
> > > Explicitly include debug sections when they're present. Add .eh_frame*
> > > to discard as it seems that these are still generated even though
> > > -fno-asynchronous-unwind-tables is being specified. Add .plt and
> > > .data.rel.ro to discards as they are not actually used. Add .got.plt
> > > to the image as it does appear to be mapped near .data. Finally enable
> > > orphan section warnings.
> >
> > Can you elaborate a bit on what .got.plt is being used for, please? I
> > wonder if there's an interaction with an erratum workaround in the linker
> > or something.
> >
>
> .got.plt is not used at all, but it has three magic entries at the
> start that the dynamic linker uses for lazy dispatch, so it turns up
> as a non-empty section of 0x18 bytes.
Interesting; is there a way to dump those entries? `--dynamic-reloc`
flag to objdump? (I suspect the answer might be hexdump...)
> We should be able to discard it afaict, but given that it does not
> actually take up any space, it doesn't really matter either way.
True, but I would prefer to explicitly discard it if we know we're not
using it, that way something explicitly breaks if someone tries to
make use of it in the future. Then we can consider not discarding it,
only if necessary. Modules on arm64 use .got.plt, IIRC? But they have
their own linker script so irrelevant I guess.
> > > --- 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)
> > > + *(.eh_frame) *(.init.eh_frame)
> >
> > Do we need to include .eh_frame_hdr here too?
> >
>
> It would be better to build with -fno-unwind-tables, in which case
> these sections should not even exist.
Interesting, so we have -fno-asynchronous-unwind-tables and
-fno-unwind-tables. Is your suggestion for -fno-unwind-tables a
global KBUILD_CFLAG (vs limited to a particular arch)? Interestingly,
there a few users of -fasynchronous-unwind-tables in the kernel.
vdso's make sense, I think, less sure about the rest.
--
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] 6+ messages in thread
* Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement
2020-06-23 14:59 ` Ard Biesheuvel
2020-06-23 19:18 ` Nick Desaulniers
@ 2020-06-23 21:06 ` Kees Cook
2020-06-23 21:21 ` Ard Biesheuvel
1 sibling, 1 reply; 6+ messages in thread
From: Kees Cook @ 2020-06-23 21:06 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Mark Rutland, Arnd Bergmann, Peter Collingbourne, Catalin Marinas,
Nick Desaulniers, Linux Kernel Mailing List, clang-built-linux,
James Morse, Nathan Chancellor, Will Deacon, Linux ARM
On Tue, Jun 23, 2020 at 04:59:39PM +0200, Ard Biesheuvel wrote:
> On Tue, 23 Jun 2020 at 16:52, Will Deacon <will@kernel.org> wrote:
> >
> > On Mon, Jun 22, 2020 at 01:58:15PM -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.
> > >
> > > Explicitly include debug sections when they're present. Add .eh_frame*
> > > to discard as it seems that these are still generated even though
> > > -fno-asynchronous-unwind-tables is being specified. Add .plt and
> > > .data.rel.ro to discards as they are not actually used. Add .got.plt
> > > to the image as it does appear to be mapped near .data. Finally enable
> > > orphan section warnings.
> >
> > Can you elaborate a bit on what .got.plt is being used for, please? I
> > wonder if there's an interaction with an erratum workaround in the linker
> > or something.
> >
>
> .got.plt is not used at all, but it has three magic entries at the
> start that the dynamic linker uses for lazy dispatch, so it turns up
> as a non-empty section of 0x18 bytes.
Is there a way to suppress the generation? I haven't found a way, so I'd
left it as-is.
> We should be able to discard it afaict, but given that it does not
> actually take up any space, it doesn't really matter either way.
I will add it to the discards then.
> > > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > > index a0d94d063fa8..3e628983445a 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)
> > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > > index 5427f502c3a6..c9ecb3b2007d 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)
> > > + *(.eh_frame) *(.init.eh_frame)
> >
> > Do we need to include .eh_frame_hdr here too?
>
> It would be better to build with -fno-unwind-tables, in which case
> these sections should not even exist.
Nothing seems to help with the .eh_frame issue
(even with -fno-asynchronous-unwind-tables and -fno-unwind-tables):
$ aarch64-linux-gnu-gcc -Wp,-MMD,arch/arm64/kernel/.smccc-call.o.d \
-nostdinc -isystem /usr/lib/gcc-cross/aarch64-linux-gnu/9/include \
-I./arch/arm64/include -I./arch/arm64/include/generated -I./include \
-I./arch/arm64/include/uapi -I./arch/arm64/include/generated/uapi \
-I./include/uapi -I./include/generated/uapi -include \
./include/linux/kconfig.h -D__KERNEL__ -mlittle-endian \
-DCC_USING_PATCHABLE_FUNCTION_ENTRY -DKASAN_SHADOW_SCALE_SHIFT=3 \
-D__ASSEMBLY__ -fno-PIE -mabi=lp64 -fno-asynchronous-unwind-tables \
-fno-unwind-tables -DKASAN_SHADOW_SCALE_SHIFT=3 -Wa,-gdwarf-2 -c -o \
arch/arm64/kernel/smccc-call.o arch/arm64/kernel/smccc-call.S
$ readelf -S arch/arm64/kernel/smccc-call.o | grep eh_frame
[17] .eh_frame PROGBITS 0000000000000000 000001f0
[18] .rela.eh_frame RELA 0000000000000000 00000618
> > > }
> > >
> > > . = KIMAGE_VADDR + TEXT_OFFSET;
> > > @@ -209,6 +210,7 @@ SECTIONS
> > > _data = .;
> > > _sdata = .;
> > > RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_ALIGN)
> > > + .got.plt : ALIGN(8) { *(.got.plt) }
> > >
> > > /*
> > > * Data written with the MMU off but read with the MMU on requires
> > > @@ -244,6 +246,7 @@ SECTIONS
> > > _end = .;
> > >
> > > STABS_DEBUG
> > > + DWARF_DEBUG
> >
> > I know you didn't add it, but do we _really_ care about stabs debug? Who
> > generates that for arm64?
It's also where .comment and the .symtab ends up living. As a result,
I think it's correct to keep it. (If we wanted to split .stabs from
.comment/.symtab, we could do that, but I'd kind of like to avoid it for
this series, as it feels kind of unrelated.)
--
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] 6+ messages in thread
* Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement
2020-06-23 21:06 ` Kees Cook
@ 2020-06-23 21:21 ` Ard Biesheuvel
2020-06-24 0:05 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2020-06-23 21:21 UTC (permalink / raw)
To: Kees Cook
Cc: Mark Rutland, Arnd Bergmann, Peter Collingbourne, Catalin Marinas,
Nick Desaulniers, Linux Kernel Mailing List, clang-built-linux,
James Morse, Nathan Chancellor, Will Deacon, Linux ARM
On Tue, 23 Jun 2020 at 23:06, Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jun 23, 2020 at 04:59:39PM +0200, Ard Biesheuvel wrote:
> > On Tue, 23 Jun 2020 at 16:52, Will Deacon <will@kernel.org> wrote:
> > >
> > > On Mon, Jun 22, 2020 at 01:58:15PM -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.
> > > >
> > > > Explicitly include debug sections when they're present. Add .eh_frame*
> > > > to discard as it seems that these are still generated even though
> > > > -fno-asynchronous-unwind-tables is being specified. Add .plt and
> > > > .data.rel.ro to discards as they are not actually used. Add .got.plt
> > > > to the image as it does appear to be mapped near .data. Finally enable
> > > > orphan section warnings.
> > >
> > > Can you elaborate a bit on what .got.plt is being used for, please? I
> > > wonder if there's an interaction with an erratum workaround in the linker
> > > or something.
> > >
> >
> > .got.plt is not used at all, but it has three magic entries at the
> > start that the dynamic linker uses for lazy dispatch, so it turns up
> > as a non-empty section of 0x18 bytes.
>
> Is there a way to suppress the generation? I haven't found a way, so I'd
> left it as-is.
>
Not really. What we could do is assert that it is empty, and emit it
as info, so the 24 bytes are not emitted into the image.
This change
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 6827da7f3aa5..9e13b371559f 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -244,6 +244,9 @@ SECTIONS
__pecoff_data_size = ABSOLUTE(. - __initdata_begin);
_end = .;
+ .got.plt (INFO) : { *(.got.plt) }
+ ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18,
".got.plt not empty")
+
STABS_DEBUG
HEAD_SYMBOLS
results in
[28] .bss NOBITS ffff800010d71000 00d70200
0000000000084120 0000000000000000 WA 0 0 4096
[29] .got.plt PROGBITS ffff800010e00000 00d70200
0000000000000018 0000000000000008 W 0 0 8
[30] .comment PROGBITS 0000000000000000 00d70218
000000000000001c 0000000000000001 MS 0 0 1
in the ELF output, so it will be emitted from the image, unless it
actually have any entries, in which case we fail the build.
> > We should be able to discard it afaict, but given that it does not
> > actually take up any space, it doesn't really matter either way.
>
> I will add it to the discards then.
>
That would prevent us from doing the assert on its size, so i think
the above is more suitable in this case
> > > > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > > > index a0d94d063fa8..3e628983445a 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)
> > > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > > > index 5427f502c3a6..c9ecb3b2007d 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)
> > > > + *(.eh_frame) *(.init.eh_frame)
> > >
> > > Do we need to include .eh_frame_hdr here too?
> >
> > It would be better to build with -fno-unwind-tables, in which case
> > these sections should not even exist.
>
> Nothing seems to help with the .eh_frame issue
> (even with -fno-asynchronous-unwind-tables and -fno-unwind-tables):
>
> $ aarch64-linux-gnu-gcc -Wp,-MMD,arch/arm64/kernel/.smccc-call.o.d \
> -nostdinc -isystem /usr/lib/gcc-cross/aarch64-linux-gnu/9/include \
> -I./arch/arm64/include -I./arch/arm64/include/generated -I./include \
> -I./arch/arm64/include/uapi -I./arch/arm64/include/generated/uapi \
> -I./include/uapi -I./include/generated/uapi -include \
> ./include/linux/kconfig.h -D__KERNEL__ -mlittle-endian \
> -DCC_USING_PATCHABLE_FUNCTION_ENTRY -DKASAN_SHADOW_SCALE_SHIFT=3 \
> -D__ASSEMBLY__ -fno-PIE -mabi=lp64 -fno-asynchronous-unwind-tables \
> -fno-unwind-tables -DKASAN_SHADOW_SCALE_SHIFT=3 -Wa,-gdwarf-2 -c -o \
> arch/arm64/kernel/smccc-call.o arch/arm64/kernel/smccc-call.S
>
> $ readelf -S arch/arm64/kernel/smccc-call.o | grep eh_frame
> [17] .eh_frame PROGBITS 0000000000000000 000001f0
> [18] .rela.eh_frame RELA 0000000000000000 00000618
>
That is because that file has CFI annotations which it doesn't need
(since we don't unwind data).
The below should fix that - I guess this may have been inherited from
32-bit ARM, where we do use unwind data in the kernel?
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
/*
> > > > }
> > > >
> > > > . = KIMAGE_VADDR + TEXT_OFFSET;
> > > > @@ -209,6 +210,7 @@ SECTIONS
> > > > _data = .;
> > > > _sdata = .;
> > > > RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_ALIGN)
> > > > + .got.plt : ALIGN(8) { *(.got.plt) }
> > > >
> > > > /*
> > > > * Data written with the MMU off but read with the MMU on requires
> > > > @@ -244,6 +246,7 @@ SECTIONS
> > > > _end = .;
> > > >
> > > > STABS_DEBUG
> > > > + DWARF_DEBUG
> > >
> > > I know you didn't add it, but do we _really_ care about stabs debug? Who
> > > generates that for arm64?
>
> It's also where .comment and the .symtab ends up living. As a result,
> I think it's correct to keep it. (If we wanted to split .stabs from
> .comment/.symtab, we could do that, but I'd kind of like to avoid it for
> this series, as it feels kind of unrelated.)
>
> --
> 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 related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement
2020-06-23 21:21 ` Ard Biesheuvel
@ 2020-06-24 0:05 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2020-06-24 0:05 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Mark Rutland, Arnd Bergmann, Peter Collingbourne, Catalin Marinas,
Nick Desaulniers, Linux Kernel Mailing List, clang-built-linux,
James Morse, Nathan Chancellor, Will Deacon, Linux ARM
On Tue, Jun 23, 2020 at 11:21:16PM +0200, Ard Biesheuvel wrote:
> On Tue, 23 Jun 2020 at 23:06, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, Jun 23, 2020 at 04:59:39PM +0200, Ard Biesheuvel wrote:
> > > On Tue, 23 Jun 2020 at 16:52, Will Deacon <will@kernel.org> wrote:
> > > >
> > > > On Mon, Jun 22, 2020 at 01:58:15PM -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.
> > > > >
> > > > > Explicitly include debug sections when they're present. Add .eh_frame*
> > > > > to discard as it seems that these are still generated even though
> > > > > -fno-asynchronous-unwind-tables is being specified. Add .plt and
> > > > > .data.rel.ro to discards as they are not actually used. Add .got.plt
> > > > > to the image as it does appear to be mapped near .data. Finally enable
> > > > > orphan section warnings.
> > > >
> > > > Can you elaborate a bit on what .got.plt is being used for, please? I
> > > > wonder if there's an interaction with an erratum workaround in the linker
> > > > or something.
> > > >
> > >
> > > .got.plt is not used at all, but it has three magic entries at the
> > > start that the dynamic linker uses for lazy dispatch, so it turns up
> > > as a non-empty section of 0x18 bytes.
> >
> > Is there a way to suppress the generation? I haven't found a way, so I'd
> > left it as-is.
> >
>
> Not really. What we could do is assert that it is empty, and emit it
> as info, so the 24 bytes are not emitted into the image.
>
>
> This change
>
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index 6827da7f3aa5..9e13b371559f 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -244,6 +244,9 @@ SECTIONS
> __pecoff_data_size = ABSOLUTE(. - __initdata_begin);
> _end = .;
>
> + .got.plt (INFO) : { *(.got.plt) }
> + ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18,
> ".got.plt not empty")
> +
Oh yes, I like that. I will do so.
> STABS_DEBUG
>
> HEAD_SYMBOLS
>
> results in
>
> [28] .bss NOBITS ffff800010d71000 00d70200
> 0000000000084120 0000000000000000 WA 0 0 4096
> [29] .got.plt PROGBITS ffff800010e00000 00d70200
> 0000000000000018 0000000000000008 W 0 0 8
> [30] .comment PROGBITS 0000000000000000 00d70218
> 000000000000001c 0000000000000001 MS 0 0 1
>
> in the ELF output, so it will be emitted from the image, unless it
> actually have any entries, in which case we fail the build.
>
>
>
> > > We should be able to discard it afaict, but given that it does not
> > > actually take up any space, it doesn't really matter either way.
> >
> > I will add it to the discards then.
> >
>
> That would prevent us from doing the assert on its size, so i think
> the above is more suitable in this case
>
> > > > > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > > > > index a0d94d063fa8..3e628983445a 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)
> > > > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > > > > index 5427f502c3a6..c9ecb3b2007d 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)
> > > > > + *(.eh_frame) *(.init.eh_frame)
> > > >
> > > > Do we need to include .eh_frame_hdr here too?
> > >
> > > It would be better to build with -fno-unwind-tables, in which case
> > > these sections should not even exist.
> >
> > Nothing seems to help with the .eh_frame issue
> > (even with -fno-asynchronous-unwind-tables and -fno-unwind-tables):
> >
> > $ aarch64-linux-gnu-gcc -Wp,-MMD,arch/arm64/kernel/.smccc-call.o.d \
> > -nostdinc -isystem /usr/lib/gcc-cross/aarch64-linux-gnu/9/include \
> > -I./arch/arm64/include -I./arch/arm64/include/generated -I./include \
> > -I./arch/arm64/include/uapi -I./arch/arm64/include/generated/uapi \
> > -I./include/uapi -I./include/generated/uapi -include \
> > ./include/linux/kconfig.h -D__KERNEL__ -mlittle-endian \
> > -DCC_USING_PATCHABLE_FUNCTION_ENTRY -DKASAN_SHADOW_SCALE_SHIFT=3 \
> > -D__ASSEMBLY__ -fno-PIE -mabi=lp64 -fno-asynchronous-unwind-tables \
> > -fno-unwind-tables -DKASAN_SHADOW_SCALE_SHIFT=3 -Wa,-gdwarf-2 -c -o \
> > arch/arm64/kernel/smccc-call.o arch/arm64/kernel/smccc-call.S
> >
> > $ readelf -S arch/arm64/kernel/smccc-call.o | grep eh_frame
> > [17] .eh_frame PROGBITS 0000000000000000 000001f0
> > [18] .rela.eh_frame RELA 0000000000000000 00000618
> >
>
> That is because that file has CFI annotations which it doesn't need
> (since we don't unwind data).
Oh no, another TLA collision. ;) "Call Frame Information". Nice find. I
will fix this as you've suggested too.
> The below should fix that - I guess this may have been inherited from
> 32-bit ARM, where we do use unwind data in the kernel?
>
> 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
--
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] 6+ messages in thread
end of thread, other threads:[~2020-06-24 0:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200622205815.2988115-1-keescook@chromium.org>
[not found] ` <20200622205815.2988115-3-keescook@chromium.org>
2020-06-23 14:52 ` [PATCH v2 2/2] arm64/build: Warn on orphan section placement Will Deacon
2020-06-23 14:59 ` Ard Biesheuvel
2020-06-23 19:18 ` Nick Desaulniers
2020-06-23 21:06 ` Kees Cook
2020-06-23 21:21 ` Ard Biesheuvel
2020-06-24 0:05 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).