Message-ID: <1620037196.377.1561400426591.JavaMail.zimbra@efficios.com>
Date:   Mon, 24 Jun 2019 14:20:26 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Will Deacon <will.deacon@....com>
Cc:     shuah <shuah@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Joel Fernandes <joelaf@...gle.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Dave Watson <davejwatson@...com>,
        Andi Kleen <andi@...stfloor.org>,
        linux-kselftest <linux-kselftest@...r.kernel.org>,
        "H. Peter Anvin" <hpa@...or.com>, Chris Lameter <cl@...ux.com>,
        Russell King <linux@....linux.org.uk>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        "Paul E . McKenney" <paulmck@...ux.vnet.ibm.com>,
        Paul Turner <pjt@...gle.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Josh Triplett <josh@...htriplett.org>,
        rostedt <rostedt@...dmis.org>, Ben Maurer <bmaurer@...com>,
        linux-api <linux-api@...r.kernel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        carlos <carlos@...hat.com>, Florian Weimer <fweimer@...hat.com>
Subject: Re: [RFC PATCH 1/1] Revert "rseq/selftests: arm: use udf
 instruction for RSEQ_SIG"

----- On Jun 24, 2019, at 1:24 PM, Will Deacon will.deacon@....com wrote:

> On Mon, Jun 17, 2019 at 05:23:04PM +0200, Mathieu Desnoyers wrote:
>> This reverts commit 2b845d4b4acd9422bbb668989db8dc36dfc8f438.
>> 
>> That commit introduces build issues for programs compiled in Thumb mode.
>> Rather than try to be clever and emit a valid trap instruction on arm32,
>> which requires special care about big/little endian handling on that
>> architecture, just emit plain data. Data in the instruction stream is
>> technically expected on arm32: this is how literal pools are
>> implemented. Reverting to the prior behavior does exactly that.
>> 
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
>> CC: Peter Zijlstra <peterz@...radead.org>
>> CC: Thomas Gleixner <tglx@...utronix.de>
>> CC: Joel Fernandes <joelaf@...gle.com>
>> CC: Catalin Marinas <catalin.marinas@....com>
>> CC: Dave Watson <davejwatson@...com>
>> CC: Will Deacon <will.deacon@....com>
>> CC: Shuah Khan <shuah@...nel.org>
>> CC: Andi Kleen <andi@...stfloor.org>
>> CC: linux-kselftest@...r.kernel.org
>> CC: "H . Peter Anvin" <hpa@...or.com>
>> CC: Chris Lameter <cl@...ux.com>
>> CC: Russell King <linux@....linux.org.uk>
>> CC: Michael Kerrisk <mtk.manpages@...il.com>
>> CC: "Paul E . McKenney" <paulmck@...ux.vnet.ibm.com>
>> CC: Paul Turner <pjt@...gle.com>
>> CC: Boqun Feng <boqun.feng@...il.com>
>> CC: Josh Triplett <josh@...htriplett.org>
>> CC: Steven Rostedt <rostedt@...dmis.org>
>> CC: Ben Maurer <bmaurer@...com>
>> CC: linux-api@...r.kernel.org
>> CC: Andy Lutomirski <luto@...capital.net>
>> CC: Andrew Morton <akpm@...ux-foundation.org>
>> CC: Linus Torvalds <torvalds@...ux-foundation.org>
>> CC: Carlos O'Donell <carlos@...hat.com>
>> CC: Florian Weimer <fweimer@...hat.com>
>> ---
>>  tools/testing/selftests/rseq/rseq-arm.h | 52 ++-------------------------------
>>  1 file changed, 2 insertions(+), 50 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/rseq/rseq-arm.h
>> b/tools/testing/selftests/rseq/rseq-arm.h
>> index 84f28f147fb6..5f262c54364f 100644
>> --- a/tools/testing/selftests/rseq/rseq-arm.h
>> +++ b/tools/testing/selftests/rseq/rseq-arm.h
>> @@ -5,54 +5,7 @@
>>   * (C) Copyright 2016-2018 - Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
>>   */
>>  
>> -/*
>> - * RSEQ_SIG uses the udf A32 instruction with an uncommon immediate operand
>> - * value 0x5de3. This traps if user-space reaches this instruction by mistake,
>> - * and the uncommon operand ensures the kernel does not move the instruction
>> - * pointer to attacker-controlled code on rseq abort.
>> - *
>> - * The instruction pattern in the A32 instruction set is:
>> - *
>> - * e7f5def3    udf    #24035    ; 0x5de3
>> - *
>> - * This translates to the following instruction pattern in the T16 instruction
>> - * set:
>> - *
>> - * little endian:
>> - * def3        udf    #243      ; 0xf3
>> - * e7f5        b.n    <7f5>
>> - *
>> - * pre-ARMv6 big endian code:
>> - * e7f5        b.n    <7f5>
>> - * def3        udf    #243      ; 0xf3
>> - *
>> - * ARMv6+ -mbig-endian generates mixed endianness code vs data: little-endian
>> - * code and big-endian data. Ensure the RSEQ_SIG data signature matches code
>> - * endianness. Prior to ARMv6, -mbig-endian generates big-endian code and data
>> - * (which match), so there is no need to reverse the endianness of the data
>> - * representation of the signature. However, the choice between BE32 and BE8
>> - * is done by the linker, so we cannot know whether code and data endianness
>> - * will be mixed before the linker is invoked.
>> - */
>> -
>> -#define RSEQ_SIG_CODE	0xe7f5def3
>> -
>> -#ifndef __ASSEMBLER__
>> -
>> -#define RSEQ_SIG_DATA							\
>> -	({								\
>> -		int sig;						\
>> -		asm volatile ("b 2f\n\t"				\
>> -			      "1: .inst " __rseq_str(RSEQ_SIG_CODE) "\n\t" \
>> -			      "2:\n\t"					\
>> -			      "ldr %[sig], 1b\n\t"			\
>> -			      : [sig] "=r" (sig));			\
>> -		sig;							\
>> -	})
>> -
>> -#define RSEQ_SIG	RSEQ_SIG_DATA
>> -
>> -#endif
>> +#define RSEQ_SIG	0x53053053
> 
> I don't get why you're reverting back to this old signature value, when the
> one we came up with will work well when interpreted as an instruction in the
> *vast* majority of scenarios that people care about (A32/T32 little-endian).
> I think you might be under-estimating just how dead things like BE32 really
> are.

My issue is that the current .instr approach is broken for programs or functions
built in Thumb mode, and I received no feedback on the solutions I proposed for
those issues, which led me to propose a patch reverting to a simple .word.

> 
> That said, when you ran into .inst.n/.inst.w issues, did you try something
> along the lines of the WASM() macro we use in arch/arm/, which adds the ".w"
> suffix when targetting Thumb?

AFAIU, the WASM macros depend on CONFIG_THUMB2_KERNEL, which may be fine within
the kernel, but for user-space things are a bit more complex.

A compile-unit can be compiled as thumb, which will set a compiler define
which we could use to detect thumb mode. However, unfortunately, a single
function can also be compiled with an attribute selecting thumb mode, which
AFAIU does not influence the preprocessor defines.

If we had the equivalent of pushsection and popsection for "arm" mode vs "thumb"
mode in asm, it would solve all our issues, but the only way I found to do
something equivalent was to use the size of a no-op in a asm conditional
to restore the previous state.

Any other ideas on how to handle this ?

Thanks,

Mathieu


> 
> Will

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Powered by blists - more mailing lists