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

Add codegen option for branch protection and pointer authentication on AArch64 #88354

Merged
merged 2 commits into from
Dec 30, 2021

Conversation

Jmc18134
Copy link
Contributor

The branch-protection codegen option enables the use of hint-space pointer
authentication code for AArch64 targets.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 26, 2021
@Absolucy
Copy link
Contributor

Absolucy commented Aug 27, 2021

I'm glad to see proper PAC support implemented, much better than my hacky attempts. Good work!

I believe this can close #73628.

@bors

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Sep 5, 2021

This only implements PAC for return addresses, right? And not for function pointers and data pointers? If it implements the latter two, those will need to be using a different target as the latter two change the abi and thus can't be linked against code that don't use them.

@jacobbramley
Copy link
Contributor

This only implements PAC for return addresses, right? And not for function pointers and data pointers? If it implements the latter two, those will need to be using a different target as the latter two change the abi and thus can't be linked against code that don't use them.

Yes, with these options, PAuth is used only for return addresses. I've found that armclang has the clearest documentation, but the options behave the same in Clang and GCC. In addition, it will generate hint instructions unless Armv8.3 is explicitly enabled, so it will also work for Armv8.0 hardware (but won't check anything).

These options can also generate BTI instructions for forward CFI. Similarly, those are hints before Armv8.5.

The motivation for this change in Rust is to support the same CFI protections as some C code linked into the same binary (e.g. using FFI); we expect compiled Rust code to have a similar number of gadgets as compiled C code, so we need to be able to protect it in the same way.

Adding a new target would be helpful for getting a standard library built with the same options, so that users don't have to rely on the unstable Cargo build-std feature, but that is not a correctness requirement on this patch, and implicitly raises the question of what exactly it should include. I expect that to require some discussion, but it might be desirable to start with something like the (draft) PAuth ABI extension.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 13, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2021
@bors
Copy link
Contributor

bors commented Oct 23, 2021

