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

* 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