Re: [PATCH v5 23/36] arm/build: Explicitly keep .ARM.attributes sections

From: Fangrui Song <maskray@google.com>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kees Cook <keescook@chromium.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Peter Collingbourne <pcc@google.com>,
	James Morse <james.morse@arm.com>, Borislav Petkov <bp@suse.de>,
	Ingo Molnar <mingo@redhat.com>,
	Russell King <linux@armlinux.org.uk>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Arvind Sankar <nivedita@alum.mit.edu>,
	Nathan Chancellor <natechancellor@gmail.com>,
	Arnd Bergmann <arnd@arndb.de>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
	<x86@kernel.org>,
	clang-built-linux <clang-built-linux@googlegroups.com>,
	linux-arch <linux-arch@vger.kernel.org>,
	linux-efi <linux-efi@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 23/36] arm/build: Explicitly keep .ARM.attributes sections
Date: Mon, 17 Aug 2020 15:06:29 -0700	[thread overview]
Message-ID: <20200817220629.3pkabegeedomsaaz@google.com> (raw)
In-Reply-To: <CAKwvOdn11z+iFQZC54JvQHC=NFX1FsoRMw2a-2P=5sQ6FKwbnw@mail.gmail.com>

On 2020-08-03, 'Nick Desaulniers' via Clang Built Linux wrote:
>On Fri, Jul 31, 2020 at 4:18 PM Kees Cook <keescook@chromium.org> wrote:
>>
>> In preparation for adding --orphan-handling=warn, explicitly keep the
>> .ARM.attributes section by expanding the existing ELF_DETAILS macro into
>> ARM_DETAILS.
>>
>> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
>> Link: https://lore.kernel.org/lkml/CAKwvOdk-racgq5pxsoGS6Vtifbtrk5fmkmnoLxrQMaOvV0nPWw@mail.gmail.com/
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  arch/arm/include/asm/vmlinux.lds.h | 4 ++++
>>  arch/arm/kernel/vmlinux-xip.lds.S  | 2 +-
>>  arch/arm/kernel/vmlinux.lds.S      | 2 +-
>>  3 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/vmlinux.lds.h b/arch/arm/include/asm/vmlinux.lds.h
>> index a08f4301b718..c4af5182ab48 100644
>> --- a/arch/arm/include/asm/vmlinux.lds.h
>> +++ b/arch/arm/include/asm/vmlinux.lds.h
>> @@ -52,6 +52,10 @@
>>                 ARM_MMU_DISCARD(*(__ex_table))                          \
>>                 COMMON_DISCARDS
>>
>> +#define ARM_DETAILS                                                    \
>> +               ELF_DETAILS                                             \
>> +               .ARM.attributes 0 : { *(.ARM.attributes) }
>
>I had to look up what the `0` meant:
>https://sourceware.org/binutils/docs/ld/Output-Section-Attributes.html#Output-Section-Attributes
>mentions it's an "address" and
>https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_chapter/ld_3.html#SEC21
>mentions it as "start" (an address).
>Unless we need those, can we drop them? (Sorry for the resulting churn
>that would cause).  I think the NO_LOAD stuff makes more sense, but
>I'm curious if the kernel checks for that.

NOLOAD means SHT_NOBITS (usually SHF_ALLOC). .ARM.attributes is a
non-SHF_ALLOC section.

An explicit 0 (output section address) is good - GNU ld's internal
linker scripts (ld --verbose output) use 0 for such non-SHF_ALLOC sections.
Without the 0, the section may get a non-zero address, which is not
wrong - but probably does not look well. See https://reviews.llvm.org/D85867 for details.

Reviewed-by: Fangrui Song <maskray@google.com>

>> +
>>  #define ARM_STUBS_TEXT                                                 \
>>                 *(.gnu.warning)                                         \
>>                 *(.glue_7)                                              \
>> diff --git a/arch/arm/kernel/vmlinux-xip.lds.S b/arch/arm/kernel/vmlinux-xip.lds.S
>> index 904c31fa20ed..57fcbf55f913 100644
>> --- a/arch/arm/kernel/vmlinux-xip.lds.S
>> +++ b/arch/arm/kernel/vmlinux-xip.lds.S
>> @@ -150,7 +150,7 @@ SECTIONS
>>         _end = .;
>>
>>         STABS_DEBUG
>> -       ELF_DETAILS
>> +       ARM_DETAILS
>>  }
>>
>>  /*
>> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
>> index bb950c896a67..1d3d3b599635 100644
>> --- a/arch/arm/kernel/vmlinux.lds.S
>> +++ b/arch/arm/kernel/vmlinux.lds.S
>> @@ -149,7 +149,7 @@ SECTIONS
>>         _end = .;
>>
>>         STABS_DEBUG
>> -       ELF_DETAILS
>> +       ARM_DETAILS
>>  }
>>
>>  #ifdef CONFIG_STRICT_KERNEL_RWX
>> --
>> 2.25.1
>>