Skip to content
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

Document ARM64 unwind handling for return address signing #4202

Merged
merged 2 commits into from
Oct 11, 2022

Conversation

mstorsjo
Copy link
Contributor

@mstorsjo mstorsjo commented Oct 4, 2022

This is deduced/guessed from studying the output of MSVC with the /d2guardsignret flag. Hopefully someone with insight into the unwinder internals can confirm whether this indeed is the case.

I made up an unwind opcode name for this opcode (0xfc, sign_ra) - other suggestions are welcome.

I'm not entirely sure about whether this is how it is handled for the packed functions, but this would intuitively be how it works.

Since there are two parallel instructions for signing return addresses, paciasp and pacibsp, the unwinder would need to know which one to use for signing - MSVC seems to generate pacibsp (togeter with autibsp) - is there any other similar opcode for paciasp, or is that one unsupported for Windows unwinding purposes?

@prmerger-automator
Copy link
Contributor

@mstorsjo : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@opbld30
Copy link

opbld30 commented Oct 4, 2022

Learn Build status updates of commit 21e4902:

✅ Validation status: passed

File Status Preview URL Details
docs/build/arm64-exception-handling.md ✅Succeeded

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@Court72
Copy link
Contributor

Court72 commented Oct 4, 2022

@corob-msft

Can you review the proposed changes? IMPORTANT: When the changes are ready for publication, add a #sign-off comment to signal that the PR is ready for the review team to merge.

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

@prmerger-automator prmerger-automator bot added the aq-pr-triaged Tracking label for the PR review team label Oct 4, 2022
@colin-home
Copy link
Contributor

colin-home commented Oct 6, 2022

@mstorsjo

Thanks for the proposal. As near as I can tell, the actual opcode is known as pac_sign_return_address, and the logic is largely as you've divined. There appears to be a flag that lets the signing/verifying opcodes be replaced with nops, so there's an additional bit of complexity involved somewhere. At least, I think so, if I'm reading things correctly, which is always a question. The signing step is "Step 1" in the internal comments, and saving registers is "Step 2", so we should probably renumber those and the following steps accordingly.

To get the inside story, I'm invoking @laurenprinn and @sigatrev for their takes. (I'd invoke James McNellis, but he's moved on to smaller and more playful things at Roblox.) I don't feel qualified to sign off on my own.

@prmerger-automator
Copy link
Contributor

@mstorsjo : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@mstorsjo
Copy link
Contributor Author

mstorsjo commented Oct 6, 2022

Thanks for the proposal. As near as I can tell, the actual opcode is known as pac_sign_return_address, and the logic is largely as you've divined. There appears to be a flag that lets the signing/verifying opcodes be replaced with nops, so there's an additional bit of complexity involved somewhere. At least, I think so, if I'm reading things correctly, which is always a question. The signing step is "Step 1" in the internal comments, and saving registers is "Step 2", so we should probably renumber those and the following steps accordingly.

Thanks! I've updated the PR to use the correct name for the opcode, and renumbered the steps for the packed form.

@opbld33
Copy link

opbld33 commented Oct 6, 2022

Learn Build status updates of commit 1887d8c:

✅ Validation status: passed

File Status Preview URL Details
docs/build/arm64-exception-handling.md ✅Succeeded

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

Copy link
Contributor

@sigatrev sigatrev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The x30/lr inconsistency should be fixed, but this otherwise looks good to me.

docs/build/arm64-exception-handling.md Outdated Show resolved Hide resolved
@prmerger-automator
Copy link
Contributor

@mstorsjo : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@opbld33
Copy link

opbld33 commented Oct 10, 2022

Learn Build status updates of commit f510c83:

✅ Validation status: passed

File Status Preview URL Details
docs/build/arm64-exception-handling.md ✅Succeeded

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@mstorsjo
Copy link
Contributor Author

mstorsjo commented Oct 10, 2022

