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).
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?
>
> > > 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
> >
>
--
