Page MenuHomePhabricator

libunwind: Don't attempt to authenticate a null return address.
ClosedPublic

Authored by pcc on Feb 11 2021, 4:25 PM.

Details

Summary

Null return addresses can appear at the bottom of the stack (i.e. the
frame corresponding to the entry point). Authenticating these addresses
will set the error code in the address, which will lead to a segfault
in the sigreturn trampoline detection code. Fix this problem by not
authenticating null addresses.

Diff Detail

Event Timeline

pcc created this revision.Feb 11 2021, 4:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2021, 4:25 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
pcc requested review of this revision.Feb 11 2021, 4:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2021, 4:25 PM

It seems OK to me. Does it make sense to also skip (pc == 0) in UnwindCursor<A, R>::setInfoForSigReturn?

pcc added a comment.Feb 11 2021, 6:02 PM

It seems OK to me. Does it make sense to also skip (pc == 0) in UnwindCursor<A, R>::setInfoForSigReturn?

Hmm, that seems redundant with the check that I'm adding here. Let's start with one and we can reconsider if we find another bug here.

In D96560#2558737, @pcc wrote:

It seems OK to me. Does it make sense to also skip (pc == 0) in UnwindCursor<A, R>::setInfoForSigReturn?

Hmm, that seems redundant with the check that I'm adding here. Let's start with one and we can reconsider if we find another bug here.

Oh, yeah, it would be redundant with this check earlier in setInfoBasedOnIPRegister:

// Exit early if at the top of the stack.
if (pc == 0) {
  _unwindInfoMissing = true;
  return;
}
danielkiss accepted this revision.Feb 15 2021, 2:16 AM

LGTM, Thanks.

mstorsjo accepted this revision.Feb 15 2021, 2:24 AM
mstorsjo added a subscriber: mstorsjo.

LGTM too

This revision is now accepted and ready to land.Feb 15 2021, 2:24 AM
This revision was landed with ongoing or failed builds.Feb 16 2021, 11:18 AM
This revision was automatically updated to reflect the committed changes.