X Tutup
The Wayback Machine - https://web.archive.org/web/20250613203507/https://github.com/python/cpython/pull/94472
Skip to content

gh-94471: Enable pointer authentication on aarch64 builds #94472

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

Closed
wants to merge 4 commits into from

Conversation

jgowdy
Copy link

@jgowdy jgowdy commented Jul 1, 2022

ARM64v8.3 supports Pointer Authentication with the PACIASP and AUTIASP instructions which are interpreted as NOP instructions on pre-8.3 architectures. These instructions sign the stack pointer and validate the stack pointer prior to return to mitigate return oriented programming.

GCC supports these options on arm64 / aarch64. The legacy option was -msign-return-address=[all | non-leaf | none] and the modern option is -mbranch-protection=none|standard|pac-ret[+leaf+b-key]|bti

I would like to suggest that the arm64 build be modified to include -mbranch-protection=pac-ret with the -march being set to ARMv8.2 or earlier or not configured, so that GCC will generate PACIASP and AUTIASP instructions. It is critical that -march=armv8.3 or higher not be passed or the non-backwards compatible RETAA instruction will be generated.

The benefit of enabling pointer authentication for the stack pointer on ARM64 would be to mitigate return oriented programming attacks against the CPython runtime.

Presently we (GoDaddy) are pursuing custom compiles of the CPython runtime for the new Graviton3 CPUs that support pointer authentication in AWS.

@ghost
Copy link

ghost commented Jul 1, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@jgowdy
Copy link
Author

jgowdy commented Jul 1, 2022

I'll need to regenerate configure with the appropriate version of autoconf

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jul 1, 2022

I'll need to regenerate configure with the appropriate version of autoconf

docker run --rm --pull=always -v"$PWD:/src" http://quay.io/tiran/cpython_autoconf:269

@erlend-aasland erlend-aasland self-requested a review July 1, 2022 23:07
@jgowdy
Copy link
Author

jgowdy commented Aug 22, 2022

@erlend-aasland Looks green 👍🏻

@jgowdy
Copy link
Author

jgowdy commented Aug 30, 2022

I've attempted to simplify the M4sh here to only need AS_CASE and AX_CHECK_COMPILE_FLAG. Is this suitable?

@erlend-aasland
Copy link
Contributor

Thanks. The AC change looks good to me. I have no opinions regarding the arch specifics here. Perhaps @corona10 or @tiran can chime in for that?

@tiran
Copy link
Member

tiran commented Aug 30, 2022

Are there potential performance issues or backwards compatibility issues with -mbranch-protection?

-msign-return-address is a legacy option. It looks like GCC 9 and newer have -mbranch-protection. GCC 9 or newer are available for most distros, even RHEL 7 with DevTool set.

@jgowdy
Copy link
Author

jgowdy commented Aug 31, 2022

Are there potential performance issues or backwards compatibility issues with -mbranch-protection?

There is a minor performance impact related to the CPU calculating the signed pointer, but it's on the order of the CPU doing math in hardware.

-msign-return-address is a legacy option. It looks like GCC 9 and newer have -mbranch-protection. GCC 9 or newer are available for most distros, even RHEL 7 with DevTool set.

Yes, sign-return-address is a legacy option. Do you think we need to exclude the legacy option? I included it for completeness.

@jgowdy
Copy link
Author

jgowdy commented Oct 24, 2022

Are there potential performance issues or backwards compatibility issues with -mbranch-protection?

I should have also stated, there isn't a backwards compatibility issue here since the instructions are implemented in the NOP space for prior architectures.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM, but I would like @corona10 or another core dev to approve before merging.

@corona10
Copy link
Member

@erlend-aasland I will take a look at this PR, I may need to understand potential impact with this patch :)

@corona10 corona10 self-assigned this Oct 26, 2022
@corona10
Copy link
Member

corona10 commented Oct 28, 2022

@pablogsal cc @erlend-aasland
Is this patch will affect trampoline implementation?
Would you like to check the impact please?

#if defined(__aarch64__) && defined(__AARCH64EL__) && !defined(__ILP32__)
// ARM64 little endian, 64bit ABI
// generate with aarch64-linux-gnu-gcc 12.1
stp x29, x30, [sp, -16]!
mov x29, sp
blr x3
ldp x29, x30, [sp], 16
ret
#endif

@jgowdy Would you like to submit the pyperformance benchmark result comparing the result between
non-pointer authentication and pointer authentication?

@pablogsal
Copy link
Member

@pablogsal cc @erlend-aasland

Is this patch will affect trampoline implementation?

It has potential to, specifically it may affect unwinders (including perf), debuggers and state inspection tools. We should check if the main unwinders know how to handle these pointers correctly.

@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 28, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit f08af17 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 28, 2022
@pablogsal
Copy link
Member

I'm also checking with the buildbot fleet.

@pablogsal
Copy link
Member

pablogsal commented Oct 28, 2022

From https://lore.kernel.org/linux-arm-kernel/Y1q914IVy6XgE1xq@hirez.programming.kicks-ass.net/t/ :

Perf report cannot produce callgraphs using dwarf on arm64 where pointer
authentication is enabled. This is because libunwind and libdw cannot
unmangle instruction pointers that have a pointer authentication code
(PAC) embedded in them.
libunwind and libdw need to be given an instruction mask which they can
use to arrive at the correct return address that does not contain the
PAC.

I think, if confirmed, this is a significant enough drawback that makes this a big blocker for merging this and having it active by default.

It seems that someone is working on this but this email chain is very recent so I doubt most perf/libunwind versions out there will know how to handle it.

In any case, we should run our own experiments to see how this affects these tools.

@erlend-aasland
Copy link
Contributor

Thanks, Dong-hee and Pablo! I'll mark this as do-not-merge, just to be on the safe side.

@pablogsal
Copy link
Member

This makes test_gdb fail on aarch64 RHEL buildbots, so this more or less confirms that this messes up with debuggers as I suspected.

@erlend-aasland
Copy link
Contributor

Given Dong-hee's and Pablo's valuable input on this, I believe we can conclude with that this feature is not something we can easily implement. Suggesting to close this PR and the linked issue.

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Oct 28, 2022
@pablogsal
Copy link
Member

Agreed, also this is something that redistributors can easily do by setting CFLAGS or similar on they custom builds if they are ok sacrificing debugging / profilings capabilities.

@erlend-aasland erlend-aasland removed the pending The issue will be closed if no feedback is provided label Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
X Tutup