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

[AArch64] Stack probing for dynamic allocas in GlobalISel #67123

Conversation

momchil-velikov
Copy link
Collaborator

No description provided.

@github-actions
Copy link

github-actions bot commented Sep 22, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@aemerson
Copy link
Contributor

This looks like too many changes for a single PR.

@momchil-velikov
Copy link
Collaborator Author

This looks like too many changes for a single PR.

Yeah, it's too many, I have separate PRs for:

In the Phabricator days those would be dependent reviews, here they are just three branches on top of each other,
with each commit visible.

Perhaps I should mark the head of each PR in the commit message, so the structure is evident and people can start review from the first one, while still seeing the big picture.

Copy link
Contributor

@oskarwirga oskarwirga left a comment

Choose a reason for hiding this comment

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

I found an interaction between -fstack-clash-protection and -ftrivial-auto-var-init=zero that can be triggered with:

// RUN: %clang_cc1 "-triple" "aarch64-none-linux-android28" "-emit-obj" "-Os" "-fstack-clash-protection" "-mstack-probe-size=65536" "-ftrivial-auto-var-init=zero" "-vectorize-loops" "-x" "c" "%s" "-o" "/dev/null"

// This test case examines a specific interaction between the -fstack-clash-protection
// mitigation and the -ftrivial-auto-var-init=zero option.
// The -fstack-clash-protection option inserts a stack probe loop into the compiled code to protect
// against stack clashes. The -ftrivial-auto-var-init=zero option initializes automatic variables to zero.
// The test case was reduced from an instance where the compiler crashed when these options were used together.
// The crash occurred when condition flags were situated at the insertion point of the stack probe loop.

# 4 "" 3
typedef a;
typedef b;
typedef struct {
  b c
} d;
#define e(f, g, h) h f[g]
i(d *j) {
  a k;
  e(l, j->c == 2, a);
  if (j->c == 2)
    k = l;
  m(k);
}

I have a fix but I do not know if it is the correct way to do it. I will add it inline.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp Outdated Show resolved Hide resolved
@momchil-velikov
Copy link
Collaborator Author

I found an interaction between -fstack-clash-protection and -ftrivial-auto-var-init=zero that can be triggered with:

Thank you, I'll take a look.

@momchil-velikov
Copy link
Collaborator Author

Indeed, that's a bug in 86e003f , specifically in the definition on PROBED_STACKALLOC_DYN pseudo-instruction - it must declare that it implicitly modifies flags.

Thank you again, I will fix that in a follow-up update.

@momchil-velikov momchil-velikov force-pushed the stack-clash-protection-dynalloc-global-isel branch from 9a6abc7 to f6b6124 Compare October 6, 2023 08:14
@momchil-velikov momchil-velikov force-pushed the stack-clash-protection-dynalloc-global-isel branch from f6b6124 to fe5016b Compare October 13, 2023 12:20
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:codegen labels Oct 13, 2023
@momchil-velikov momchil-velikov force-pushed the stack-clash-protection-dynalloc-global-isel branch from fe5016b to e9cf239 Compare October 13, 2023 14:36
@oskarwirga
Copy link
Contributor

I was able to confirm this stack works as expected in my local testing. I haven't uncovered any further interactions with other mitigations. Thank you for working on this!

@momchil-velikov momchil-velikov force-pushed the stack-clash-protection-dynalloc-global-isel branch 4 times, most recently from b678566 to 352cd0f Compare November 17, 2023 12:05
@momchil-velikov momchil-velikov force-pushed the stack-clash-protection-dynalloc-global-isel branch 4 times, most recently from 6a7b24a to dbf035f Compare November 30, 2023 14:00
@momchil-velikov momchil-velikov force-pushed the stack-clash-protection-dynalloc-global-isel branch 2 times, most recently from e052a15 to bb62d6b Compare December 1, 2023 22:49
This adds a stack probing instruction sequence for dynamic stack
allocations, to protect against stack clash attacks. The instruction
sequence used is the same one used for unknown-size allocations in
function prologues.

Change-Id: Iba5f94462d18bcf62d09bb9b4f0d7563f680c19c
Co-authored-by: Oliver Stannard <[email protected]>
@momchil-velikov momchil-velikov force-pushed the stack-clash-protection-dynalloc-global-isel branch from bb62d6b to 18960a9 Compare December 2, 2023 15:35
@momchil-velikov momchil-velikov merged commit c1140d4 into llvm:main Dec 4, 2023
3 checks passed
@momchil-velikov momchil-velikov deleted the stack-clash-protection-dynalloc-global-isel branch January 30, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 clang:codegen clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category llvm:globalisel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants