cc@fweimer-rh

In the current implementation the cf-protection test for x86_64 passes but not the branch-protection test for aarch64.

You should add PAC support at the same time, usinghint 25 (paciasp, replacesbti c) andhint 29 (autiasp). These instructions are in the NOP space, but some CPUs have trouble executing them. This applies tobit c as well. So it's best to make this conditional on compiler flags, by checking the__ARM_FEATURE_BTI_DEFAULT preprocessor macro. Then you can change the flag at the end of the note from1 to3, and this should make annocheck happy.

Rebased. The conditionals are a bit of a mess right now so I'll do some macros later on.

However annobin still reports:

Hardened: libpython3.12.so.1.0: FAIL: dynamic-tags test because the BTI_PLT flag is missing from the dynamic tags
Hardened: libpython3.12.so.1.0: FAIL: property-note test because properly formatted .note.gnu.property not found (it is needed for branch protection support)

Btw the arm blog proposes a different styled note than what the annobin docs mention:https://community.arm.com/arm-community-blogs/b/architectures-and-processors-blog/posts/p2-enabling-pac-and-bti-on-aarch64

Also regarding x86_64. Shouldn't the note be conditionalized in the case CET is active?

cc'ing also@pablogsal as the original author of the file.

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

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

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I am also not sure why this is needed other than make theannocheck tool happy so I am not sure we want to add this extra complexity.

The justification for this change is provided at the linked issue.

I am also not sure why this is needed other than make theannocheck tool happy so I am not sure we want to add this extra complexity.

The justification for this change is provided at the linked issue.

I don't think that justification is enough. The only thing the issue mentioned is that we are not using a feature and that theannocheck is unhappy. Could you ellaborate what are the consequences of not adding these instructions?

In any case, we need to check if the DWARF needs to be updated to account for the new instructions and if GDB can still unwind across them.

I don't think that justification is enough. The only thing the issue mentioned is that we are not using a feature and that theannocheck is unhappy. Could you ellaborate what are the consequences of not adding these instructions?

Of course. Both ubuntu and Fedora (and by extension RHEL) and I assume other distros, enable by default the-fcf-protection flag for x86_64 and-mbranch-protection=standard for aarch64. When these flags are applied, the compiler places those instructions at the appropriate places when the CPU supports them and a NOP when it doesn't.

Both are implemented to mitigate return-oriented programming (ROP) and Call or Jump Oriented Programming (COP/JOP) attacks.

Minimum supported CPU models are Intel's Tiger lake, AMD's Zen 3, Arm 8.3 (PAC) and Arm 8.5 (BTI).

With pure C code everything is fine by just using the compiler flag(s), however when an assembly file is added to the mix the instructions need to be added manually.

This feature is an all-or-nothing for the hardware functionality to work. All object files are checked for the note and if it's missing on any one, the linker does not place the note to the final executable and the hardening feature is disabled in the hardware. That essentially means that for Python 3.12+ these flags have no effect anymore.

Ok makes sense. Thanks a lot for the extra context.

We need to complement the DWARF information to ensure that the Perf JIT integration still works and we need to ensure gdb can still unwind through the trampolines when this information is active.

This feature is an all-or-nothing for the hardware functionality to work. All object files are checked for the note and if it's missing on any one, the linker does not place the note to the final executable and the hardening feature is disabled in the hardware. That essentially means that for Python 3.12+ these flags have no effect anymore.

Are JIT compilers expected to implement this as well? Or no, since they aren't statically present in the executable?

This feature is an all-or-nothing for the hardware functionality to work. All object files are checked for the note and if it's missing on any one, the linker does not place the note to the final executable and the hardening feature is disabled in the hardware. That essentially means that for Python 3.12+ these flags have no effect anymore.

Are JIT compilers expected to implement this as well? Or no, since they aren't statically present in the executable?

Technically this code is also not present in the executable: we copy it from memory but the region itself where we copy it from is never executed. So if the answer is "no" then we shouldn't need this either

Ok makes sense. Thanks a lot for the extra context.

We need to complement the DWARF information to ensure that the Perf JIT integration still works and we need to ensure gdb can still unwind through the trampolines when this information is active.

Providing also a more practical reproducer.

On Python 3.11 on x86_64:

$ ./configure && make -j10 CFLAGS=-fcf-protection
$ readelf -n python

Properties: x86 feature: IBT, SHSTK

Both of those flags indicate that CET protection is enabled. In Python 3.12+ they are not present.

Of course when building with--enable-shared libpython needs to be inspected instead.

I'll familiarize myself with the debug information and try to figure it out.

This feature is an all-or-nothing for the hardware functionality to work. All object files are checked for the note and if it's missing on any one, the linker does not place the note to the final executable and the hardening feature is disabled in the hardware. That essentially means that for Python 3.12+ these flags have no effect anymore.

Are JIT compilers expected to implement this as well? Or no, since they aren't statically present in the executable?

