[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170814152915.GS6321@e103592.cambridge.arm.com>
Date: Mon, 14 Aug 2017 16:29:15 +0100
From: Dave Martin <Dave.Martin@....com>
To: Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc: Mark Rutland <mark.rutland@....com>, Kees Cook <keescook@...omium.org>,
Arnd Bergmann <arnd@...db.de>, Nicolas Pitre <nico@...aro.org>,
Marc Zyngier <marc.zyngier@....com>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>,
Russell King <linux@...linux.org.uk>,
Tony Lindgren <tony@...mide.com>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
Thomas Garnier <thgarnie@...gle.com>,
Matt Fleming <matt@...eblueprint.co.uk>
Subject: Re: [PATCH 17/30] arm-soc: tegra: make sleep asm code runtime
relocatable
On Mon, Aug 14, 2017 at 03:49:15PM +0100, Ard Biesheuvel wrote:
> On 14 August 2017 at 15:42, Dave Martin <Dave.Martin@....com> wrote:
> > On Mon, Aug 14, 2017 at 01:53:58PM +0100, Ard Biesheuvel wrote:
> >> The PIE kernel build does not allow absolute references encoded in
> >> movw/movt instruction pairs, so use our mov_l macro instead (which
> >> will still use such a pair unless CONFIG_RELOCATABLE is defined)
> >>
> >> Also, avoid 32-bit absolute literals to refer to absolute symbols.
> >> Instead, use a 16 bit reference so that PIE linker cannot get
> >> confused whether the symbol reference is subject to relocation at
> >> runtime.
> >
> > This sounds like we're papering over something.
> >
> > If the linker is "confused", that sounds like we are either abusing
> > it somehow, or the linker is broken.
> >
>
> There is some ambiguity in how SHN_ABS symbols are treated in shared
> libraries and PIE executables.
>
> https://sourceware.org/ml/binutils/2012-05/msg00019.html
>
> I haven't confirmed whether it actually causes problems in this
> particular case, but it is safer (and not entirely inappropriate) to
> use a 16-bit field for a quantity that can easily fit one.
OK, so, fundamental linker design flaw that cannot be fixed for
historical reasons. Gotcha.
This "fix" feels like a bit of a hack, since it relies on the absence of
a certain relocation type that could be added later (though it seems
highly unlikely).
Cleaner options would be to expose both symbols that are subtracted,
rather than the result, or to do the subtraction at build time and
generate a header that affected files include (though that's more
invasive on the Makefile side).
May be overkill though.
>
> >
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>
> >> ---
> >> arch/arm/mach-tegra/sleep-tegra20.S | 22 ++++++++++++--------
> >> arch/arm/mach-tegra/sleep-tegra30.S | 6 +++---
> >> arch/arm/mach-tegra/sleep.S | 4 ++--
> >> 3 files changed, 18 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-tegra/sleep-tegra20.S b/arch/arm/mach-tegra/sleep-tegra20.S
> >> index 5c8e638ee51a..cab95de5c8f1 100644
> >> --- a/arch/arm/mach-tegra/sleep-tegra20.S
> >> +++ b/arch/arm/mach-tegra/sleep-tegra20.S
> >> @@ -99,7 +99,7 @@ ENTRY(tegra20_cpu_shutdown)
> >> cmp r0, #0
> >> reteq lr @ must not be called for CPU 0
> >> mov32 r1, TEGRA_IRAM_RESET_BASE_VIRT
> >> - ldr r2, =__tegra20_cpu1_resettable_status_offset
> >> + ldrh r2, 0f
> >> mov r12, #CPU_RESETTABLE
> >> strb r12, [r1, r2]
> >>
> >> @@ -121,6 +121,7 @@ ENTRY(tegra20_cpu_shutdown)
> >> beq .
> >> ret lr
> >> ENDPROC(tegra20_cpu_shutdown)
> >> +0: .short __tegra20_cpu1_resettable_status_offset
> >> #endif
> >>
> >> #ifdef CONFIG_PM_SLEEP
> >> @@ -181,6 +182,9 @@ ENTRY(tegra_pen_unlock)
> >> ret lr
> >> ENDPROC(tegra_pen_unlock)
> >>
> >> +.L__tegra20_cpu1_resettable_status_offset:
> >> + .short __tegra20_cpu1_resettable_status_offset
> >> +
> >> /*
> >> * tegra20_cpu_clear_resettable(void)
> >> *
> >> @@ -189,7 +193,7 @@ ENDPROC(tegra_pen_unlock)
> >> */
> >> ENTRY(tegra20_cpu_clear_resettable)
> >> mov32 r1, TEGRA_IRAM_RESET_BASE_VIRT
> >
> > Can we simply port mov32 to mov_l? Or do we hit a problem with
> > multiplatform kernels where mov_l may involve a literal pool entry
> > (and does it matter)?
> >
>
> The only place where it matters is in code that lives in idmap.text,
> since the relative reference will point to the ID mapped alias of a
> section that is not covered by the ID ID map. I think we should be
> able to use mov_l everywhere else, and it should do the right thing
> for ordinary and PIE builds
Fair enough.
Cheers
---Dave
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.
