Message-ID: <20190625091507.GA13263@fuggles.cambridge.arm.com>
Date:   Tue, 25 Jun 2019 10:15:07 +0100
From:   Will Deacon <will.deacon@....com>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.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 Mon, Jun 24, 2019 at 02:20:26PM -0400, Mathieu Desnoyers wrote:
> ----- 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.

I understand why you're moving from .inst to .word, but I don't understand
why that necessitates a change in the value. Why not .word 0xe7f5def3 ? You
could also flip the bytes around in case of big-endian, which would keep the
instruction coding clean for BE8.

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

Thanks, I hadn't considered that case. I don't know the right way to handle
that in the toolchain, so using .word is probably the best bet in the
absence of any better suggestions from the tools folks.

Will

Powered by blists - more mailing lists