Re: rseq/arm32: choosing rseq code signature

[Will Deacon <will.deacon@arm.com>]

Hi Mathieu,

On Wed, Apr 10, 2019 at 04:29:19PM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 9, 2019, at 3:32 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> > We are about to include the code signature required prior to restartable
> > sequences abort handlers into glibc, which will make this ABI choice final.
> > We need architecture maintainer input on that signature value.
> > 
> > That code signature is placed before each abort handler, so the kernel can
> > validate that it is indeed jumping to an abort handler (and not some
> > arbitrary attacker-chosen code). The signature is never executed.
> > 
> > The current discussion thread on the glibc mailing list leads us towards
> > using a trap with uncommon immediate operand, which simplifies integration
> > with disassemblers, emulators, makes it easier to debug if the control
> > flow gets redirected there by mistake, and is nicer for some architecture's
> > speculative execution.
> > 
> > We can have different signatures for each sub-architecture, as long as they
> > don't have to co-exist within the same process. We can special-case with
> > #ifdef for each sub-architecture and endianness if need be. If the architecture
> > has instruction set extensions that can co-exist with the architecture
> > instruction set within the same process (e.g. thumb for arm), we need to take
> > into account to which instruction the chosen signature value would map (and
> > possibly decide if we need to extend rseq to support many signatures).
> > 
> > Here is an example of rseq signature definition template:
> > 
> > /*
> > * TODO: document trap instruction objdump output on each sub-architecture
> > * instruction sets, as well as instruction set extensions.
> > */
> > #define RSEQ_SIG 0x########
> > 
> > Ideally we'd need a patch on top of the Linux kernel
> > tools/testing/selftests/rseq/rseq-arm.h file that updates
> > the signature value, so I can then pick it up for the glibc
> > patchset.
> 
> Would the following diff work for you ? If so, can I get your
> acked-by ?

I had a quick chat with Richard and Peter (CC'd), since they're much more
familiar with the A32 instruction set than I am and also have a better view
of what might already be in use.

Peter suggests that anything of the form 0xe7fxdefx should trap in both A32
and T32, although it does assemble to UDF; B <imm11> in T16. I'm not sure we
should get too obsessed with trying to encode a signature that universally
decodes to a trap.

Whatever you choose, it would be worth checking that it doesn't clash with
other allocations such as software breakpoints in GDB.

Will

> diff --git a/tools/testing/selftests/rseq/rseq-arm.h b/tools/testing/selftests/rseq/rseq-arm.h
> index 5f262c54364f..1f261ad2ac1b 100644
> --- a/tools/testing/selftests/rseq/rseq-arm.h
> +++ b/tools/testing/selftests/rseq/rseq-arm.h
> @@ -5,7 +5,17 @@
>   * (C) Copyright 2016-2018 - Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>   */
>  
> -#define RSEQ_SIG       0x53053053
> +/*
> + * RSEQ_SIG uses the udf A32 instruction with an uncommon immediate operand
> + * value 0x5305. 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 is:
> + *
> + * e7f530f5    udf    #21253    ; 0x5305
> + */
> +#define RSEQ_SIG       0xe7f530f5
>  
>  #define rseq_smp_mb()  __asm__ __volatile__ ("dmb" ::: "memory", "cc")
>  #define rseq_smp_rmb() __asm__ __volatile__ ("dmb" ::: "memory", "cc")
> @@ -78,7 +88,8 @@ do {                                                                  \
>                 __rseq_str(table_label) ":\n\t"                         \
>                 ".word " __rseq_str(version) ", " __rseq_str(flags) "\n\t" \
>                 ".word " __rseq_str(start_ip) ", 0x0, " __rseq_str(post_commit_offset) ", 0x0, " __rseq_str(abort_ip) ", 0x0\n\t" \
> -               ".word " __rseq_str(RSEQ_SIG) "\n\t"                    \
> +               ".arm\n\t"                                              \
> +               ".inst " __rseq_str(RSEQ_SIG) "\n\t"                    \
>                 __rseq_str(label) ":\n\t"                               \
>                 teardown                                                \
>                 "b %l[" __rseq_str(abort_label) "]\n\t"