Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
8277204: Implement PAC-RET branch protection on Linux/AArch64 #6334
8277204: Implement PAC-RET branch protection on Linux/AArch64 #6334
Changes from 12 commits
9b05854
66b05a6
2c7e273
e0e3f66
29471d3
cfad2fe
25e6249
2c27eb5
a810ea7
6f501e6
dbd6bda
deb17a5
280abc4
995d8aa
38c08ef
63f7515
9c4f349
3cc2c5e
f6f8041
83d2167
1479942
0b47654
b792561
78da1bd
6255d4c
d97883b
614a326
f779513
001a8f1
2062cce
7f80f28
f9882ff
97ae934
c4e0ee3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: this is called "UseROPProtection", the configure option is called "enable-branch-protection", and GCC option is called "-mbranch-protection". This is confusing. I would have thought we would want the same name, and use it for all branch protection. So why is this not "UseBranchProtection"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-mbranch-protection
switches on both PAC-RET and BTI. This PR only covers a use of PAC that looks very ROP-focused to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, because we don't (yet) support BTI. Is there any point having two separate flags for BTI and PAC-RET? If someone wants one, they'll very likely want the other, won't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can support one without the other.
The architecture allows you to have one without the other.
The GCC flag is an enum of "none|standard|pac-ret[+leaf]|bti", with some of them changing depending on which cpu you specify to -mcpu (8.0,8.3,8.5 etc).
Clang has the same flags. Interestingly, on MacOS Clang, -mbranch-protection is available but it'll give incorrect code. Instead you build with -arch arm64e.
If your system had both, the only scenario I could see for only wanting just one would be for test/dev purposes. In a real production scenario you would want everything the system supports or nothing.
An earlier version of my code had a UseBranchProtection="pac|bti|pac+bti|all|none" style option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so we have a precedent.
Yes.
That sounds great.
It seems to me that following the GCC/Clang precedent is the wisest thing we could do. I can see no possible advantage in diverging: it only confuses programmers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That gives us:
A new flag -XX:UseBranchProtection
With the options:
none - no PAC support. (Default)
standard - PAC support if the system supports it and the java binary was compiled with PAC. Otherwise off.
pac-ret - PAC support, regardless if the system supports it or the java binary was compiled with PAC.
A later BTI patch would add:
standard - also adds BTI if the system supports it and the java binary was compiled with BTI.
bti - BTI support, regardless if the system supports it or the java binary was compiled with BTI.
Also, concat the flags with "+". Eg: standard+bti. No need to do this until BTI is added.
For MacOS, you can only use PAC functionality when compiled for arm64e. Therefore arm64e would be supported by compiling the java binary for the arm64e and would always be enabled in that scenario. UseBranchProtection on MacoOS will only support the none option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to the above. The CSR will need an update too.