| Message ID | 20190503193858.9676-1-mathieu.desnoyers@efficios.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
----- On May 3, 2019, at 3:38 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: > Use udf as the guard instruction for the restartable sequence abort > handler. > > Previously, the chosen signature was not a valid instruction, based > on the assumption that it could always sit in a literal pool. However, > there are compilation environments in which literal pools are not > available, for instance execute-only code. Therefore, we need to > choose a signature value that is also a valid instruction. > > Handle compiling with -mbig-endian on ARMv6+, which generates binaries > with mixed code vs data endianness (little endian code, big endian > data). > > Else mismatch between code endianness for the generated signatures and > data endianness for the RSEQ_SIG parameter passed to the rseq > registration will trigger application segmentation faults when the > kernel try to abort rseq critical sections. > > Prior to ARMv6, -mbig-endian generates big-endian code and data, so > endianness should not be reversed in that case. And of course it cannot be that easy. This breaks when building in thumb mode (-mthumb). Output from librseq arm32 build [1] (code similar to what is found in the rseq selftests): CC rseq.lo /tmp/ccu6Jw1b.s: Assembler messages: /tmp/ccu6Jw1b.s:297: Error: cannot determine Thumb instruction size. Use .inst.n/.inst.w instead /tmp/ccu6Jw1b.s:490: Error: cannot determine Thumb instruction size. Use .inst.n/.inst.w instead Makefile:460: recipe for target 'rseq.lo' failed This appears to be caused by a missing .arm directive in RSEQ_SIG_DATA. Fixing with: - asm volatile ("b 2f\n\t" \ + asm volatile (".arm\n\t" \ + "b 2f\n\t" \ gets the build to go further, but breaks at: CC basic_percpu_ops_test.o /tmp/ccpHOMHZ.s: Assembler messages: /tmp/ccpHOMHZ.s:148: Error: misaligned branch destination /tmp/ccpHOMHZ.s:956: Error: misaligned branch destination Makefile:378: recipe for target 'basic_percpu_ops_test.o' failed I suspect it's caused by the change from: - ".word " __rseq_str(RSEQ_SIG) "\n\t" \ to + ".arm\n\t" \ + ".inst " __rseq_str(RSEQ_SIG_CODE) "\n\t" \ which changes the mode from thumb to arm for the rest of the inline asm within __RSEQ_ASM_DEFINE_ABORT. Better yet, there appears to be no way to save the arm/thumb state and restore it afterwards. I'm really starting to wonder if we should go our of our way to try to get this signature to be a valid instruction on arm32. Perhaps we should consider going back to use ".word" on arm32 so it ensures it uses data endianness (which matches the parameter received by the sys_rseq system call), let objdump and friends print it as a literal pool (which it is), and just choose an instruction which has little chances to appear for the cases we care about between ARM32 BE, LE and THUMB. Perhaps a 32-bit palindrome ? Bonus points if this is a trap instruction in common configurations for odd-cases-debugging purposes. Thoughts ? Thanks, Mathieu [1] https://github.com/compudj/librseq > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > CC: Peter Zijlstra <peterz@infradead.org> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Joel Fernandes <joelaf@google.com> > CC: Catalin Marinas <catalin.marinas@arm.com> > CC: Dave Watson <davejwatson@fb.com> > CC: Will Deacon <will.deacon@arm.com> > CC: Shuah Khan <shuah@kernel.org> > CC: Andi Kleen <andi@firstfloor.org> > CC: linux-kselftest@vger.kernel.org > CC: "H . Peter Anvin" <hpa@zytor.com> > CC: Chris Lameter <cl@linux.com> > CC: Russell King <linux@arm.linux.org.uk> > CC: Michael Kerrisk <mtk.manpages@gmail.com> > CC: "Paul E . McKenney" <paulmck@linux.vnet.ibm.com> > CC: Paul Turner <pjt@google.com> > CC: Boqun Feng <boqun.feng@gmail.com> > CC: Josh Triplett <josh@joshtriplett.org> > CC: Steven Rostedt <rostedt@goodmis.org> > CC: Ben Maurer <bmaurer@fb.com> > CC: linux-api@vger.kernel.org > CC: Andy Lutomirski <luto@amacapital.net> > CC: Andrew Morton <akpm@linux-foundation.org> > CC: Linus Torvalds <torvalds@linux-foundation.org> > --- > Changes since v1: > - Fix checkpatch error and warning. > > --- > tools/testing/selftests/rseq/rseq-arm.h | 52 +++++++++++++++++++++++++++++++-- > 1 file changed, 50 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/rseq/rseq-arm.h > b/tools/testing/selftests/rseq/rseq-arm.h > index 5f262c54364f..84f28f147fb6 100644 > --- a/tools/testing/selftests/rseq/rseq-arm.h > +++ b/tools/testing/selftests/rseq/rseq-arm.h > @@ -5,7 +5,54 @@ > * (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 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_smp_mb() __asm__ __volatile__ ("dmb" ::: "memory", "cc") > #define rseq_smp_rmb() __asm__ __volatile__ ("dmb" ::: "memory", "cc") > @@ -78,7 +125,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_CODE) "\n\t" \ > __rseq_str(label) ":\n\t" \ > teardown \ > "b %l[" __rseq_str(abort_label) "]\n\t" > -- > 2.11.0
----- On Jun 6, 2019, at 8:02 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: > ----- On May 3, 2019, at 3:38 PM, Mathieu Desnoyers > mathieu.desnoyers@efficios.com wrote: > >> Use udf as the guard instruction for the restartable sequence abort >> handler. >> >> Previously, the chosen signature was not a valid instruction, based >> on the assumption that it could always sit in a literal pool. However, >> there are compilation environments in which literal pools are not >> available, for instance execute-only code. Therefore, we need to >> choose a signature value that is also a valid instruction. >> >> Handle compiling with -mbig-endian on ARMv6+, which generates binaries >> with mixed code vs data endianness (little endian code, big endian >> data). >> >> Else mismatch between code endianness for the generated signatures and >> data endianness for the RSEQ_SIG parameter passed to the rseq >> registration will trigger application segmentation faults when the >> kernel try to abort rseq critical sections. >> >> Prior to ARMv6, -mbig-endian generates big-endian code and data, so >> endianness should not be reversed in that case. > > And of course it cannot be that easy. This breaks when building in > thumb mode (-mthumb). Output from librseq arm32 build [1] (code similar > to what is found in the rseq selftests): > > CC rseq.lo > /tmp/ccu6Jw1b.s: Assembler messages: > /tmp/ccu6Jw1b.s:297: Error: cannot determine Thumb instruction size. Use > .inst.n/.inst.w instead > /tmp/ccu6Jw1b.s:490: Error: cannot determine Thumb instruction size. Use > .inst.n/.inst.w instead > Makefile:460: recipe for target 'rseq.lo' failed > > This appears to be caused by a missing .arm directive in RSEQ_SIG_DATA. > Fixing with: > > - asm volatile ("b 2f\n\t" \ > + asm volatile (".arm\n\t" \ > + "b 2f\n\t" \ > > gets the build to go further, but breaks at: > > CC basic_percpu_ops_test.o > /tmp/ccpHOMHZ.s: Assembler messages: > /tmp/ccpHOMHZ.s:148: Error: misaligned branch destination > /tmp/ccpHOMHZ.s:956: Error: misaligned branch destination > Makefile:378: recipe for target 'basic_percpu_ops_test.o' failed > > I suspect it's caused by the change from: > > - ".word " __rseq_str(RSEQ_SIG) "\n\t" \ > > to > > + ".arm\n\t" \ > + ".inst " __rseq_str(RSEQ_SIG_CODE) "\n\t" \ > > which changes the mode from thumb to arm for the rest of the > inline asm within __RSEQ_ASM_DEFINE_ABORT. Better yet, there appears > to be no way to save the arm/thumb state and restore it afterwards. > > I'm really starting to wonder if we should go our of our way to try > to get this signature to be a valid instruction on arm32. Perhaps > we should consider going back to use ".word" on arm32 so it ensures > it uses data endianness (which matches the parameter received by the > sys_rseq system call), let objdump and friends print it as a literal > pool (which it is), and just choose an instruction which has little > chances to appear for the cases we care about between ARM32 BE, LE > and THUMB. Perhaps a 32-bit palindrome ? Bonus points if this is a > trap instruction in common configurations for odd-cases-debugging > purposes. So I'm not particularly proud of the result, but I found a rather ugly way to figure out if we are currently in thumb mode within an inline asm, and restore that mode: test the length of a nop instruction with a ".if" asm statement. Do we want to go for this kind of approach, or should we revert back to a ".word" and accept that the rseq signature before the abort handler will be seen as data rather than an instruction on arm32 ? Is there a better way to do this ? Thanks, Mathieu diff --git a/include/rseq/rseq-arm.h b/include/rseq/rseq-arm.h index 1ce9231..b6c36dd 100644 --- a/include/rseq/rseq-arm.h +++ b/include/rseq/rseq-arm.h @@ -43,7 +43,14 @@ ({ \ int sig; \ asm volatile ("b 2f\n\t" \ + "3:\n\t" \ + "nop\n\t" \ + "4:\n\t" \ + ".arm\n\t" \ "1: .inst " __rseq_str(RSEQ_SIG_CODE) "\n\t" \ + ".if ((4b - 3b) == 2)\n\t" \ + ".thumb\n\t" \ + ".endif\n\t" \ "2:\n\t" \ "ldr %[sig], 1b\n\t" \ : [sig] "=r" (sig)); \ @@ -125,8 +132,14 @@ 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" \ + "333:\n\t" \ + "nop\n\t" \ + "444:\n\t" \ ".arm\n\t" \ ".inst " __rseq_str(RSEQ_SIG_CODE) "\n\t" \ + ".if ((444b - 333b) == 2)\n\t" \ + ".thumb\n\t" \ + ".endif\n\t" \ __rseq_str(label) ":\n\t" \ teardown \ "b %l[" __rseq_str(abort_label) "]\n\t"
diff --git a/tools/testing/selftests/rseq/rseq-arm.h b/tools/testing/selftests/rseq/rseq-arm.h index 5f262c54364f..84f28f147fb6 100644 --- a/tools/testing/selftests/rseq/rseq-arm.h +++ b/tools/testing/selftests/rseq/rseq-arm.h @@ -5,7 +5,54 @@ * (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 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_smp_mb() __asm__ __volatile__ ("dmb" ::: "memory", "cc") #define rseq_smp_rmb() __asm__ __volatile__ ("dmb" ::: "memory", "cc") @@ -78,7 +125,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_CODE) "\n\t" \ __rseq_str(label) ":\n\t" \ teardown \ "b %l[" __rseq_str(abort_label) "]\n\t"
Use udf as the guard instruction for the restartable sequence abort handler. Previously, the chosen signature was not a valid instruction, based on the assumption that it could always sit in a literal pool. However, there are compilation environments in which literal pools are not available, for instance execute-only code. Therefore, we need to choose a signature value that is also a valid instruction. Handle compiling with -mbig-endian on ARMv6+, which generates binaries with mixed code vs data endianness (little endian code, big endian data). Else mismatch between code endianness for the generated signatures and data endianness for the RSEQ_SIG parameter passed to the rseq registration will trigger application segmentation faults when the kernel try to abort rseq critical sections. Prior to ARMv6, -mbig-endian generates big-endian code and data, so endianness should not be reversed in that case. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> CC: Peter Zijlstra <peterz@infradead.org> CC: Thomas Gleixner <tglx@linutronix.de> CC: Joel Fernandes <joelaf@google.com> CC: Catalin Marinas <catalin.marinas@arm.com> CC: Dave Watson <davejwatson@fb.com> CC: Will Deacon <will.deacon@arm.com> CC: Shuah Khan <shuah@kernel.org> CC: Andi Kleen <andi@firstfloor.org> CC: linux-kselftest@vger.kernel.org CC: "H . Peter Anvin" <hpa@zytor.com> CC: Chris Lameter <cl@linux.com> CC: Russell King <linux@arm.linux.org.uk> CC: Michael Kerrisk <mtk.manpages@gmail.com> CC: "Paul E . McKenney" <paulmck@linux.vnet.ibm.com> CC: Paul Turner <pjt@google.com> CC: Boqun Feng <boqun.feng@gmail.com> CC: Josh Triplett <josh@joshtriplett.org> CC: Steven Rostedt <rostedt@goodmis.org> CC: Ben Maurer <bmaurer@fb.com> CC: linux-api@vger.kernel.org CC: Andy Lutomirski <luto@amacapital.net> CC: Andrew Morton <akpm@linux-foundation.org> CC: Linus Torvalds <torvalds@linux-foundation.org> --- Changes since v1: - Fix checkpatch error and warning. --- tools/testing/selftests/rseq/rseq-arm.h | 52 +++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-)