Skip to content

[DRAFT] gh-128605: Add branch protections for aarch64 in asm_trampoline.S #130864

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[DRAFT] gh-128605: Add branch protections for aarch64 in asm_trampoline.S #130864

wants to merge 1 commit into from

Conversation

stratakis
Copy link
Contributor

@stratakis stratakis commented Mar 5, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Mar 5, 2025
Open
@stratakis
Copy link
Contributor Author

The current equivalent eh_frame for the aarch64 assembly code, generated by C code doesn't seem to properly match, although at the same time it works fine. Here is a comparison between x86_64 and aarch64.

x86_64 eh_frame:

00000000 0000000000000014 00000000 CIE
  Version:               1
  Augmentation:          "zR"
  Code alignment factor: 1
  Data alignment factor: -8
  Return address column: 16
  Augmentation data:     1b
  DW_CFA_def_cfa: r7 (rsp) ofs 8
  DW_CFA_offset: r16 (rip) at cfa-8
  DW_CFA_nop
  DW_CFA_nop

00000018 0000000000000014 0000001c FDE cie=00000000 pc=0000000000000000..000000000000000b
  DW_CFA_advance_loc: 4 to 0000000000000004
  DW_CFA_def_cfa_offset: 16
  DW_CFA_advance_loc: 6 to 000000000000000a
  DW_CFA_def_cfa_offset: 8
  DW_CFA_nop

Equivalent to:

cpython/Python/perf_jit_trampoline.c

Lines 474 to 478 in d0ecbdd

#ifdef __x86_64__
DWRF_U8(DWRF_CFA_advance_loc | 4);
DWRF_U8(DWRF_CFA_def_cfa_offset); DWRF_UV(16);
DWRF_U8(DWRF_CFA_advance_loc | 6);
DWRF_U8(DWRF_CFA_def_cfa_offset); DWRF_UV(8);

Whereas the eh_frame for aarch64:

00000000 0000000000000010 00000000 CIE
  Version:               1
  Augmentation:          "zR"
  Code alignment factor: 4
  Data alignment factor: -8
  Return address column: 30
  Augmentation data:     1b
  DW_CFA_def_cfa: r31 (sp) ofs 0

00000014 0000000000000020 00000018 FDE cie=00000000 pc=0000000000000000..0000000000000014
  DW_CFA_advance_loc: 4 to 0000000000000004
  DW_CFA_def_cfa_offset: 16
  DW_CFA_offset: r29 (x29) at cfa-16
  DW_CFA_offset: r30 (x30) at cfa-8
  DW_CFA_advance_loc: 12 to 0000000000000010
  DW_CFA_restore: r30 (x30)
  DW_CFA_restore: r29 (x29)
  DW_CFA_def_cfa_offset: 0
  DW_CFA_nop
  DW_CFA_nop
  DW_CFA_nop
  DW_CFA_nop
  DW_CFA_nop
  DW_CFA_nop
  DW_CFA_nop

Equivalent to:

cpython/Python/perf_jit_trampoline.c

Lines 480 to 489 in d0ecbdd

#elif defined(__aarch64__) && defined(__AARCH64EL__) && !defined(__ILP32__)
DWRF_U8(DWRF_CFA_advance_loc | 1);
DWRF_U8(DWRF_CFA_def_cfa_offset); DWRF_UV(16);
DWRF_U8(DWRF_CFA_offset | 29); DWRF_UV(2);
DWRF_U8(DWRF_CFA_offset | 30); DWRF_UV(1);
DWRF_U8(DWRF_CFA_advance_loc | 3);
DWRF_U8(DWRF_CFA_offset | -(64 - 29));
DWRF_U8(DWRF_CFA_offset | -(64 - 30));
DWRF_U8(DWRF_CFA_def_cfa_offset);
DWRF_UV(0);

@stratakis stratakis force-pushed the aarch64_branch_protection branch from a6fe267 to fe6fb29 Compare March 12, 2025 14:25
@stratakis
Copy link
Contributor Author

Ah wasn't taking into account the code factor alignment for aarch64. Fixed that.

