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@arm.com wrote:

quoted

On Mon, Jun 24, 2019 at 02:20:26PM -0400, Mathieu Desnoyers wrote:

quoted

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

quoted

On Mon, Jun 17, 2019 at 05:23:04PM +0200, Mathieu Desnoyers wrote:

quoted

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