Yury Khrustalev <yury.khrusta...@arm.com> writes: > From: Szabolcs Nagy <szabolcs.n...@arm.com> > > Follows the current linux ABI that uses single signal entry token > and shared shadow stack between thread and alt stack. > Could be behind __ARM_FEATURE_GCS_DEFAULT ifdef (only do anything > special with gcs compat codegen) but there is a runtime check anyway. > > Change affected tests to be compatible with -mbranch-protection=standard > > gcc/testsuite/ChangeLog: > > * g++.target/aarch64/pr94515-1.C (f1_no_pac_ret): Update. > (main): Update. > Co-authored-by: Matthieu Longo <matthieu.lo...@arm.com> > > * gcc.target/aarch64/pr104689.c (unwind): Update. > Co-authored-by: Matthieu Longo <matthieu.lo...@arm.com>
Too many Co-authored-bys :) The one below is indented correctly. > libgcc/ChangeLog: > > * config/aarch64/aarch64-unwind.h (_Unwind_Frames_Extra): Update. > (_Unwind_Frames_Increment): Define. > > Co-authored-by: Matthieu Longo <matthieu.lo...@arm.com> > --- > gcc/testsuite/g++.target/aarch64/pr94515-1.C | 6 +- > gcc/testsuite/gcc.target/aarch64/pr104689.c | 3 +- > libgcc/config/aarch64/aarch64-unwind.h | 59 +++++++++++++++++++- > 3 files changed, 63 insertions(+), 5 deletions(-) > > diff --git a/gcc/testsuite/g++.target/aarch64/pr94515-1.C > b/gcc/testsuite/g++.target/aarch64/pr94515-1.C > index 359039e1753..8175ea50c32 100644 > --- a/gcc/testsuite/g++.target/aarch64/pr94515-1.C > +++ b/gcc/testsuite/g++.target/aarch64/pr94515-1.C > @@ -5,7 +5,7 @@ > > volatile int zero = 0; > > -__attribute__((noinline, target("branch-protection=none"))) > +__attribute__((noinline, target("branch-protection=bti"))) > void unwind (void) > { > if (zero == 0) > @@ -22,7 +22,7 @@ int test (int z) > // autiasp -> cfi_negate_ra_state: RA_signing_SP -> RA_no_signing > return 1; > } else { > - // 2nd cfi_negate_ra_state because the CFI directives are processed > linearily. > + // 2nd cfi_negate_ra_state because the CFI directives are processed > linearly. > // At this point, the unwinder would believe that the address is not > signed > // due to the previous return. That's why the compiler has to emit second > // cfi_negate_ra_state to mean that the return address is still signed. > @@ -33,7 +33,7 @@ int test (int z) > } > } > > -__attribute__((target("branch-protection=none"))) > +__attribute__((target("branch-protection=bti"))) > int main () > { > try { > diff --git a/gcc/testsuite/gcc.target/aarch64/pr104689.c > b/gcc/testsuite/gcc.target/aarch64/pr104689.c > index 3b7adbdfe7d..9688ecc85f9 100644 > --- a/gcc/testsuite/gcc.target/aarch64/pr104689.c > +++ b/gcc/testsuite/gcc.target/aarch64/pr104689.c > @@ -98,6 +98,7 @@ asm("" > "unusual_no_pac_ret:\n" > " .cfi_startproc\n" > " " SET_RA_STATE_0 "\n" > +" bti c\n" > " stp x29, x30, [sp, -16]!\n" > " .cfi_def_cfa_offset 16\n" > " .cfi_offset 29, -16\n" > @@ -121,7 +122,7 @@ static void f2_pac_ret (void) > die (); > } > > -__attribute__((target("branch-protection=none"))) > +__attribute__((target("branch-protection=bti"))) > static void f1_no_pac_ret (void) > { > unusual_pac_ret (f2_pac_ret); Could you explain these testsuite changes in more detail? It seems on the face of it that they're changing the tests to test something other than the original intention. Having new tests alongside the same lines would be fine though. > diff --git a/libgcc/config/aarch64/aarch64-unwind.h > b/libgcc/config/aarch64/aarch64-unwind.h > index 4d36f0b26f7..cf4ec749c05 100644 > --- a/libgcc/config/aarch64/aarch64-unwind.h > +++ b/libgcc/config/aarch64/aarch64-unwind.h > @@ -178,6 +178,9 @@ aarch64_demangle_return_addr (struct _Unwind_Context > *context, > return addr; > } > > +/* GCS enable flag for chkfeat instruction. */ > +#define CHKFEAT_GCS 1 > + > /* SME runtime function local to libgcc, streaming compatible > and preserves more registers than the base PCS requires, but > we don't rely on that here. */ > @@ -185,12 +188,66 @@ __attribute__ ((visibility ("hidden"))) > void __libgcc_arm_za_disable (void); > > /* Disable the SME ZA state in case an unwound frame used the ZA > - lazy saving scheme. */ > + lazy saving scheme. And unwind the GCS for EH. */ > #undef _Unwind_Frames_Extra > #define _Unwind_Frames_Extra(x) \ > do \ > { \ > __libgcc_arm_za_disable (); \ > + if (__builtin_aarch64_chkfeat (CHKFEAT_GCS) == 0) \ > + { \ > + for (_Unwind_Word n = (x); n != 0; n--) \ > + __builtin_aarch64_gcspopm (); \ > + } \ > + } \ > + while (0) > + > +/* On signal entry the OS places a token on the GCS that can be used to > + verify the integrity of the GCS pointer on signal return. It also > + places the signal handler return address (the restorer that calls the > + signal return syscall) on the GCS so the handler can return. > + Because of this token, each stack frame visited during unwinding has > + exactly one corresponding entry on the GCS, so the frame count is > + the number of entries that will have to be popped at EH return time. > + > + Note: This depends on the GCS signal ABI of the OS. > + > + When unwinding across a stack frame for each frame the corresponding > + entry is checked on the GCS against the computed return address from > + the normal stack. If they don't match then _URC_FATAL_PHASE2_ERROR > + is returned. This check is omitted if > + > + 1. GCS is disabled. Note: asynchronous GCS disable is supported here > + if GCSPR and the GCS remains readable. > + 2. Non-catchable exception where exception_class == 0. Note: the > + pthread cancellation implementation in glibc sets exception_class > + to 0 when the unwinder is used for cancellation cleanup handling, > + so this allows the GCS to get out of sync during cancellation. > + This weakens security but avoids an ABI break in glibc. > + 3. Zero return address which marks the outermost stack frame. I suppose this is a question for the x86 implementation too, but: doesn't this weaken the checks somewhat? I would imagine zero is the easiest value to force. Would it be possible to add some extra sanity check that this really is the outermost frame? E.g. IIUC, the entry above the outermost frame's would be a cap, or at least would not be a valid procedure return record, so could we check for that? Thanks, Richard > + 4. Signal stack frame, the GCS entry is an OS specific token then > + with the top bit set. > + */ > +#undef _Unwind_Frames_Increment > +#define _Unwind_Frames_Increment(exc, context, frames) \ > + do \ > + { \ > + frames++; \ > + if (__builtin_aarch64_chkfeat (CHKFEAT_GCS) != 0 \ > + || exc->exception_class == 0 \ > + || _Unwind_GetIP (context) == 0) \ > + break; \ > + const _Unwind_Word *gcs = __builtin_aarch64_gcspr (); \ > + if (_Unwind_IsSignalFrame (context)) \ > + { \ > + if (gcs[frames] >> 63 == 0) \ > + return _URC_FATAL_PHASE2_ERROR; \ > + } \ > + else \ > + { \ > + if (gcs[frames] != _Unwind_GetIP (context)) \ > + return _URC_FATAL_PHASE2_ERROR; \ > + } \ > } \ > while (0)