☔ The latest upstream changes (presumably #90188) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2021
@jyn514
Copy link
Member

jyn514 commented Nov 9, 2021

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned matthewjasper Nov 9, 2021
@nagisa
Copy link
Member

nagisa commented Nov 11, 2021

I'm not sure if I'll be able to get to looking at the PR this weekend, but maybe the next one ^^

// Setting PAC/BTI function attributes is only necessary for LLVM 11 and earlier.
// For LLVM 12 and greater, module-level metadata attributes are set in
// `compiler/rustc_codegen_llvm/src/context.rs`.
if llvm_util::get_version() >= (12, 0, 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The minimum LLVM version is now 12 - this expression is always true.

Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

This seems in a pretty solid shape.

$(COMPILE_OBJ) $(TMPDIR)/test.o test.c -mbranch-protection=bti+pac-ret+leaf
$(AR) rcs $(TMPDIR)/libtest.a $(TMPDIR)/test.o
$(RUSTC) -C branch-protection=bti+pac-ret+leaf test.rs
$(call RUN,test)
Copy link
Member

Choose a reason for hiding this comment

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

style nit: Missing trailing newline.

@@ -907,6 +935,8 @@ options! {

ar: String = (String::new(), parse_string, [UNTRACKED],
"this option is deprecated and does nothing"),
branch_protection: BranchProtection = (BranchProtection::default(), parse_branch_protection, [TRACKED],
Copy link
Member

@nagisa nagisa Nov 19, 2021

Choose a reason for hiding this comment

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

The option should either initially be -Z (see DebuggingOptions) or only unsable when -Zunstable-options is specified.

In particular of concern to me are the fact that it is ignored on targets that do not have a concept of branch protection (yet?) I think the right thing to do would be to raise an error, even if we don't do the same for -Ccontrol-flow-guard, but this could be a future improvement if the flag is made -Z.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean that control-flow-guard should also be under -Z, or that branch-protection should start at -Z before moving to -C at some later date?

On one hand, I'd prefer this not to be nightly-only. However, in order to complete this protection, the currently-unstable build-std must also be used (assuming Cargo), so perhaps putting this under -Z is reasonable for now.

I agree that the option should ideally be an error on non-AArch64 targets (as should control-flow-guard where not supported, really).

Copy link
Member

@nagisa nagisa Nov 27, 2021

Choose a reason for hiding this comment

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

Do you mean that control-flow-guard should also be under -Z, or that branch-protection should start at -Z before moving to -C at some later date?

branch-protection should be a -Z before moving to a -C at a later date. Feature additions like these should go through the same rigor other features do at the very least to give ourselves some time to consider whether the design of this is indeed the right one.

The alternative would be to have this PR to serve as a place for tracking readiness for this feature, but that would mean not landing this PR until it goes through the stabilization process all the way.

crate fn parse_branch_protection(slot: &mut BranchProtection, v: Option<&str>) -> bool {
match v {
Some(s) => {
for opt in s.split('+') {
Copy link
Member

Choose a reason for hiding this comment

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

Conventionally we separate the values in multi-value options by a comma. In some cases, such as for target-features, each value need to be prefixed by a + or - to indicate the feature being enabled or disabled, but the values are still separated by a comma.

Is the branch-protection option significantly different to warrant use of a different separator here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was an attempt to be consistent with C toolchains; both Clang and GCC use the + separator. Currently, these are only used additively, so we could simply replace + with , if consistency with other Rust options is preferred.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we don't particularly care about being consistent with the CLI of the some of the C compilers, and consistency with other flags seems like it'd be nice to have. (If user knows how to combine values for flag A, they'll know how to do same for flag B too)

For example, `-C branch-protection=bti+pac-ret+leaf` is valid, but
`-C branch-protection=bti+leaf+pac-ret` is not.

Repeated values are ignored.
Copy link
Member

@nagisa nagisa Nov 19, 2021

Choose a reason for hiding this comment

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

Would be nice to have a test that ensures nothing goes wrong when multiple options are provided.

- `b-key` - Sign return addresses with key B, instead of the default key A.
- `bti` - Enable branch target identification.

`leaf` and `b-key` are only valid if `pac-ret` was previously specified.
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have a UI test that demonstrates the compiler behaviour in absence of the pac-ret value when the other two are specified.

src/test/assembly/aarch64-pointer-auth.rs Show resolved Hide resolved
Some(pac) => pac.leaf = true,
_ => return false,
},
"b-key" => match slot.pac_ret.as_mut() {
Copy link
Member

Choose a reason for hiding this comment

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

This should ideally work even if b-key comes before pac-ret in the list of options. Same for leaf.

Copy link
Contributor

Choose a reason for hiding this comment

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

Those are modifiers for pac-ret so to me it makes sense to put them after it. This is also what Clang requires (empirically). However, the main reason behind this is more practical: Making Rust flexible (i.e. treating these as an unordered set) is great, but we can't go back without breaking users' build scripts. Conversely, making Rust strict (like C compilers) makes it easier for us to match future changes to the C options, or to extend it in Rust-specific ways (currently unexplored).

We can of course change this to suit Rust's practices if you think that's more appropriate, but I thought I should explain why it was done this way. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with leaving this as an outstanding question for the tracking issue for this feature.

Comment on lines 245 to 265
if let Some(pac_opts) = pac {
llvm::LLVMRustAddModuleFlag(llmod, "sign-return-address\0".as_ptr().cast(), 1);
llvm::LLVMRustAddModuleFlag(
llmod,
"sign-return-address-all\0".as_ptr().cast(),
pac_opts.leaf.into(),
);
llvm::LLVMRustAddModuleFlag(
llmod,
"sign-return-address-with-bkey\0".as_ptr().cast(),
if pac_opts.key == PAuthKey::A { 0 } else { 1 },
);
} else {
llvm::LLVMRustAddModuleFlag(llmod, "sign-return-address\0".as_ptr().cast(), 0);
llvm::LLVMRustAddModuleFlag(llmod, "sign-return-address-all\0".as_ptr().cast(), 0);
llvm::LLVMRustAddModuleFlag(
llmod,
"sign-return-address-with-bkey\0".as_ptr().cast(),
0,
);
}
Copy link
Member

Choose a reason for hiding this comment

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

This can probably be made simpler by doing something along the lines of:

llvm::LLVMRustAddModuleFlag(llmod, "sign-return-address\0".as_ptr().cast(), pac.is_some().into());
let pac_opts = pac.unwrap_or(PacRet { leaf: false, key: PAuthKey::A });
llvm::LLVMRustAddModuleFlag(llmod, "sign-return-address-all\0".as_ptr().cast(), pac_opts.leaf.into());
let is_bkey =  if pac_opts.key == PAuthKey::A { false } else { true };
llvm::LLVMRustAddModuleFlag(llmod, "sign-return-address-with-bkey\0".as_ptr().cast(), is_bkey.into());

@nagisa nagisa added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2021
@jacobbramley
Copy link
Contributor

I'm not sure if @Jmc18134 is available right now but I'll try to answer some of the questions now, and one of us (arm team, @Jmc18134) will pick up the necessary PR updates.

Jmc18134 and others added 2 commits December 1, 2021 12:24
…n AArch64

The branch-protection codegen option enables the use of hint-space pointer
authentication code for AArch64 targets
- Changed the separator from '+' to ','.
- Moved the branch protection options from -C to -Z.
- Additional test for incorrect branch-protection option.
- Remove LLVM < 12 code.
- Style fixes.

Co-authored-by: James McGregor <james.mcgregor2@arm.com>
@sanxiyn sanxiyn added the O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Dec 18, 2021
@nagisa
Copy link
Member

nagisa commented Dec 27, 2021

@bors r+

I imagine there will be some discussion around how the flag options are handled when a stabilization is attempted, but the implementation right now seems good enough to enable use and experimentation with the feature.

@bors
Copy link
Contributor

bors commented Dec 27, 2021

📌 Commit 984ca46 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 27, 2021
@bors
Copy link
Contributor

bors commented Dec 29, 2021

⌛ Testing commit 984ca46 with merge d331cb7...

@bors
Copy link
Contributor

bors commented Dec 30, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing d331cb7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 30, 2021
@bors bors merged commit d331cb7 into rust-lang:master Dec 30, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 30, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d331cb7): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.