On 4/17/20 1:55 PM, Szabolcs Nagy wrote:
The 04/17/2020 12:50, Jason Merrill wrote:
On 4/17/20 6:08 AM, Kyrylo Tkachov wrote:
Hi Szabolcs,
-----Original Message-----
From: Szabolcs Nagy <szabolcs.n...@arm.com>
Sent: 09 April 2020 15:20
To: gcc-patches@gcc.gnu.org
Cc: Richard Earnshaw <richard.earns...@arm.com>; Richard Sandiford
<richard.sandif...@arm.com>; Kyrylo Tkachov <kyrylo.tkac...@arm.com>
Subject: [PATCH] aarch64: Fix .cfi_window_save with pac-ret [PR94515]
On aarch64 -mbranch-protection=pac-ret reuses the dwarf
opcode for window_save to mean "toggle the return address
mangle state", but in the dwarf2cfi internal logic the
state was not properly recorded so the remember/restore
logic was broken.
This can cause the unwinder not to authenticate return
addresses that were signed (or vice versa) which means
a runtime crash on a pauth enabled system.
Currently only aarch64 pac-ret uses REG_CFA_TOGGLE_RA_MANGLE.
I think this is ok, but this code is in the midend so I've CC'ed Jason who,
from what I gather from MAINTAINERS, is maintainer for this file.
Thanks,
Kyrill
gcc/ChangeLog:
2020-04-XX Szabolcs Nagy <szabolcs.n...@arm.com>
PR target/94515
* dwarf2cfi.c (dwarf2out_frame_debug): Record RA
mangle state in cur_row->window_save.
gcc/testsuite/ChangeLog:
2020-04-XX Szabolcs Nagy <szabolcs.n...@arm.com>
PR target/94515
* g++.target/aarch64/pr94515.C: New test.
---
gcc/dwarf2cfi.c | 3 ++
gcc/testsuite/g++.target/aarch64/pr94515.C | 43 ++++++++++++++++++++++
2 files changed, 46 insertions(+)
create mode 100644 gcc/testsuite/g++.target/aarch64/pr94515.C
diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c
index 229fbfacc30..22989a6c2fb 100644
--- a/gcc/dwarf2cfi.c
+++ b/gcc/dwarf2cfi.c
@@ -2145,6 +2145,9 @@ dwarf2out_frame_debug (rtx_insn *insn)
case REG_CFA_TOGGLE_RA_MANGLE:
/* This uses the same DWARF opcode as the next operation. */
dwarf2out_frame_debug_cfa_window_save (true);
+ /* Keep track of RA mangle state by toggling the window_save bit.
+ This is needed so .cfi_window_save is emitted correctly. */
+ cur_row->window_save = !cur_row->window_save;
It looks like passing 'true' to dwarf2out_frame_debug_cfa_window_save
prevents that function from messing with the window_safe flag. Does
changing the argument to 'false' get the behavior you want?
we want the state = !state toggling.
it might make more sense to do that in
dwarf2out_frame_debug_cfa_window_save(true)
or to inline that entire logic into the two
places where it is used (instead of
dispatching with a bool argument).
I think that inlining and dropping the parameter would be cleaner.
for the bug fix i'd like a minimal change
(so it can be backported), doing the fix in
dwarf2out_frame_debug_cfa_window_save
is fine with me, would you prefer that?
No, thanks. If you want to commit your patch as is for backporting and
then do the inlining in a separate commit, that works for me.
handled_one = true;
break;
diff --git a/gcc/testsuite/g++.target/aarch64/pr94515.C
b/gcc/testsuite/g++.target/aarch64/pr94515.C
new file mode 100644
index 00000000000..7707c773a72
--- /dev/null
+++ b/gcc/testsuite/g++.target/aarch64/pr94515.C
@@ -0,0 +1,43 @@
+/* PR target/94515. Check .cfi_window_save with multiple return paths. */
+/* { dg-do run } */
+/* { dg-additional-options "-O2 -mbranch-protection=pac-ret --save-temps" }
*/
+
+volatile int zero = 0;
+
+__attribute__((noinline, target("branch-protection=none")))
+void unwind (void)
+{
+ if (zero == 0)
+ throw 42;
+}
+
+__attribute__((noinline, noipa, target("branch-protection=pac-ret")))
+int test (int z)
+{
+ if (z) {
+ asm volatile ("":::"x20","x21");
+ unwind ();
+ return 1;
+ } else {
+ unwind ();
+ return 2;
+ }
+}
+
+__attribute__((target("branch-protection=none")))
+int main ()
+{
+ try {
+ test (zero);
+ __builtin_abort ();
+ } catch (...) {
+ return 0;
+ }
+ __builtin_abort ();
+}
+
+/* This check only works if there are two return paths in test and
+ cfi_window_save is used for both instead of cfi_remember_state
+ plus cfi_restore_state. This is currently the case with -O2. */
+
+/* { dg-final { scan-assembler-times {\t\.cfi_window_save\n} 4 } } */
--
2.17.1