@stratakis stratakis force-pushed the aarch64_branch_protection branch from fe6fb29 to 8772759 Compare March 17, 2025 13:04
@stratakis
Copy link
Contributor Author

eh_frame for the new assembly which should be implemented in C:

0000000 0000000000000010 0000000 CIE
Version: 1
Augmentation: "zR"
Code alignment factor: 4
Data alignment factor: -8
Return address column: 30
Augmentation data: 1b
DW_CFA_def_cfa: r31 (sp) ofs 0

00000014 0000000000000020 00000018 FDE cie=00000000 pc=0000000000000000..000000000000001c
DW_CFA_advance_loc: 4 to 0000000000000004
DW_CFA_AARCH64_negate_ra_state
DW_CFA_advance_loc: 4 to 0000000000000008
DW_CFA_def_cfa_offset: 16
DW_CFA_offset: r29 (x29) at cfa-16
DW_CFA_offset: r30 (x30) at cfa-8
DW_CFA_advance_loc: 12 to 0000000000000014
DW_CFA_restore: r30 (x30)
DW_CFA_restore: r29 (x29)
DW_CFA_def_cfa_offset: 0
DW_CFA_advance_loc: 4 to 0000000000000018
DW_CFA_AARCH64_negate_ra_state
DW_CFA_nop
DW_CFA_nop
DW_CFA_nop

@stratakis stratakis force-pushed the aarch64_branch_protection branch from 8772759 to 8519ff1 Compare June 3, 2025 02:57
@vstinner vstinner changed the title [DRAFT]gh-128605: Add branch protections for aarch64 in asm_trampoline.S [DRAFT] gh-128605: Add branch protections for aarch64 in asm_trampoline.S Jun 3, 2025
vstinner
vstinner reviewed Jun 3, 2025
View reviewed changes
@vstinner vstinner mentioned this pull request Jun 3, 2025
Open
@vstinner
Copy link
Member

vstinner commented Jun 3, 2025

You might add a NEWS entry using blurb or blurb-it, see: https://devguide.python.org/

@stratakis stratakis force-pushed the aarch64_branch_protection branch from 8519ff1 to 1728096 Compare June 3, 2025 14:55
@stratakis
Copy link
Contributor Author

Noting here that while the protections are enabled with this PR, Perf unwinding which includes the Python functions does not work without Frame Pointers.

For that the hint 25 and hint 29 instructions have to be implemented here: https://github.com/python/cpython/blob/main/Python/perf_jit_trampoline.c#L882C1-L896C1

And they should match this .eh_frame:

https://github.com/python/cpython/commit/0000000003b38411e14b1216e442699bdbf03a15 0000000000000010 https://github.com/python/cpython/commit/0000000003b38411e14b1216e442699bdbf03a15 CIE
Version: 1
Augmentation: "zR"
Code alignment factor: 4
Data alignment factor: -8
Return address column: 30
Augmentation data: 1b
DW_CFA_def_cfa: r31 (sp) ofs 0

00000014 0000000000000020 00000018 FDE cie=00000000 pc=0000000000000000..000000000000001c
DW_CFA_advance_loc: 4 to 0000000000000004
DW_CFA_AARCH64_negate_ra_state
DW_CFA_advance_loc: 4 to 0000000000000008
DW_CFA_def_cfa_offset: 16
DW_CFA_offset: r29 (x29) at cfa-16
DW_CFA_offset: r30 (x30) at cfa-8
DW_CFA_advance_loc: 12 to 0000000000000014
DW_CFA_restore: r30 (x30)
DW_CFA_restore: r29 (x29)
DW_CFA_def_cfa_offset: 0
DW_CFA_advance_loc: 4 to 0000000000000018
DW_CFA_AARCH64_negate_ra_state
DW_CFA_nop
DW_CFA_nop
DW_CFA_nop

Haven't yet figured the correct way to do that though as utilizing the way that the ARM abi describes it by using 0x2D for DW_CFA_AARCH64_negate_ra_state, doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@brandtbucher brandtbucher Awaiting requested review from brandtbucher

@savannahostrowski savannahostrowski Awaiting requested review from savannahostrowski

Assignees
No one assigned
Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stratakis @vstinner