From: Kees Cook <keescook@chromium.org>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
Arnd Bergmann <arnd@arndb.de>,
Peter Collingbourne <pcc@google.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Nick Desaulniers <ndesaulniers@google.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
clang-built-linux <clang-built-linux@googlegroups.com>,
James Morse <james.morse@arm.com>,
Nathan Chancellor <natechancellor@gmail.com>,
Will Deacon <will@kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement
Date: Tue, 23 Jun 2020 17:05:48 -0700 [thread overview]
Message-ID: <202006231704.EBCD26CE7@keescook> (raw)
In-Reply-To: <CAMj1kXGnm=uujNfNGJzQxd7BSF0qT2n6jPX1OqwnGov1nKC_cg@mail.gmail.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Kees Cook <keescook@chromium.org>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Will Deacon <will@kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Mark Rutland <mark.rutland@arm.com>,
Arnd Bergmann <arnd@arndb.de>,
Peter Collingbourne <pcc@google.com>,
James Morse <james.morse@arm.com>,
Nick Desaulniers <ndesaulniers@google.com>,
Nathan Chancellor <natechancellor@gmail.com>,
clang-built-linux <clang-built-linux@googlegroups.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] arm64/build: Warn on orphan section placement
Date: Tue, 23 Jun 2020 17:05:48 -0700 [thread overview]
Message-ID: <202006231704.EBCD26CE7@keescook> (raw)
In-Reply-To: <CAMj1kXGnm=uujNfNGJzQxd7BSF0qT2n6jPX1OqwnGov1nKC_cg@mail.gmail.com>
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
next prev parent reply other threads:[~2020-06-24 0:07 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-22 20:58 [PATCH v2 0/2] arm64: Warn on orphan section placement Kees Cook
2020-06-22 20:58 ` [PATCH v2 1/2] arm64/build: Use common DISCARDS in linker script Kees Cook
2020-06-22 20:58 ` [PATCH v2 2/2] arm64/build: Warn on orphan section placement Kees Cook
2020-06-23 14:52 ` Will Deacon
2020-06-23 14:52 ` Will Deacon
2020-06-23 14:59 ` Ard Biesheuvel
2020-06-23 14:59 ` Ard Biesheuvel
2020-06-23 19:18 ` Nick Desaulniers
2020-06-23 19:18 ` Nick Desaulniers
2020-06-23 21:06 ` Kees Cook
2020-06-23 21:06 ` Kees Cook
2020-06-23 21:21 ` Ard Biesheuvel
2020-06-23 21:21 ` Ard Biesheuvel
2020-06-24 0:05 ` Kees Cook [this message]
2020-06-24 0:05 ` Kees Cook
2020-06-24 3:52 ` kernel test robot
2020-06-24 4:41 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202006231704.EBCD26CE7@keescook \
--to=keescook@chromium.org \
--cc=ardb@kernel.org \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=clang-built-linux@googlegroups.com \
--cc=james.morse@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=natechancellor@gmail.com \
--cc=ndesaulniers@google.com \
--cc=pcc@google.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.