[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 14 Jan 2016 10:05:42 +0100
From: Ard Biesheuvel <ard.biesheuvel@...aro.org>
To: Mark Rutland <mark.rutland@....com>
Cc: "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
kernel-hardening@...ts.openwall.com, Will Deacon <will.deacon@....com>,
Catalin Marinas <catalin.marinas@....com>, Leif Lindholm <leif.lindholm@...aro.org>,
Kees Cook <keescook@...omium.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Stuart Yoder <stuart.yoder@...escale.com>,
Sharma Bhupesh <bhupesh.sharma@...escale.com>, Arnd Bergmann <arnd@...db.de>,
Marc Zyngier <marc.zyngier@....com>, Christoffer Dall <christoffer.dall@...aro.org>
Subject: Re: [PATCH v3 11/21] arm64: avoid R_AARCH64_ABS64 relocations for
Image header fields
On 14 January 2016 at 09:51, Ard Biesheuvel <ard.biesheuvel@...aro.org> wrote:
> On 13 January 2016 at 19:48, Ard Biesheuvel <ard.biesheuvel@...aro.org> wrote:
>> On 13 January 2016 at 19:12, Mark Rutland <mark.rutland@....com> wrote:
>>> On Mon, Jan 11, 2016 at 02:19:04PM +0100, Ard Biesheuvel wrote:
>>>> Unfortunately, the current way of using the linker to emit build time
>>>> constants into the Image header will no longer work once we switch to
>>>> the use of PIE executables. The reason is that such constants are emitted
>>>> into the binary using R_AARCH64_ABS64 relocations, which we will resolve
>>>> at runtime, not at build time, and the places targeted by those
>>>> relocations will contain zeroes before that.
>>>>
>>>> So move back to assembly time constants or R_AARCH64_ABS32 relocations
>>>> (which, interestingly enough, do get resolved at build time)
>>>
>>> To me it seems very odd that ABS64 and ABS32 are treated differently,
>>> and it makes me somewhat uncomfortable becuase it feels like a bug.
>>>
>>> Do we know whether the inconsistency between ABS64 and ABS32 was
>>> deliberate?
>>>
>>> I couldn't spot anything declaring a difference in the AArch64 ELF
>>> spec, and I'm not sure where else to look.
>>>
>>
>> My assumption is that PIE only defers resolving R_AARCH64_ABS64
>> relocations since those are the only ones that can be used to refer to
>> memory addresses
>>
>
> OK, digging into the binutils source code, it turns out that indeed,
> ABSnn relocations where nn equals the ELFnn memory size are treated
> differently, but only if they have default visibility. This is simply
> a result of the fact the code path is shared between shared libraries
> and PIE executables, since PIE executable are fully linked. It also
> means that we can simply work around it by emitting the linker symbols
> as hidden.
>
... and the bad news is that, while emitting the symbols as hidden
turns them from R_AARCH64_ABS64 into a R_AARCH64_RELATIVE relocations,
it does not actually force the value to be emitted at build time.
So I am going to stick with the patch, but elaborate in a comment
about why R_AARCH64_ABSnn are treated differently if nn equals the
pointer size. (look at elfNN_aarch64_final_link_relocate() in binutils
if you are keen to look at the code yourself)
--
Ard.
>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>
>>>> ---
>>>> arch/arm64/include/asm/assembler.h | 15 ++++++++
>>>> arch/arm64/kernel/head.S | 17 +++++++--
>>>> arch/arm64/kernel/image.h | 37 ++++++--------------
>>>> 3 files changed, 40 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>>>> index d8bfcc1ce923..e211af783a3d 100644
>>>> --- a/arch/arm64/include/asm/assembler.h
>>>> +++ b/arch/arm64/include/asm/assembler.h
>>>> @@ -222,4 +222,19 @@ lr .req x30 // link register
>>>> .size __pi_##x, . - x; \
>>>> ENDPROC(x)
>>>>
>>>> + .macro le16, val
>>>> + .byte \val & 0xff
>>>> + .byte (\val >> 8) & 0xff
>>>> + .endm
>>>> +
>>>> + .macro le32, val
>>>> + le16 \val
>>>> + le16 (\val >> 16)
>>>> + .endm
>>>> +
>>>> + .macro le64, val
>>>> + le32 \val
>>>> + le32 (\val >> 32)
>>>> + .endm
>>>> +
>>>> #endif /* __ASM_ASSEMBLER_H */
>>>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>>>> index 350515276541..211f75e673f4 100644
>>>> --- a/arch/arm64/kernel/head.S
>>>> +++ b/arch/arm64/kernel/head.S
>>>> @@ -51,6 +51,17 @@
>>>> #define KERNEL_START _text
>>>> #define KERNEL_END _end
>>>>
>>>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>>>> +#define __HEAD_FLAG_BE 1
>>>> +#else
>>>> +#define __HEAD_FLAG_BE 0
>>>> +#endif
>>>> +
>>>> +#define __HEAD_FLAG_PAGE_SIZE ((PAGE_SHIFT - 10) / 2)
>>>> +
>>>> +#define __HEAD_FLAGS ((__HEAD_FLAG_BE << 0) | \
>>>> + (__HEAD_FLAG_PAGE_SIZE << 1))
>>>> +
>>>> /*
>>>> * Kernel startup entry point.
>>>> * ---------------------------
>>>> @@ -83,9 +94,9 @@ efi_head:
>>>> b stext // branch to kernel start, magic
>>>> .long 0 // reserved
>>>> #endif
>>>> - .quad _kernel_offset_le // Image load offset from start of RAM, little-endian
>>>> - .quad _kernel_size_le // Effective size of kernel image, little-endian
>>>> - .quad _kernel_flags_le // Informative flags, little-endian
>>>> + le64 TEXT_OFFSET // Image load offset from start of RAM, little-endian
>>>> + .long _kernel_size_le, 0 // Effective size of kernel image, little-endian
>>>> + le64 __HEAD_FLAGS // Informative flags, little-endian
>>>> .quad 0 // reserved
>>>> .quad 0 // reserved
>>>> .quad 0 // reserved
>>>> diff --git a/arch/arm64/kernel/image.h b/arch/arm64/kernel/image.h
>>>> index bc2abb8b1599..bb6b0e69d0a4 100644
>>>> --- a/arch/arm64/kernel/image.h
>>>> +++ b/arch/arm64/kernel/image.h
>>>> @@ -26,41 +26,26 @@
>>>> * There aren't any ELF relocations we can use to endian-swap values known only
>>>> * at link time (e.g. the subtraction of two symbol addresses), so we must get
>>>> * the linker to endian-swap certain values before emitting them.
>>>> + * Note that this will not work for 64-bit values: these are resolved using
>>>> + * R_AARCH64_ABS64 relocations, which are fixed up at runtime rather than at
>>>> + * build time when building the PIE executable (for KASLR).
>>>> */
>>>> #ifdef CONFIG_CPU_BIG_ENDIAN
>>>> -#define DATA_LE64(data) \
>>>> - ((((data) & 0x00000000000000ff) << 56) | \
>>>> - (((data) & 0x000000000000ff00) << 40) | \
>>>> - (((data) & 0x0000000000ff0000) << 24) | \
>>>> - (((data) & 0x00000000ff000000) << 8) | \
>>>> - (((data) & 0x000000ff00000000) >> 8) | \
>>>> - (((data) & 0x0000ff0000000000) >> 24) | \
>>>> - (((data) & 0x00ff000000000000) >> 40) | \
>>>> - (((data) & 0xff00000000000000) >> 56))
>>>> +#define DATA_LE32(data) \
>>>> + ((((data) & 0x000000ff) << 24) | \
>>>> + (((data) & 0x0000ff00) << 8) | \
>>>> + (((data) & 0x00ff0000) >> 8) | \
>>>> + (((data) & 0xff000000) >> 24))
>>>> #else
>>>> -#define DATA_LE64(data) ((data) & 0xffffffffffffffff)
>>>> +#define DATA_LE32(data) ((data) & 0xffffffff)
>>>> #endif
>>>>
>>>> -#ifdef CONFIG_CPU_BIG_ENDIAN
>>>> -#define __HEAD_FLAG_BE 1
>>>> -#else
>>>> -#define __HEAD_FLAG_BE 0
>>>> -#endif
>>>> -
>>>> -#define __HEAD_FLAG_PAGE_SIZE ((PAGE_SHIFT - 10) / 2)
>>>> -
>>>> -#define __HEAD_FLAGS ((__HEAD_FLAG_BE << 0) | \
>>>> - (__HEAD_FLAG_PAGE_SIZE << 1))
>>>> -
>>>> /*
>>>> * These will output as part of the Image header, which should be little-endian
>>>> - * regardless of the endianness of the kernel. While constant values could be
>>>> - * endian swapped in head.S, all are done here for consistency.
>>>> + * regardless of the endianness of the kernel.
>>>> */
>>>> #define HEAD_SYMBOLS \
>>>> - _kernel_size_le = DATA_LE64(_end - _text); \
>>>> - _kernel_offset_le = DATA_LE64(TEXT_OFFSET); \
>>>> - _kernel_flags_le = DATA_LE64(__HEAD_FLAGS);
>>>> + _kernel_size_le = DATA_LE32(_end - _text);
>>>>
>>>> #ifdef CONFIG_EFI
>>>>
>>>> --
>>>> 2.5.0
>>>>
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
