Bug 94515 - aarch64: broken unwind information for pac-ret
Summary: aarch64: broken unwind information for pac-ret
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 10.0
: P3 normal
Target Milestone: ---
Assignee: nsz
URL:
Keywords: EH, wrong-code
Depends on: 94514
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-07 11:48 UTC by nsz
Modified: 2020-05-14 15:52 UTC (History)
0 users

See Also:
Host:
Target: aarch64*
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description nsz 2020-04-07 11:48:21 UTC
pac-ret uses the .cfi_window_save directive to toggle between signed/unsigned
return address, alternatively .cfi_remember_state and .cfi_restore_state pair
can be used to keep track of the "return address signedness" state.

in some cases, when there are several return paths, gcc fails to generate
the correct cfi directives for all return paths which can cause the unwinder
not to authenticate a signed return address leading to a runtime crash on
pauth enabled systems.

example c++ test that segfaults (after fixing bug 94514 ):

volatile int zero = 0;

__attribute__((noinline))
void unwind (void)
{
  if (zero == 0)
    throw 42;
}

__attribute__((noinline,noipa))
static int test (int z)
{
  if (z) {
    asm volatile("":::"x20","x21");
    unwind();
    return 1;
  } else {
    unwind();
    return 2;
  }
}

int main ()
{
  try {
    test (zero);
    __builtin_abort ();
  } catch (...) {
    return 0;
  }
  __builtin_abort ();
}

the test() function with -mbranch-protection=standard -O2 compiles to

_ZL4testi:
.LFB1:
        .cfi_startproc
        hint    25 // paciasp
        .cfi_window_save  // pauth on
        stp     x29, x30, [sp, -32]!
        .cfi_def_cfa_offset 32
        .cfi_offset 29, -32
        .cfi_offset 30, -24
        mov     x29, sp
        cbz     w0, .L9
        stp     x20, x21, [sp, 16]
        .cfi_offset 21, -8
        .cfi_offset 20, -16
        bl      _Z6unwindv
        mov     w0, 1
        ldp     x20, x21, [sp, 16]
        .cfi_restore 21
        .cfi_restore 20
        ldp     x29, x30, [sp], 32
        .cfi_restore 30
        .cfi_restore 29
        .cfi_def_cfa_offset 0
        hint    29 // autiasp
        .cfi_window_save  // pauth off
        ret
        .p2align 2,,3
.L9:
        //// ret addr pauth state is wrong here !
        .cfi_def_cfa_offset 32
        .cfi_offset 29, -32
        .cfi_offset 30, -24
        bl      _Z6unwindv
        ldp     x29, x30, [sp], 32
        .cfi_restore 30
        .cfi_restore 29
        .cfi_def_cfa_offset 0
        hint    29 // autiasp
        .cfi_window_save
        mov     w0, 2
        ret
        .cfi_endproc
.LFE1:
        .size   _ZL4testi, .-_ZL4testi
Comment 1 nsz 2020-04-21 14:26:42 UTC
i had a fix but it's not enough, so here is another test case:

__attribute__((noreturn)) void unwind(void);
int bar(void);
int global;

int foo(int x)
{
  if (x==1) return 2;
  int y = bar();
  if (y > global) global=y;
  if (y==3) unwind();
  return 0;
}

-O2 -S -mbranch-protection=pac-ret the asm:

foo:
        .cfi_startproc
        cmp     w0, 1
        beq     .L4
        hint    25 // paciasp
        .cfi_window_save  //// pauth on
        stp     x29, x30, [sp, -16]!
        .cfi_def_cfa_offset 16
        .cfi_offset 29, -16
        .cfi_offset 30, -8
        mov     x29, sp
        bl      bar
        mov     w1, w0
        adrp    x2, .LANCHOR0
        ldr     w0, [x2, #:lo12:.LANCHOR0]
        cmp     w0, w1
        blt     .L11
.L3:
        mov     w0, 0
        cmp     w1, 3
        beq     .L12
        ldp     x29, x30, [sp], 16
        .cfi_remember_state
        .cfi_restore 30
        .cfi_restore 29
        .cfi_def_cfa_offset 0
        hint    29 // autiasp
        .cfi_window_save  //// pauth off
        ret
        .p2align 2,,3
.L11:
        .cfi_restore_state  //// pauth on
        str     w1, [x2, #:lo12:.LANCHOR0]
        b       .L3
        .p2align 2,,3
.L4:
        .cfi_def_cfa_offset 0
        .cfi_restore 29
        .cfi_restore 30
        mov     w0, 2  //// pauth should be off but it's on 
        ret
.L12:
        .cfi_def_cfa_offset 16
        .cfi_offset 29, -16
        .cfi_offset 30, -8
        bl      unwind
        .cfi_endproc
Comment 2 GCC Commits 2020-04-27 08:11:27 UTC
The master branch has been updated by Szabolcs Nagy <nsz@gcc.gnu.org>:

https://gcc.gnu.org/g:acdf733634745548c0167c40bad80e6140ac2eeb

commit r10-7986-gacdf733634745548c0167c40bad80e6140ac2eeb
Author: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date:   Mon Apr 27 09:07:15 2020 +0100

    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 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.
    
    This should be backported to gcc-9 and gcc-8 branches.
    
    gcc/ChangeLog:
    
            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:
    
            PR target/94515
            * g++.target/aarch64/pr94515-1.C: New test.
            * g++.target/aarch64/pr94515-2.C: New test.
Comment 3 GCC Commits 2020-05-14 15:17:52 UTC
The releases/gcc-9 branch has been updated by Szabolcs Nagy <nsz@gcc.gnu.org>:

https://gcc.gnu.org/g:95833c34424f340a7e465ee38b6a41369bc7c90b

commit r9-8593-g95833c34424f340a7e465ee38b6a41369bc7c90b
Author: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date:   Mon Apr 27 09:07:15 2020 +0100

    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 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.
    
    This should be backported to gcc-9 and gcc-8 branches.
    
    gcc/ChangeLog:
    
            Backport from mainline.
            2020-04-27  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:
    
            Backport from mainline.
            2020-04-27  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.
Comment 4 GCC Commits 2020-05-14 15:48:10 UTC
The releases/gcc-8 branch has been updated by Szabolcs Nagy <nsz@gcc.gnu.org>:

https://gcc.gnu.org/g:62ab8b9114b0bdae508ed76fa9028e0040d35e6b

commit r8-10253-g62ab8b9114b0bdae508ed76fa9028e0040d35e6b
Author: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date:   Mon Apr 27 09:07:15 2020 +0100

    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 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.
    
    This should be backported to gcc-9 and gcc-8 branches.
    
    Changed branch-protection=pac-ret to sign-return-address=all etc
    in the tests for the backport and adjusted the dwarf2cfi.c
    changes because the sparc change was missing.
    
    gcc/ChangeLog:
    
            Backport from mainline.
            2020-04-27  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:
    
            Backport from mainline.
            2020-04-27  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.
Comment 5 nsz 2020-05-14 15:52:01 UTC
fixed for gcc-10.1 and on gcc-9 and gcc-8 branches.