The answer here would be possibly yes, assembly instructions emitted by JITs should implement that, however I am not familiar myself with JITs in general or the way CPython implements it to provide a definite answer. As JIT is still being developed I'd treat this as an afterthought and revisit it in the future.

Ok makes sense. Thanks a lot for the extra context.

We need to complement the DWARF information to ensure that the Perf JIT integration still works and we need to ensure gdb can still unwind through the trampolines when this information is active.

@pablogsal As the DWARF needs to be updated anyway, it may make sense to add the missing frame pointer to the x86-64 assembler (if Python's policy is to have frame pointers). Or is the goal to elide the calling frame from profiling backtraces? If the latter, there should really be a comment to this effect.

Are JIT compilers expected to implement this as well? Or no, since they aren't statically present in the executable?

@brandtbucher For JIT compilers to keep working unchanged, they need to be marked as incompatible for now. On x86-64, SHSTK is generally automatically compatible with JIT compilation unless stack unwinding code is generated. However, x86-64 IBT requires marker instructions at the target of indirect branches. On AArch64, BTI is similar (marker instructions), but PAC is not process-switched on Linux: it's determined at boot whether it is turned on or not. The presence of the extra instructions merely ensures that the JIT code does not contain any easy-to-use JOP/ROP gadgets.

Whether any of this matters is of course debatable, assuming that Python does not write-protect any of its bytecode in the process image.

Ok makes sense. Thanks a lot for the extra context.

We need to complement the DWARF information to ensure that the Perf JIT integration still works and we need to ensure gdb can still unwind through the trampolines when this information is active.

@pablogsal As the DWARF needs to be updated anyway, it may make sense to add the missing frame pointer to the x86-64 assembler (if Python's policy is to have frame pointers). Or is the goal to elide the calling frame from profiling backtraces? If the latter, there should really be a comment to this effect.

One of the problems is that if you add the frame pointer to the x86-64 assembler gdb cannot unwind through this when Python is compiled without frame pointers. The current version allows unwinding with and without frame pointers.

@brandtbucher For JIT compilers to keep working unchanged, they need to be marked as incompatible for now. On x86-64, SHSTK is generally automatically compatible with JIT compilation unless stack unwinding code is generated. However, x86-64 IBT requires marker instructions at the target of indirect branches. On AArch64, BTI is similar (marker instructions), but PAC is not process-switched on Linux: it's determined at boot whether it is turned on or not. The presence of the extra instructions merely ensures that the JIT code does not contain any easy-to-use JOP/ROP gadgets.

Whether any of this matters is of course debatable, assuming that Python does not write-protect any of its bytecode in the process image.

I'm afraid this is a bit too jargon-heavy for me to follow without doing a bunch of self-guided research on the topic.

What does it mean for the JIT code to be "marked incompatible"? I take it that if the process/kernel has these hardware features enabled, JIT code that doesn't use it will probably crash? What's the typical performance overhead of enabling this, and who exactly is using it (Ubuntu and Fedora were mentioned above... is itall users of these distros who would be unable to use our JIT)?

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

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F128606%2Fmerge

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

Will run the buildbots and I will do a pass and merge

Btw this should be fine to backport to 3.13 and, without the perf_jit_trampoline.c changes, to 3.12.

A single buildbot worker failed:

buildbot/AMD64 Arch Linux TraceRefs PR

But it's an unrelated failure:

_bootstrap_python: Objects/object.c:240: _PyRefchain_Remove: Assertion `value == REFCHAIN_VALUE' failed.make: *** [Makefile:1787: Python/frozen_modules/abc.h] Aborted (core dumped)

There is the same error in the main branch:https://buildbot.python.org/#/builders/484/builds/6705

There is the same error in the main branch:https://buildbot.python.org/#/builders/484/builds/6705

It's a regression introduced by commitfd545d7: issue#127705.

I built Python with this PR:

$ ./configure && make -j10 CFLAGS=-fcf-protection$ readelf -n python|grep PropertiesProperties: x86 feature: IBT, SHSTK

IBT andSHSTK protections are present as expected.

Without this PR (main branch), the protections are missing which is bad for security :-(

$ ./configure && make -j10 CFLAGS=-fcf-protection$ readelf -n python|grep PropertiesProperties: x86 ISA needed: x86-64-baseline

This change fix a security issue, it should be backported up to Python 3.12. Python 3.11 is not affected.

LGTM. I confirm usingreadelf that the change works as expected.

@pablogsal@brandtbucher: I plan to merge this change at the beginning of next week. Tell me if you want to review it and would prefer to have more time to review it.@pablogsal wrote that he planned to merge the change in March.

I am happy yo go ahead. Let's do a last builedbot pass in the perf builedbot just to be sure

!buildbot perf

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

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F128606%2Fmerge

The command will test the builders whose names match following regular expression:perf

The builders matched are:

• AMD64 Arch Linux Perf PR