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

Jmc18134 · 2021-08-26T09:48:24Z

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

rust-highfive · 2021-08-26T09:48:26Z

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.

Absolucy · 2021-08-27T13:10:41Z

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

I believe this can close #73628.

bjorn3 · 2021-09-05T19:22:02Z

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 · 2021-09-06T09:45:47Z

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.

bors · 2021-10-23T09:34:29Z

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

jyn514 · 2021-11-09T11:02:42Z

r? @nagisa

nagisa · 2021-11-11T16:19:57Z

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

adamgemmell · 2021-11-16T14:42:25Z

compiler/rustc_codegen_llvm/src/attributes.rs

+    // 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) {


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

nagisa

This seems in a pretty solid shape.

nagisa · 2021-11-19T22:20:55Z

src/test/run-make-fulldeps/pointer-auth-link-with-c/Makefile

+	$(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)


style nit: Missing trailing newline.

nagisa · 2021-11-19T22:24:01Z

compiler/rustc_session/src/options.rs

@@ -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],


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.

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).

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.

nagisa · 2021-11-19T22:25:38Z

compiler/rustc_session/src/options.rs

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


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?

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.

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)

nagisa · 2021-11-19T22:26:58Z

src/doc/rustc/src/codegen-options/index.md

+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.


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

nagisa · 2021-11-19T22:27:38Z

src/doc/rustc/src/codegen-options/index.md

+- `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.


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

nagisa · 2021-11-19T22:36:36Z

compiler/rustc_session/src/options.rs

+                            Some(pac) => pac.leaf = true,
+                            _ => return false,
+                        },
+                        "b-key" => match slot.pac_ret.as_mut() {


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

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?

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

nagisa · 2021-11-19T23:07:51Z

compiler/rustc_codegen_llvm/src/context.rs

+        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,
+            );
+        }


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());

jacobbramley · 2021-11-23T19:53:16Z

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.

…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>

nagisa · 2021-12-27T17:39:40Z

@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 · 2021-12-27T17:39:42Z

📌 Commit 984ca46 has been approved by nagisa

bors · 2021-12-29T22:35:15Z

⌛ Testing commit 984ca46 with merge d331cb7...

bors · 2021-12-30T01:47:27Z

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

rust-timer · 2021-12-30T04:20:20Z

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

rust-highfive assigned matthewjasper Aug 26, 2021

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

Absolucy mentioned this pull request Aug 27, 2021

Support for ARMv8.3+ targets #73628

Open

This comment has been minimized.

Sign in to view

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 13, 2021

jacobbramley mentioned this pull request Sep 16, 2021

Use the 64b inner:monotonize() implementation not the 128b one for aarch64 #88651

Merged

Jmc18134 force-pushed the hint-space-pauth-opt branch from 68e1970 to 6153e3b Compare September 28, 2021 19:19

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

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

rust-highfive assigned nagisa and unassigned matthewjasper Nov 9, 2021

adamgemmell reviewed Nov 16, 2021

View reviewed changes

nagisa reviewed Nov 19, 2021

View reviewed changes

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

akirilov-arm mentioned this pull request Dec 1, 2021

RFC: CFI Improvements with PAuth and BTI bytecodealliance/rfcs#17

Merged

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

Add codegen option for branch protection and pointer authentication o…

837cc16

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

Review comments

984ca46

- 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>

jacobbramley force-pushed the hint-space-pauth-opt branch from 6153e3b to 984ca46 Compare December 3, 2021 14:03

sanxiyn added the O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Dec 18, 2021

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 added the merged-by-bors This PR was explicitly merged by bors. label Dec 30, 2021

bors merged commit d331cb7 into rust-lang:master Dec 30, 2021

rustbot added this to the 1.59.0 milestone Dec 30, 2021

bors mentioned this pull request Dec 30, 2021

Extend -Cdebuginfo with new options and named aliases #83947

Closed

glandium mentioned this pull request Jan 14, 2022

Some mixes of Rust with C/C++ are broken for arm64 mac and windows #92885

Closed

apiraino mentioned this pull request Feb 1, 2022

Use error-on-mismatch policy for PAuth module flags. #93269

Merged

jacobbramley mentioned this pull request Jul 5, 2023

Tracking Issue for stabilisation of -Z branch-protection #113369

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Jmc18134 commented Aug 26, 2021

rust-highfive commented Aug 26, 2021

Absolucy commented Aug 27, 2021 •

edited

Loading

This comment has been minimized.

bjorn3 commented Sep 5, 2021

jacobbramley commented Sep 6, 2021

bors commented Oct 23, 2021

jyn514 commented Nov 9, 2021

nagisa commented Nov 11, 2021

adamgemmell Nov 16, 2021

nagisa left a comment

nagisa Nov 19, 2021

nagisa Nov 19, 2021 •

edited

Loading

jacobbramley Nov 23, 2021

nagisa Nov 27, 2021 •

edited

Loading

nagisa Nov 19, 2021

jacobbramley Nov 23, 2021

nagisa Nov 27, 2021

nagisa Nov 19, 2021 •

edited

Loading

nagisa Nov 19, 2021

nagisa Nov 19, 2021

jacobbramley Nov 23, 2021

nagisa Nov 27, 2021

nagisa Nov 19, 2021

jacobbramley commented Nov 23, 2021

nagisa commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 29, 2021

bors commented Dec 30, 2021

rust-timer commented Dec 30, 2021

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

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

Conversation

Jmc18134 commented Aug 26, 2021

rust-highfive commented Aug 26, 2021

Absolucy commented Aug 27, 2021 • edited Loading

This comment has been minimized.

bjorn3 commented Sep 5, 2021

jacobbramley commented Sep 6, 2021

bors commented Oct 23, 2021

jyn514 commented Nov 9, 2021

nagisa commented Nov 11, 2021

Choose a reason for hiding this comment

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa Nov 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobbramley commented Nov 23, 2021

nagisa commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 29, 2021

bors commented Dec 30, 2021

rust-timer commented Dec 30, 2021

Absolucy commented Aug 27, 2021 •

edited

Loading

nagisa Nov 19, 2021 •

edited

Loading

nagisa Nov 27, 2021 •

edited

Loading

nagisa Nov 19, 2021 •

edited

Loading