| Message ID | 20200424081936.GF29015@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] aarch64: Fix .cfi_window_save with pac-ret [PR94515] | expand |
Szabolcs Nagy <szabolcs.nagy@arm.com> writes: > @@ -2221,6 +2239,14 @@ change_cfi_row (dw_cfi_row *old_row, dw_cfi_row *new_row) > cfi->dw_cfi_opc = DW_CFA_GNU_window_save; > add_cfi (cfi); > } > + > + if (old_row->ra_mangled != new_row->ra_mangled) > + { > + dw_cfi_ref cfi = new_cfi (); > + /* DW_CFA_GNU_window_save is reused for toggling RA mangle state. */ > + cfi->dw_cfi_opc = DW_CFA_GNU_window_save; > + add_cfi (cfi); > + } > } > > /* Examine CFI and return true if a cfi label and set_loc is needed I wonder if it would be worth asserting: gcc_assert (!old_row->ra_mangled && !new_row->ra_mangled); in the "SPARC" block and: gcc_assert (!old_row->window_save && !new_row->window_save); in the "aarch64" block? > diff --git a/gcc/testsuite/g++.target/aarch64/pr94515-2.C b/gcc/testsuite/g++.target/aarch64/pr94515-2.C > new file mode 100644 > index 00000000000..1f13e1200ad > --- /dev/null > +++ b/gcc/testsuite/g++.target/aarch64/pr94515-2.C > @@ -0,0 +1,41 @@ > +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ > +/* { dg-do run } */ > +/* { dg-require-effective-target lp64 } */ > +/* { dg-additional-options "-O2 -mbranch-protection=pac-ret" } */ > + > +volatile int zero = 0; > +int global = 0; > + > +__attribute__((noinline)) > +int bar(void) > +{ > + if (zero == 0) return 3; > + return 0; > +} > + > +__attribute__((noinline, noreturn)) > +void unwind (void) > +{ > + throw 42; > +} > + > +__attribute__((noinline, noipa, target("branch-protection=pac-ret"))) I'm probably just showing my ignorance, but is this target attribute needed? It looks like all functions in the test are compiled with pac-ret. LGTM otherwise, but please give others 24 hrs to object. Thanks, Richard
The 04/24/2020 15:17, Richard Sandiford wrote: > Szabolcs Nagy <szabolcs.nagy@arm.com> writes: > > @@ -2221,6 +2239,14 @@ change_cfi_row (dw_cfi_row *old_row, dw_cfi_row *new_row) > > cfi->dw_cfi_opc = DW_CFA_GNU_window_save; > > add_cfi (cfi); > > } > > + > > + if (old_row->ra_mangled != new_row->ra_mangled) > > + { > > + dw_cfi_ref cfi = new_cfi (); > > + /* DW_CFA_GNU_window_save is reused for toggling RA mangle state. */ > > + cfi->dw_cfi_opc = DW_CFA_GNU_window_save; > > + add_cfi (cfi); > > + } > > } > > > > /* Examine CFI and return true if a cfi label and set_loc is needed > > I wonder if it would be worth asserting: > > gcc_assert (!old_row->ra_mangled && !new_row->ra_mangled); > > in the "SPARC" block and: > > gcc_assert (!old_row->window_save && !new_row->window_save); > > in the "aarch64" block? makes sense. i'll fix it. > > diff --git a/gcc/testsuite/g++.target/aarch64/pr94515-2.C b/gcc/testsuite/g++.target/aarch64/pr94515-2.C > > new file mode 100644 > > index 00000000000..1f13e1200ad > > --- /dev/null > > +++ b/gcc/testsuite/g++.target/aarch64/pr94515-2.C > > @@ -0,0 +1,41 @@ > > +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ > > +/* { dg-do run } */ > > +/* { dg-require-effective-target lp64 } */ > > +/* { dg-additional-options "-O2 -mbranch-protection=pac-ret" } */ > > + > > +volatile int zero = 0; > > +int global = 0; > > + > > +__attribute__((noinline)) > > +int bar(void) > > +{ > > + if (zero == 0) return 3; > > + return 0; > > +} > > + > > +__attribute__((noinline, noreturn)) > > +void unwind (void) > > +{ > > + throw 42; > > +} > > + > > +__attribute__((noinline, noipa, target("branch-protection=pac-ret"))) > > I'm probably just showing my ignorance, but is this target attribute > needed? It looks like all functions in the test are compiled with pac-ret. no, that's left over before i added it to the cflags, i'll fix it. > > LGTM otherwise, but please give others 24 hrs to object. thanks. > > Thanks, > Richard --
On 4/24/20 4:19 AM, Szabolcs Nagy wrote: > 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 updated when an opcode was emitted, the > currently present update logic is only valid for the > original sparc use of window_save so a separate bool is > used on aarch64 to track the state. > > This bug 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 suppose factoring things so that this logic could live in the backend is too much trouble for how small the code is. The patch is OK. > This should be backported to gcc-9 and gcc-8 branches. > > gcc/ChangeLog: > > 2020-04-23 Szabolcs Nagy <szabolcs.nagy@arm.com> > > PR target/94515 > * dwarf2cfi.c (struct GTY): Add ra_mangled. > (cfi_row_equal_p): Check ra_mangled. > (dwarf2out_frame_debug_cfa_window_save): Remove the argument, > this only handles the sparc logic now. > (dwarf2out_frame_debug_cfa_toggle_ra_mangle): New function for > the aarch64 specific logic. > (dwarf2out_frame_debug): Update to use the new subroutines. > (change_cfi_row): Check ra_mangled. > > gcc/testsuite/ChangeLog: > > 2020-04-23 Szabolcs Nagy <szabolcs.nagy@arm.com> > > PR target/94515 > * g++.target/aarch64/pr94515-1.C: New test. > * g++.target/aarch64/pr94515-2.C: New test. > --- > gcc/dwarf2cfi.c | 40 ++++++++++++++---- > gcc/testsuite/g++.target/aarch64/pr94515-1.C | 44 ++++++++++++++++++++ > gcc/testsuite/g++.target/aarch64/pr94515-2.C | 41 ++++++++++++++++++ > 3 files changed, 118 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/g++.target/aarch64/pr94515-1.C > create mode 100644 gcc/testsuite/g++.target/aarch64/pr94515-2.C > > diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c > index 229fbfacc30..fa9e6089b2a 100644 > --- a/gcc/dwarf2cfi.c > +++ b/gcc/dwarf2cfi.c > @@ -71,6 +71,9 @@ struct GTY(()) dw_cfi_row > > /* True if the register window is saved. */ > bool window_save; > + > + /* True if the return address is in a mangled state. */ > + bool ra_mangled; > }; > > /* The caller's ORIG_REG is saved in SAVED_IN_REG. */ > @@ -772,6 +775,9 @@ cfi_row_equal_p (dw_cfi_row *a, dw_cfi_row *b) > if (a->window_save != b->window_save) > return false; > > + if (a->ra_mangled != b->ra_mangled) > + return false; > + > return true; > } > > @@ -1370,20 +1376,33 @@ dwarf2out_frame_debug_cfa_restore (rtx reg) > } > > /* A subroutine of dwarf2out_frame_debug, process a REG_CFA_WINDOW_SAVE. > - FAKE is true if this is not really a window save but something else. > > ??? Perhaps we should note in the CIE where windows are saved (instead > of assuming 0(cfa)) and what registers are in the window. */ > > static void > -dwarf2out_frame_debug_cfa_window_save (bool fake) > +dwarf2out_frame_debug_cfa_window_save (void) > +{ > + dw_cfi_ref cfi = new_cfi (); > + > + cfi->dw_cfi_opc = DW_CFA_GNU_window_save; > + add_cfi (cfi); > + cur_row->window_save = true; > +} > + > +/* A subroutine of dwarf2out_frame_debug, process a REG_CFA_TOGGLE_RA_MANGLE. > + Note: DW_CFA_GNU_window_save dwarf opcode is reused for toggling RA mangle > + state, this is a target specific operation on AArch64 and can only be used > + on other targets if they don't use the window save operation otherwise. */ > + > +static void > +dwarf2out_frame_debug_cfa_toggle_ra_mangle (void) > { > dw_cfi_ref cfi = new_cfi (); > > cfi->dw_cfi_opc = DW_CFA_GNU_window_save; > add_cfi (cfi); > - if (!fake) > - cur_row->window_save = true; > + cur_row->ra_mangled = !cur_row->ra_mangled; > } > > /* Record call frame debugging information for an expression EXPR, > @@ -2143,13 +2162,12 @@ dwarf2out_frame_debug (rtx_insn *insn) > break; > > case REG_CFA_TOGGLE_RA_MANGLE: > - /* This uses the same DWARF opcode as the next operation. */ > - dwarf2out_frame_debug_cfa_window_save (true); > + dwarf2out_frame_debug_cfa_toggle_ra_mangle (); > handled_one = true; > break; > > case REG_CFA_WINDOW_SAVE: > - dwarf2out_frame_debug_cfa_window_save (false); > + dwarf2out_frame_debug_cfa_window_save (); > handled_one = true; > break; > > @@ -2221,6 +2239,14 @@ change_cfi_row (dw_cfi_row *old_row, dw_cfi_row *new_row) > cfi->dw_cfi_opc = DW_CFA_GNU_window_save; > add_cfi (cfi); > } > + > + if (old_row->ra_mangled != new_row->ra_mangled) > + { > + dw_cfi_ref cfi = new_cfi (); > + /* DW_CFA_GNU_window_save is reused for toggling RA mangle state. */ > + cfi->dw_cfi_opc = DW_CFA_GNU_window_save; > + add_cfi (cfi); > + } > } > > /* Examine CFI and return true if a cfi label and set_loc is needed > diff --git a/gcc/testsuite/g++.target/aarch64/pr94515-1.C b/gcc/testsuite/g++.target/aarch64/pr94515-1.C > new file mode 100644 > index 00000000000..cd63f4fb259 > --- /dev/null > +++ b/gcc/testsuite/g++.target/aarch64/pr94515-1.C > @@ -0,0 +1,44 @@ > +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ > +/* { dg-do run } */ > +/* { dg-require-effective-target lp64 } */ > +/* { 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 } } */ > diff --git a/gcc/testsuite/g++.target/aarch64/pr94515-2.C b/gcc/testsuite/g++.target/aarch64/pr94515-2.C > new file mode 100644 > index 00000000000..1f13e1200ad > --- /dev/null > +++ b/gcc/testsuite/g++.target/aarch64/pr94515-2.C > @@ -0,0 +1,41 @@ > +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ > +/* { dg-do run } */ > +/* { dg-require-effective-target lp64 } */ > +/* { dg-additional-options "-O2 -mbranch-protection=pac-ret" } */ > + > +volatile int zero = 0; > +int global = 0; > + > +__attribute__((noinline)) > +int bar(void) > +{ > + if (zero == 0) return 3; > + return 0; > +} > + > +__attribute__((noinline, noreturn)) > +void unwind (void) > +{ > + throw 42; > +} > + > +__attribute__((noinline, noipa, target("branch-protection=pac-ret"))) > +int test(int x) > +{ > + if (x==1) return 2; /* This return path may not use the stack. */ > + int y = bar(); > + if (y > global) global=y; > + if (y==3) unwind(); /* This return path must have RA mangle state set. */ > + return 0; > +} > + > +int main () > +{ > + try { > + test (zero); > + __builtin_abort (); > + } catch (...) { > + return 0; > + } > + __builtin_abort (); > +} >
diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c index 229fbfacc30..fa9e6089b2a 100644 --- a/gcc/dwarf2cfi.c +++ b/gcc/dwarf2cfi.c @@ -71,6 +71,9 @@ struct GTY(()) dw_cfi_row /* True if the register window is saved. */ bool window_save; + + /* True if the return address is in a mangled state. */ + bool ra_mangled; }; /* The caller's ORIG_REG is saved in SAVED_IN_REG. */ @@ -772,6 +775,9 @@ cfi_row_equal_p (dw_cfi_row *a, dw_cfi_row *b) if (a->window_save != b->window_save) return false; + if (a->ra_mangled != b->ra_mangled) + return false; + return true; } @@ -1370,20 +1376,33 @@ dwarf2out_frame_debug_cfa_restore (rtx reg) } /* A subroutine of dwarf2out_frame_debug, process a REG_CFA_WINDOW_SAVE. - FAKE is true if this is not really a window save but something else. ??? Perhaps we should note in the CIE where windows are saved (instead of assuming 0(cfa)) and what registers are in the window. */ static void -dwarf2out_frame_debug_cfa_window_save (bool fake) +dwarf2out_frame_debug_cfa_window_save (void) +{ + dw_cfi_ref cfi = new_cfi (); + + cfi->dw_cfi_opc = DW_CFA_GNU_window_save; + add_cfi (cfi); + cur_row->window_save = true; +} + +/* A subroutine of dwarf2out_frame_debug, process a REG_CFA_TOGGLE_RA_MANGLE. + Note: DW_CFA_GNU_window_save dwarf opcode is reused for toggling RA mangle + state, this is a target specific operation on AArch64 and can only be used + on other targets if they don't use the window save operation otherwise. */ + +static void +dwarf2out_frame_debug_cfa_toggle_ra_mangle (void) { dw_cfi_ref cfi = new_cfi (); cfi->dw_cfi_opc = DW_CFA_GNU_window_save; add_cfi (cfi); - if (!fake) - cur_row->window_save = true; + cur_row->ra_mangled = !cur_row->ra_mangled; } /* Record call frame debugging information for an expression EXPR, @@ -2143,13 +2162,12 @@ dwarf2out_frame_debug (rtx_insn *insn) break; case REG_CFA_TOGGLE_RA_MANGLE: - /* This uses the same DWARF opcode as the next operation. */ - dwarf2out_frame_debug_cfa_window_save (true); + dwarf2out_frame_debug_cfa_toggle_ra_mangle (); handled_one = true; break; case REG_CFA_WINDOW_SAVE: - dwarf2out_frame_debug_cfa_window_save (false); + dwarf2out_frame_debug_cfa_window_save (); handled_one = true; break; @@ -2221,6 +2239,14 @@ change_cfi_row (dw_cfi_row *old_row, dw_cfi_row *new_row) cfi->dw_cfi_opc = DW_CFA_GNU_window_save; add_cfi (cfi); } + + if (old_row->ra_mangled != new_row->ra_mangled) + { + dw_cfi_ref cfi = new_cfi (); + /* DW_CFA_GNU_window_save is reused for toggling RA mangle state. */ + cfi->dw_cfi_opc = DW_CFA_GNU_window_save; + add_cfi (cfi); + } } /* Examine CFI and return true if a cfi label and set_loc is needed diff --git a/gcc/testsuite/g++.target/aarch64/pr94515-1.C b/gcc/testsuite/g++.target/aarch64/pr94515-1.C new file mode 100644 index 00000000000..cd63f4fb259 --- /dev/null +++ b/gcc/testsuite/g++.target/aarch64/pr94515-1.C @@ -0,0 +1,44 @@ +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ +/* { 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 } } */ diff --git a/gcc/testsuite/g++.target/aarch64/pr94515-2.C b/gcc/testsuite/g++.target/aarch64/pr94515-2.C new file mode 100644 index 00000000000..1f13e1200ad --- /dev/null +++ b/gcc/testsuite/g++.target/aarch64/pr94515-2.C @@ -0,0 +1,41 @@ +/* PR target/94515. Check .cfi_window_save with multiple return paths. */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ +/* { dg-additional-options "-O2 -mbranch-protection=pac-ret" } */ + +volatile int zero = 0; +int global = 0; + +__attribute__((noinline)) +int bar(void) +{ + if (zero == 0) return 3; + return 0; +} + +__attribute__((noinline, noreturn)) +void unwind (void) +{ + throw 42; +} + +__attribute__((noinline, noipa, target("branch-protection=pac-ret"))) +int test(int x) +{ + if (x==1) return 2; /* This return path may not use the stack. */ + int y = bar(); + if (y > global) global=y; + if (y==3) unwind(); /* This return path must have RA mangle state set. */ + return 0; +} + +int main () +{ + try { + test (zero); + __builtin_abort (); + } catch (...) { + return 0; + } + __builtin_abort (); +}