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

From: Mark Rutland
Date: Thu Jan 14 2016 - 05:47:28 EST

Next message: Andrey Ryabinin: "Re: [PATCH v2] kernel: add kcov code coverage"
Previous message: Peter Zijlstra: "Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users"
In reply to: Ard Biesheuvel: "Re: [PATCH v3 11/21] arm64: avoid R_AARCH64_ABS64 relocations for Image header fields"
Next in thread: Ard Biesheuvel: "Re: [PATCH v3 11/21] arm64: avoid R_AARCH64_ABS64 relocations for Image header fields"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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);

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
...

Thanks,
Mark.

>
> --
> 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: Andrey Ryabinin: "Re: [PATCH v2] kernel: add kcov code coverage"
Previous message: Peter Zijlstra: "Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users"
In reply to: Ard Biesheuvel: "Re: [PATCH v3 11/21] arm64: avoid R_AARCH64_ABS64 relocations for Image header fields"
Next in thread: Ard Biesheuvel: "Re: [PATCH v3 11/21] arm64: avoid R_AARCH64_ABS64 relocations for Image header fields"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]