Re: [PATCH v3 11/21] arm64: avoid R_AARCH64_ABS64 relocations for Image header fields

From: Ard Biesheuvel
Date: Thu Jan 14 2016 - 06:22:50 EST

Next message: Ingo Molnar: "Re: [PATCH v2] reboot: Backup orderly_poweroff"
Previous message: Martin Kepplinger: "Re: [PATCH] iio: mma8452: add support for MMA8451Q"
In reply to: Mark Rutland: "Re: [PATCH v3 11/21] arm64: avoid R_AARCH64_ABS64 relocations for Image header fields"
Next in thread: Ard Biesheuvel: "[PATCH v3 08/21] arm64: add support for module PLTs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 14 January 2016 at 11:46, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Thu, Jan 14, 2016 at 10:05:42AM +0100, Ard Biesheuvel wrote:
>> On 14 January 2016 at 09:51, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>> > On 13 January 2016 at 19:48, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>> >> On 13 January 2016 at 19:12, Mark Rutland <mark.rutland@xxxxxxx> 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)
>
> Ok. Thanks for digging into that.
>
> One thing though: I would prefer if we could still keep all the LE64
> image header values together, to have them dealt with consistently.
>
> Could we hide the ABS32 usage behind some macros to do so, e.g.
>
> in image.h:
>
> #define DEFINE_IMAGE_LE64(sym, data) \
> sym##_lo32 = DATA_LE32(data & 0xffffffff); \
> sym##_hi32 = DATA_LE32(data >> 32);
>
> #define HEAD_SYMBOLS \
> DEFINE_IMAGE_LE64(_kernel_size_le, _end - _text); \
> DEFINE_IMAGE_LE64(_kernel_offset_le, TEXT_OFFSET); \
> DEFINE_IMAGE_LE64(_kernel_flags_le, __HEAD_FLAGS);
>

I will steal this

> and in head.S:
>
> #define IMAGE_LE64(sym) .long sym##_lo32, sym##_hi32
>
> ...
> IMAGE_LE64(_kernel_size_le) // Image load offset from start of RAM, little-endian
> IMAGE_LE64(_kernel_offset_le) // Effective size of kernel image, little-endian
> IMAGE_LE64(_kernel_flags_le) // Informative flags, little-endian
> ...

... and implement this with an asm macro.

Thanks,
Ard.

>>
>> --
>> Ard.
>>
>>
>>
>> >
>> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> >>>> ---
>> >>>> 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
>> >>>>
>>

Next message: Ingo Molnar: "Re: [PATCH v2] reboot: Backup orderly_poweroff"
Previous message: Martin Kepplinger: "Re: [PATCH] iio: mma8452: add support for MMA8451Q"
In reply to: Mark Rutland: "Re: [PATCH v3 11/21] arm64: avoid R_AARCH64_ABS64 relocations for Image header fields"
Next in thread: Ard Biesheuvel: "[PATCH v3 08/21] arm64: add support for module PLTs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]