-
Notifications
You must be signed in to change notification settings - Fork 288
Add support for aarch64-unknown-linux-pauthtest
#755
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jchlanda
wants to merge
1
commit into
rust-lang:master
Choose a base branch
from
jchlanda:jakub/pauthtest
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+26
−1
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is
ipnot signed? It is directly the result of_Unwind_GetIP. It should be possible to pass that to_Unwind_FindEnclosingFunction. How else would you even be able to use_Unwind_FindEnclosingFunctionotherwise?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A raw
ipis expected._Unwind_GetIPauthenticates the pointer and returns a stripped version (viaptrauth_auth_datahere).This is the crux of the issue.
_Unwind_FindEnclosingFunctionis fundamentally incompatible with the pointer authentication model. The problem is that_Unwind_FindEnclosingFunctiondoes not follow PAC semantics, it blindly creates a fresh unwind cursor and then resets the instruction pointer here:__unw_set_reg(cursor, UNW_REG_IP, pc);. As if to say: "treat thispcas if it belonged to this new cursor's frame".__unw_set_regdoes apply PAC logic internally (authenticates) under the assumption that theipbehaves like a return address for the current frame.This cannot be fixed on the Rust side.
_Unwind_FindEnclosingFunctionconstructs a new cursor (and therefore a newsp), so there is no way to correctly sign pointer: any signing we would perform would use the wrong context and would still fail authentication inside libunwind.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a bug in libunwind to me. What purpose does
_Unwind_FindEnclosingFunctionhave at all if you can't pass the result of_Unwind_GetIPto it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, the documentation seems to be pretty scarce, and one I found is rather quite lax about possible outcomes of the call, explicitly allowing failure.
I personally don't read it as a bug. To me it is more that
_Unwind_FindEnclosingFunctionwas designed before pointer authentication existed, and its API implicitly assumes that instruction pointers are context-free values. But in PAC-aware code PC is only meaningful in the context of the (original) SP.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does
_Unwind_FindEnclosingFunctionneed an authenticated pointer? It doesn't dereference the pointer, only looks it up in a side table with the result not having any security relevance. The only useful thing you can do with it afaik is looking up debuginfo for the function. Due to inlining and outlining you can't guarantee that it points to any particular function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if my description of why
uw::_Unwind_FindEnclosingFunctionis incompatible with pointer authentication is insufficient. Alternatively, if you have any ideas on how it should be addressed, I would be very keen to hear them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjorn3 If you have any questions related to how pointer authentication is working in terms of libunwind, I would be happy to answer - I was previously working on that from Linux perspective, while most work was done by folks from Apple.
I confirm @jchlanda 's reasoning in the thread above and I think that it should be good enough to just return
self.ip()for pauthtest target.Also note that the signing/auehtntication behavior from libunwind is mostly shared between Apple's arm64e and Linux's pauthtest (when not platform-specific, e.g. personality pointers are stored and signed differently on Darwin and Linux). So, while I do not believe that we have any bug in libunwind's behavior and API design, even if we do - it would require a big discussion involving folks from Apple. And at this point, it's probably better just to conform to status-quo here in backtrace-rs.
So, we would very much appreciate your feedback on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using
self.ip()here will break the internal frame omission for backtraces:backtrace-rs/src/capture.rs
Lines 300 to 303 in 06b2cc5
And in tokio: https://github.com/tokio-rs/tokio/blob/dac81bf8c8de0a3e35f1626643674ba9faf9569c/tokio/src/runtime/task/trace/trace_impl.rs#L16
I think it would also effectively break the scoped-trace crate: https://github.com/jswrenn/scoped-trace/blob/e299c46905a2063957414c34d76264281852461b/src/lib.rs#L126
Is there really no way to retain this support? Even if just stripping the pointer signature?
_Unwind_FindEnclosingFunctioncan be fixed to accept arbitrary pointer signatures without compromising security, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will need to dig into it again to provide a more detailed answer, but for now, a couple of quick observations:
self.ip()is exactly what happens on Apple, so the precedence is already there;uw::_Unwind_FindEnclosingFunction(self.ip())) causes every program compiled foraarch64-unknown-linux-pauthtestthat relies on backtrace-rs to encounter a pointer authentication failure. As a first approximation, that's every program that panics.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjorn3 I spent some time looking into this issue again.
To sum up, there are two stages:
This is done with
_Unwind_GetIP(context). On PAC-enabled AArch64, libunwind authenticates the stored PC and returns an instruction pointer value associated with the original frame context (including the frame's SP-based PAC discriminator). That value is what we keep inFrame::ip()._Unwind_FindEnclosingFunction.The expectation is that libunwind returns the start address of the enclosing function. This call is implemented as:
The root cause of the issue is in this snippet: the cursor is not the original frame that produced
pc. It is a brand new unwind cursor initialized from the current thread context.On PAC-enabled AArch64, the instruction pointer returned by
_Unwind_GetIP()is not simply an arbitrary integer address. Although it is suitable for use as a code address by callers, it is still a value whose PAC validity depends on the original frame context (in particular, the SP discriminator used by the PAC scheme). When_Unwind_FindEnclosingFunction()imports this IP into its newly created cursor, it goes through__unw_set_reg(), which calls the AArch64 implementation ofRegisters_arm64::setIP(value):getSP()here returns the SP stored in the newly created cursor. It is not the SP belonging to the frame from which_Unwind_GetIP()obtained the instruction pointer. Therefore,setIP()attempts to authenticate an IP that belongs to the original frame using the SP of a different frame. The authentication step fails because the PAC discriminator does not match; the issue is not that the IP lacks a signature, but that it is being reintroduced into a different unwind context.Not from backtrace-rs, as far as I can see. By the time we call
_Unwind_FindEnclosingFunction, all we have is the instruction pointer returned by_Unwind_GetIP()._Unwind_FindEnclosingFunctionimmediately constructs its own unwind cursor and, as we know now, this must fail authentication - since the original SP is no longer available. And because that cursor is created entirely inside libunwind, backtrace-rs has no opportunity to provide the original SP that was associated with the instruction pointer returned by_Unwind_GetIP(). From our side, there doesn't appear to be any way to make this call succeed._Unwind_GetIP()already returns the public instruction pointer representation expected by callers. The problem is not that backtrace-rs passes an unstripped/signed pointer incorrectly; the problem is that_Unwind_FindEnclosingFunction()tries to import that pointer into a new unwind context with a different SP, eventually callsRegisters_arm64::setIP(), which authenticates/re-signs the supplied value using the SP of the newly-created cursor.I think it should be possible. I earlier said that
_Unwind_FindEnclosingFunctionis fundamentally incompatible with PAC enabled arch, that is probably not fully correct, I should have said: the current implementation in LLVM's libunwind doesn't work for this use case.For example, if libunwind implemented
_Unwind_FindEnclosingFunctionwithout synthesizing a fresh unwind cursor, or otherwise avoided routing throughRegisters_arm64::setIP(), then the problem would not manifest. I am not in position to say if either of those is possible to implement.And finally, from the perspective of backtrace-rs, the current implementation aborts on
aarch64-unknown-linux-pauthtest, and we don't have enough information (most notably the original frame's SP) to make this call succeed with the current API.Therefore, the only implementation that works reliably for pauthtest today is to use the instruction pointer already returned by
_Unwind_GetIP()(self.ip()), which is also what backtrace-rs already does on Apple targets.