[PATCH 0/6] Fix unwinding through sigreturn trampolines

[Will Deacon]

On Tue, Jun 23, 2020 at 12:11:24PM +0200, Ard Biesheuvel wrote:
> On Tue, 23 Jun 2020 at 11:54, Dave Martin <Dave.Martin at arm.com> wrote:
> > 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)

I think Dave makes an interesting point about .cfi_signal_frame, though.
With any released arm64 kernel, unwinding here requires heuristics to
identify the signal frame. The usual approach seems to pattern match the
instruction stream (and I'm not convinced libunwind works on big-endian
here), but you could also have looked up pc+4 to see if the function was
marked with .cfi_signal_frame. You can't do that anymore after this patch.
We have no evidence anybody bothered with that (it seems like more trouble
than it's worth), but it's an interesting observation. Furthermore, maybe
the right answer *is* to punt this all to the unwinder given that we're
already forced to keep the instruction sequence exactly as it is. I suspect
the LLVM unwinder has to use heuristics for most other architectures
anyway, so it should already have whatever hooks it needs.

It would be a different story if most unwinders were expecting to use
CFI and it generally worked well on other architectures, but that really
doesn't appear to be the case.

Will