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@xxxxxxx 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@xxxxxxx 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
