linux-kernel - Re: [RFC PATCH 1/1] Revert "rseq/selftests: arm: use udf instruction for RSEQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190626180012.q6deohrtzwpbhqky@willie-the-truck>
Date:   Wed, 26 Jun 2019 19:00:13 +0100
From:   Will Deacon <will@...nel.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     Will Deacon <will.deacon@....com>, 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 Tue, Jun 25, 2019 at 10:08:52AM -0400, Mathieu Desnoyers wrote:
> ----- On Jun 25, 2019, at 5:15 AM, Will Deacon will.deacon@....com wrote:
> > 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:
> >> >> -#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.
> 
> As long as we state and document that this should not be expected to generate
> valid instructions on big endian prior to ARMv6, I'm OK with that approach, e.g.:
> 
> /*
>  * - ARM little endian
>  *
>  * 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>
>  *
>  * - ARMv6+ big endian:

Maybe mention "(BE8)" here...

>  *
>  * ARMv6+ -mbig-endian generates mixed endianness code vs data: little-endian
>  * code and big-endian data. The data value of the signature needs to have its
>  * byte order reversed to generate the trap instruction:
>  *
>  * Data: 0xf3def5e7
>  *
>  * Translates to this A32 instruction pattern:
>  *
>  * e7f5def3    udf    #24035    ; 0x5de3
>  *
>  * Translates to this T16 instruction pattern:
>  *
>  * def3        udf    #243      ; 0xf3
>  * e7f5        b.n    <7f5>
>  *
>  * - Prior to ARMv6 big endian:

... and "(BE32)" here.

With that, this looks fine to me.

Will