[PATCH v2 0/2] arm64: Warn on orphan section placement

[PATCH v2 0/2] arm64: Warn on orphan section placement

[Kees Cook]

v2:
- split by architecture, rebase to v5.8-rc2
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 arm64.

This series needs one additional commit that is not yet in
any tree, but I hope to have it landed via x86 -tip shortly:
https://lore.kernel.org/lkml/20200622205341.2987797-2-keescook@chromium.org

Thanks!

-Kees

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

Kees Cook (2):
  arm64/build: Use common DISCARDS in linker script
  arm64/build: Warn on orphan section placement

 arch/arm64/Makefile             |  4 ++++
 arch/arm64/kernel/vmlinux.lds.S | 10 ++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] 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 v2 2/2] 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.

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.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm64/Makefile             | 4 ++++
 arch/arm64/kernel/vmlinux.lds.S | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

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)
 	}
 
 	. = 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
 
 	HEAD_SYMBOLS
 }
-- 
2.25.1

Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement

[Will Deacon]

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

Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement

[Ard Biesheuvel]

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

Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement

[Nick Desaulniers]

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

Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement

[Kees Cook]

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

Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement

[Ard Biesheuvel]

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

Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement

[Kees Cook]

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