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.

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

AArch64 BTI support - No BTI after setjmp #48888

Closed
llvmbot opened this issue Mar 11, 2021 · 16 comments
Closed

AArch64 BTI support - No BTI after setjmp #48888

llvmbot opened this issue Mar 11, 2021 · 16 comments
Assignees
@tstellar
Labels
backend:AArch64 bugzilla Issues migrated from bugzilla confirmed Verified by a second party release:backport release:cherry-pick-failed release:reviewed
Milestone
LLVM 14.0.2 Release

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 11, 2021

Bugzilla Link 49544
Version 11.0
OS other
Reporter LLVM Bugzilla Contributor
CC @DougGregor,@zygoloid,@smithp35

Extended Description

When enabling the -mbranch-protection={bti,standard} option in Clang, BTI/PAC instruction are well inserted at each function entry, which works well.
However, the setjmp/longjmp API is not supported while it is in the GNU GCC compiler (see https://gcc.gnu.org/legacy-ml/gcc-patches/2018-11/msg02472.html).
To work properly, a BTI instruction must be added after each call to setjmp function. I have not found any reference to similar in Clang.

While this code is under fixing, is it possible to build a Clang/LLVM module to add instruction right after these setjmp calls ?
@smithp35
Copy link
Collaborator

As I understand it there is a trade off between adding a BTI after a call to setjmp which permits setjmp to return with an indirect jump, but also means there are more gadgets in the program, the address in the jump_buf could be retrieved and branched to.

For example the Bionic implementation of longjump returns with a ret which means a BTI is not needed https://android.googlesource.com/platform/bionic/+/refs/heads/master/libc/arch-arm64/bionic/setjmp.S

Having said that, if the compiler doesn't know what libc is being used it should assume that it might need a BTI and insert one after setjmp. If it does happen to know, such as when Android is used, then it won't need to.

So I agree we should do this. I couldn't give you a commitment of when it will happen though.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
@DavidSpickett
Copy link
Collaborator

I've just started to look at doing this. https://reviews.llvm.org/D112427 did the same thing for Arm 32 bit M profile targets so I will base it on that. Then we can have the clang side drivers set it if we know what that platform's C lib does.

Showing the difference: https://godbolt.org/z/PzMhvo6WM

@llvmbot
Copy link
Collaborator Author

llvmbot commented Jan 10, 2022

@llvm/issue-subscribers-backend-AArch64

@llvmbot llvmbot added the confirmed Verified by a second party label Jan 26, 2022
tom-cosgrove-arm added a commit to tom-cosgrove-arm/openssl that referenced this issue Feb 14, 2022
@tom-cosgrove-arm
aarch64: Fix async_fibre_swapcontext() on clang BTI builds
4d056f2 
Reverting to using swapcontext() when compiling with clang on BTI-enabled
builds fixes the BTI setjmp() failure seen when running asynctest.

The issue with setjmp/longjmp is a known clang bug: see
llvm/llvm-project#48888

Change-Id: I6eeaaa2e15f402789f1b3e742038f84bef846e29
@tom-cosgrove-arm tom-cosgrove-arm mentioned this issue Feb 14, 2022
Closed
openssl-machine pushed a commit to openssl/openssl that referenced this issue Feb 28, 2022
@tom-cosgrove-arm @paulidale
aarch64: Fix async_fibre_swapcontext() on clang BTI builds
f5ec705 
Reverting to using swapcontext() when compiling with clang on BTI-enabled
builds fixes the BTI setjmp() failure seen when running asynctest.

The issue with setjmp/longjmp is a known clang bug: see
llvm/llvm-project#48888

Change-Id: I6eeaaa2e15f402789f1b3e742038f84bef846e29

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from #17698)

(cherry picked from commit d2d2401)
openssl-machine pushed a commit to openssl/openssl that referenced this issue Feb 28, 2022
@tom-cosgrove-arm @paulidale
aarch64: Fix async_fibre_swapcontext() on clang BTI builds
d2d2401 
Reverting to using swapcontext() when compiling with clang on BTI-enabled
builds fixes the BTI setjmp() failure seen when running asynctest.

The issue with setjmp/longjmp is a known clang bug: see
llvm/llvm-project#48888

Change-Id: I6eeaaa2e15f402789f1b3e742038f84bef846e29

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from #17698)
@DavidSpickett DavidSpickett changed the title ARM BTI support - No BTI after setjmp AArch64 BTI support - No BTI after setjmp Mar 14, 2022
@DavidSpickett
Copy link
Collaborator

OP-TEE workaround for future reference: OP-TEE/optee_os@30e743f

I now have a working patch just need to see how to add GlobalIsel support as well.

@DavidSpickett
Copy link
Collaborator

https://reviews.llvm.org/D121707

@EugeneZelenko EugeneZelenko removed the c label Mar 23, 2022
@DavidSpickett
Copy link
Collaborator

This has landed on main, I will request a backport to 14 tomorrow if the buildbots don't bring up anything else.

@DavidSpickett DavidSpickett reopened this Mar 24, 2022
@DavidSpickett DavidSpickett added this to the LLVM 14.0.1 Release milestone Mar 24, 2022
@DavidSpickett
Copy link
Collaborator

/cherry-pick c3b9819

@llvmbot
Copy link
Collaborator Author

llvmbot commented Mar 24, 2022

Failed to cherry-pick: c3b9819

https://github.com/llvm/llvm-project/actions/runs/2033442748

Please manually backport the fix and push it to your github fork. Once this is done, please add a comment like this:

/branch <user>/<repo>/<branch>

@DavidSpickett
Copy link
Collaborator

/branch DavidSpickett/llvm-project/setjmp-bti-llvm-14

@llvmbot llvmbot mentioned this issue Mar 24, 2022
Merged
@llvmbot
Copy link
Collaborator Author

llvmbot commented Mar 24, 2022

/pull-request llvmbot#132

@tstellar
Copy link
Collaborator

tstellar commented Apr 2, 2022

@TNorthover What do you think about backporting this? c3b9819

@TNorthover
Copy link
Contributor

I think it looks OK.

@TNorthover TNorthover assigned tstellar and unassigned TNorthover Apr 7, 2022
@tstellar
Copy link
Collaborator

@DavidSpickett Can you rebase your branch.

@DavidSpickett
Copy link
Collaborator

Done.

@DavidSpickett
Copy link
Collaborator

I see the change on 14.x now so I've removed it from main's release notes 218b5c8.

@DavidSpickett
Copy link
Collaborator

Closing now that this has gone into 14.0.2.

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

@tstellar tstellar

Labels
backend:AArch64 bugzilla Issues migrated from bugzilla confirmed Verified by a second party release:backport release:cherry-pick-failed release:reviewed
Projects
LLVM Release Status
Archived in project
Milestone
LLVM 14.0.2 Release
Development

No branches or pull requests
7 participants
@tstellar @DavidSpickett @TNorthover @EugeneZelenko @kbeyls @smithp35 @llvmbot