[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 14 Aug 2017 15:49:15 +0100
From: Ard Biesheuvel <ard.biesheuvel@...aro.org>
To: Dave Martin <Dave.Martin@....com>
Cc: Kernel Hardening <kernel-hardening@...ts.openwall.com>,
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>,
Russell King <linux@...linux.org.uk>, Tony Lindgren <tony@...mide.com>,
Matt Fleming <matt@...eblueprint.co.uk>, Thomas Garnier <thgarnie@...gle.com>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 17/30] arm-soc: tegra: make sleep asm code runtime relocatable
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.
>
>> 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
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.