@sigatrev BTW, are you able to comment on what the Windows unwinder implementation does, if the actual code does use PAC (and running on HW that exposes it, with a Windows configuration that has taken it into use), but the unwind info is lacking these opcodes? (That's the current status quo if building code with Clang today, with -mbranch-protection=standard.) I.e., does the unwinder implicitly run xpaci on the return address after finishing RtlVirtualUnwind, or does it only do that (or similar) if explicitly told so by an unwinding opcode?

@pmsjt
Copy link
Contributor

pmsjt commented Oct 10, 2022

@sigatrev BTW, are you able to comment on what the Windows unwinder implementation does, if the actual code does use PAC (and running on HW that exposes it, with a Windows configuration that has taken it into use), but the unwind info is lacking these opcodes? (That's the current status quo if building code with Clang today, with -mbranch-protection=standard.) I.e., does the unwinder implicitly run xpaci on the return address after finishing RtlVirtualUnwind, or does it only do that (or similar) if explicitly told so by an unwinding opcode?

It must be explicitly present.

@mstorsjo
Copy link
Contributor Author

@sigatrev BTW, are you able to comment on what the Windows unwinder implementation does, if the actual code does use PAC (and running on HW that exposes it, with a Windows configuration that has taken it into use), but the unwind info is lacking these opcodes? (That's the current status quo if building code with Clang today, with -mbranch-protection=standard.) I.e., does the unwinder implicitly run xpaci on the return address after finishing RtlVirtualUnwind, or does it only do that (or similar) if explicitly told so by an unwinding opcode?

It must be explicitly present.

Ok, thanks for the clarification!

@prmerger-automator
Copy link
Contributor

@corob-msft : Thanks for your contribution! The author(s) have been notified to review your proposed change.

Copy link
Contributor

@colin-home colin-home left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mstorsjo
Thanks for the useful update.

@opbld30
Copy link

opbld30 commented Oct 11, 2022

Learn Build status updates of commit 4547444:

✅ Validation status: passed

File Status Preview URL Details
docs/build/arm64-exception-handling.md ✅Succeeded

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@colin-home
Copy link
Contributor

#sign-off

@colin-home colin-home merged commit 8e6bd58 into MicrosoftDocs:main Oct 11, 2022
@mstorsjo mstorsjo deleted the arm64-unwind-pac branch October 11, 2022 08:14
mstorsjo added a commit to llvm/llvm-project that referenced this pull request Oct 11, 2022
mstorsjo added a commit to llvm/llvm-project that referenced this pull request Oct 12, 2022
mstorsjo added a commit to llvm/llvm-project that referenced this pull request Oct 12, 2022
This new opcode was initially documented as "pac_sign_return_address"
in MicrosoftDocs/cpp-docs#4202, but was
soon afterwards renamed into "pac_sign_lr" in
MicrosoftDocs/cpp-docs#4209, as the other
name was unwieldy, and there were no other external references to
that name anywhere.

Rename our external .seh assembler directive - it hasn't been merged
for very long yet, so there's probably no external use to account for.

Rename all other internal references to the opcode similarly.

Differential Revision: https://reviews.llvm.org/D135762
veselypeta pushed a commit to veselypeta/cherillvm that referenced this pull request May 27, 2024
veselypeta pushed a commit to veselypeta/cherillvm that referenced this pull request May 28, 2024
veselypeta pushed a commit to veselypeta/cherillvm that referenced this pull request May 28, 2024
This new opcode was initially documented as "pac_sign_return_address"
in MicrosoftDocs/cpp-docs#4202, but was
soon afterwards renamed into "pac_sign_lr" in
MicrosoftDocs/cpp-docs#4209, as the other
name was unwieldy, and there were no other external references to
that name anywhere.

Rename our external .seh assembler directive - it hasn't been merged
for very long yet, so there's probably no external use to account for.

Rename all other internal references to the opcode similarly.

Differential Revision: https://reviews.llvm.org/D135762
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants