Skip to content

[OpenMP][AArch64] Fix branch protection in microtasks (#102317) #103491

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

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

tuliom
Copy link
Contributor

@tuliom tuliom commented Aug 14, 2024

Start __kmp_invoke_microtask with PACBTI in order to identify the function as a valid branch target. Before returning, SP is authenticated.
Also add the BTI and PAC markers to z_Linux_asm.S.

With this patch, libomp.so can now be generated with DT_AARCH64_BTI_PLT when built with -mbranch-protection=standard.

The implementation is based on the code available in compiler-rt.

(cherry picked from commit 0aa22dc)

@tuliom
Copy link
Contributor Author

tuliom commented Aug 16, 2024

@tru Could you take a look at this backport PR for release/19.x, please?

@tru
Copy link
Collaborator

tru commented Aug 19, 2024

@DanielKristofKiss can you have a look if we want to backport this to 19.x? At this point in the release process we only want to take regressions and serious bugs. It was hard for me to understand if this falls under that.

@DanielKristofKiss DanielKristofKiss self-requested a review August 19, 2024 06:59
Copy link
Member

@DanielKristofKiss DanielKristofKiss left a comment

Choose a reason for hiding this comment

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

Some Distros(e.g. Fedora) are build by default branch-protection=standard.
Without this patch every application and library that links libopenmp.a will not be protected with BTI ( CFI protection for JOP attacks)
Also libopenmp.so won't be protected as not all file is BTI compatible.
So in security view this is an issue.
Not a regression as this was the case in previous releases.
Not a bug, as code will work just won't be CFI protected.
There are systems out there with BTI as of today with such a distro.

@tuliom do you have usecase for this?
@tru Change is simple and straightforward. I support the backport but leave it up to you.

HTH

@tru
Copy link
Collaborator

tru commented Aug 19, 2024

Ok - I am always open to accept things that improve security unless the risk is huge. But it sounds like this will only affect applications that are linking to openmp? Do you see any other risks of accepting this now?

@tuliom
Copy link
Contributor Author

tuliom commented Aug 19, 2024

@tuliom do you have usecase for this?

My usecase is indeed having Fedora and CentOS/RHEL to be fully protected against JOP attacks.
The lack of BTI support causes annocheck to report this issue, e.g. https://issues.redhat.com/browse/RHEL-50807

@tuliom
Copy link
Contributor Author

tuliom commented Aug 19, 2024

But it sounds like this will only affect applications that are linking to openmp?

Correct.

Do you see any other risks of accepting this now?

IMHO, no. But if you prefer to delay this to 19.1.1, that looks good to me.

Start __kmp_invoke_microtask with PACBTI in order to identify the
function as a valid branch target. Before returning, SP is
authenticated.
Also add the BTI and PAC markers to z_Linux_asm.S.

With this patch, libomp.so can now be generated with DT_AARCH64_BTI_PLT
when built with -mbranch-protection=standard.

The implementation is based on the code available in compiler-rt.

(cherry picked from commit 0aa22dc)
@tru tru force-pushed the libomp-bti-19.x branch from 92587cf to 38a591d Compare August 20, 2024 07:14
@tru tru merged commit 38a591d into llvm:release/19.x Aug 20, 2024
6 checks passed
Copy link

@tuliom (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openmp:libomp OpenMP host runtime
Projects
Development

Successfully merging this pull request may close these issues.

4 participants