[PATCH 0/6] Fix unwinding through sigreturn trampolines

[Ard Biesheuvel]

On Tue, 23 Jun 2020 at 11:54, Dave Martin <Dave.Martin at arm.com> wrote:
>
> On Tue, Jun 23, 2020 at 09:54:30AM +0100, Will Deacon wrote:
> > Hi all,
> >
> > This series fixes unwinding through our 64-bit sigreturn trampoline (by
> > effectively removing the incomplete CFI support enabled during the merge
> > window) and the 32-bit sigreturn trampoline in the compat vDSO (by removing
> > it in favour of the sigpage). This forces the unwinder to fallback on the
> > heuristics it was using previously without any reported issues.
> >
> > The downside is that this series undoes the LLVM unwinder support added
> > during the merge window, in favour of not regressing libgcc. Although Ard
> > and I tried very hard to make this all work with CFI, it became obvious
> > that it's not 5.8 material, particularly as testing this stuff is extremely
> > difficult and appears to be fragile on other architectures too. For example,
> > trying to unwind out of a SIGCANCEL handler triggered from within a leaf
> > function with pthread cleanup handlers stacked further up in the callchain
> > doesn't appear to work at all when compiling with -fexceptions, and works
> > partially (invoking a subset of the handlers...) if you reduce the
> > optimisation level.
> >
> > The compat vDSO changes are a result of me auditing our 32-bit sigreturn,
> > realising the compat vDSO trampoline is broken and then deciding that we
> > may as well just use the sigpage unconditionally, like arch/arm/ does.
> >
> > Thanks to Ard for wasting a significant chunk of his weekend on this mess.
> >
> > Patches 1-4 are targetting 5.8, the remaining two are cosmetic.
>
> In the longer term, we should perhaps keep .cfi_signal_frame, and ditch
> all the other annotations.  If the unwinder knows for sure it's the
> signal frame (and understands this annotation), it can use its own
> knowledge to do the right thing.  Otherwise, it can fall back on its
> other heuristics.
>

What we found out is that libunwind unconditionally uses its own
unwind strategy when it spots the mov/svc instruction pair at the
return address, and so what we put here does not matter at all.
The libgcc case is different, as it only applies the heuristics if it
doesn't find any unwind info covering the return address. So this is
what bit us here: by fixing the way the CFI directives are emitted,
libgcc no longer uses its own baked in unwind strategy for the signal
frame, but interprets the one we provide, which results in a bogus
address for the /next/ frame. Since that is not covered by unwind info
either, libgcc attempts to apply the heuristics at this level, and
explodes because the bogus address cannot be dereferenced to get at
the opcodes.

So libgcc doesn't even check whether it looks like a signal frame
unless the unwind info is missing.

> This is probably better than having rich annotations that are
> pedantically correct, but only half-supported by unwinders out in the
> wild.
>

Agreed. But without providing any annotations, other unwinders will
fail to unwind through the sigreturn trampoline, which is why the
change was made in the first place.

So the safest way to fix this (imho) is to add CFI directives that
mirror what libunwind does unconditionally, and libgcc when it spots
the mov/svc pair on unannotated addresses, which is to restore all the
registers from the sigframe, and to use regs->pc as the return address
(which is where the unwind should proceed, rather than at whatever
value happened to reside in x30 at the point the signal was taken)

The only hairy bit is the FP/SIMD registers, which are ignored by
libunwind, and only half-restored by libgcc (which appears to assume
that only d8..d15 need to be restored). This means throwing a C++
exception in a signal handler and catching it outside of it is going
to be difficult to get 100% correct, but this is something that
appears to be broken already everywhere. (Unless there are some
AAPCS/ABI rules that specify that FP/SIMD registers have to be assumed
clobbered when catching an exception)