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

Unconditionally allow shadow call-stack sanitizer for AArch64 #128348

Merged

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Jul 29, 2024

It is possible to do so whenever -Z fixed-x18 is applied.

cc @Darksonn for context

The reasoning is that, as soon as reservation on x18 is forced through the flag fixed-x18, on AArch64 the option to instrument with Shadow Call Stack sanitizer is then applicable regardless of the target configuration.

At the every least, we would like to relax the restriction on specifically aarch64-unknonw-none. For this option, we can include a documentation change saying that users of compiled objects need to ensure that they are linked to runtime with Shadow Call Stack instrumentation support.

Related: #121972

@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2024

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 29, 2024
@dingxiangfei2009 dingxiangfei2009 marked this pull request as ready for review July 29, 2024 19:37
@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

let mut unsupported_sanitizers = sess.opts.unstable_opts.sanitizer - supported_sanitizers;
// Niche: if `fixed-x18`, or effectively switching on `reserved-x18` flag, is enabled
// we should allow Shadow Call Stack sanitizer.
if sess.opts.unstable_opts.fixed_x18 && sess.target.arch == "aarch64" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This only checks fixed-x18 is passed as opts IIUC? What if the target specify reserved-x18 in their features?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can actually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then the sanitizer is listed as supported in the target config, so it will already be in supported_sanitizers.

Copy link
Contributor

Choose a reason for hiding this comment

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

A search showed that compiler/rustc_target/src/spec/targets/aarch64_unknown_linux_ohos.rs has reserve-x18 set, where it doesn't set SanitizerSet::SHADOWCALLSTACK as supported sanitizers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A quick search in ohos revealed no support of SCS. At least not with their musl library.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case we need to ensure that SCS cannot be turned on despite they have enabled reserve-x18

Copy link
Contributor Author

@dingxiangfei2009 dingxiangfei2009 Aug 9, 2024

Choose a reason for hiding this comment

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

One question. How shall we handle with aarch64-unknown-none? The fact that whether the runtime on which the code would be executed on has SCS support is unknown to us.

The single most important use case that this patch enables is to allow code targeting aarch64-unknown-none to be instrumented with SCS sanitizer with -Z fixed-x18.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, some targets are going to support SCS only on a bring-your-own-runtime basis. We can't know up front if that's the case or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of incumbent on the user to supply the correct flags to the compiler, including which runtimes and ABIs are selected. Making the sanitizer config incompatible w/o -Z fixed-x18 (or equivalent) is a requirement due to codegen/ABI. Saying it cannot be used on a particular platform is much harder to do. After all, as long a user could supply their own libc I don't think you could know apriori if SCS would work one way or the other. For platforms, like Android that more or less mandate it (at least in the native/system bits, as I don't recall what requirement are for apps), they can assume you're using a compatible libc(e.g., bionic) and if you're not that's on you .

So, I'd be hesitant to say SCS isn't compatible w/ ohos, though it probably shouldn't be enabled by default for testing, etc.

At some level you have to allow users to opt into things, and if it breaks because they selected an unsupported/incompatible feature on their specific platform configuration, that can't be considered a defect in the Toolchain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I would propose to stick with the wording that user discretion is required.

In fact, checking on LLVM SCS instrumentation, it looks like it demands very little from the runtime. If necessary, we can put a link to the bionic code, so that the user will have an idea what the runtime should subscribe to.

@tmandry
Copy link
Member

tmandry commented Jul 31, 2024

This is at the intersection of two nightly-only features, so I see little risk here.

@dingxiangfei2009 can you update these places in the unstable book as part of this PR to say that this is now supported? It is helpful to have accurate documentation when considering stabilization, and of course for users.

r=me when done (edit: and after addressing @nbdd0121's comments).
@bors delegate+

@bors
Copy link
Contributor

bors commented Jul 31, 2024

✌️ @dingxiangfei2009, you can now approve this pull request!

If @tmandry told you to "r=me" after making some further change, please make that change, then do @bors r=@tmandry

supported_sanitizers: SanitizerSet::KCFI | SanitizerSet::KERNELADDRESS,
supported_sanitizers: SanitizerSet::KCFI
| SanitizerSet::KERNELADDRESS
| SanitizerSet::SHADOWCALLSTACK,
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this make SHADOWCALLSTACK always supported regardless of fixed-x18 flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we can't assume that. I should just revert this hunk.

@ilovepi
Copy link
Contributor

ilovepi commented Jul 31, 2024

On Aarch64, X18 is reserved as the platform register, or otherwise a scratch register. The software SCS implementation for Aarch64 is only compatible when it is reserved, but that isn't enough to know if the platform will support it (e.g. will the SCS be set up correctly on thread creation, etc.). However, it's probably fine to mark it as supported when X18 is reserved, but you may want to make sure the documentation is clear.

Clang has some notions of which sanitizers a triple supports, and those should probably be what you model in rustc. IIRC that's how the new support checking was intended to work, but if there are gaps, I think it makes sense to shore those up.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Aug 1, 2024

In that case I wonder if we do it the other way around, i.e. remove shadow call stack sanitizer from supported sanitizers when fixed-x18 flag (or reserve-x18 feature) is not specified?

@ilovepi
Copy link
Contributor

ilovepi commented Aug 1, 2024

In that case I wonder if we do it the other way around, i.e. remove shadow call stack sanitizer from supported sanitizers when fixed-x18 flag (or reserve-x18 feature) is not specified?

That's probably fine. This is where the Aarch64 rule for SCS is defined in the clang driver https://github.com/llvm/llvm-project/blob/f0944f4be0b3187fa39e9161bc7b344029c200f5/clang/lib/Driver/SanitizerArgs.cpp#L585. The way you've suggested phrasing it is probably closer to that impl, but I don't think there's a meaningful difference in which way you spell it, so long as the right combination is chosen.

@Darksonn
Copy link
Contributor

Darksonn commented Aug 3, 2024

Let's have some tests to verify that:

  • On aarch64-linux-android the sanitizer works both with and without -Zfixed-x18.
  • On aarch64-unknown-none the sanitizer works only with -Zfixed-x18 and not without.

I think if we have those tests, then that should help show that the implementation does the correct thing.

@dingxiangfei2009 dingxiangfei2009 force-pushed the allow-shadow-call-stack-sanitizer branch from 558f649 to 0a9b5cf Compare August 8, 2024 21:15
@rustbot
Copy link
Collaborator

rustbot commented Aug 8, 2024

Some changes occurred in tests/codegen/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

@rustbot rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label Aug 8, 2024
@rust-log-analyzer

This comment has been minimized.

@dingxiangfei2009 dingxiangfei2009 force-pushed the allow-shadow-call-stack-sanitizer branch from 0a9b5cf to 88a2327 Compare August 9, 2024 11:04
@rustbot
Copy link
Collaborator

rustbot commented Aug 9, 2024

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

Some changes occurred in src/doc/unstable-book/src/compiler-flags/sanitizer.md

cc @rust-lang/project-exploit-mitigations, @rcvalle

@rust-log-analyzer

This comment has been minimized.

@@ -787,6 +787,10 @@ A runtime must be provided by the application or operating system.

See the [Clang ShadowCallStack documentation][clang-scs] for more details.

* `aarch64-unknown-none`

In addition to support from a runtime by the application or operating system, `-Zfixed-x18` flag is mandatory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In addition to support from a runtime by the application or operating system, `-Zfixed-x18` flag is mandatory.
In addition to support from a runtime by the application or operating system, the `-Zfixed-x18` flag is mandatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied

Comment on lines 3 to 4
//@[aarch64] compile-flags: --target aarch64-unknown-none -Zfixed-x18 -Zsanitizer=shadow-call-stack
//@[android] compile-flags: --target aarch64-linux-android -Zfixed-x18 -Zsanitizer=shadow-call-stack
Copy link
Contributor

Choose a reason for hiding this comment

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

The android one should also compile fine without -Zfixed-x18.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Flag is dropped.

@dingxiangfei2009 dingxiangfei2009 force-pushed the allow-shadow-call-stack-sanitizer branch from 88a2327 to 1f774d6 Compare August 9, 2024 11:23
@rust-log-analyzer

This comment has been minimized.

@dingxiangfei2009 dingxiangfei2009 force-pushed the allow-shadow-call-stack-sanitizer branch from 1f774d6 to b368dcb Compare August 9, 2024 11:35
@chenyukang
Copy link
Member

r? compiler

@rustbot rustbot assigned BoxyUwU and unassigned chenyukang Aug 12, 2024
@dingxiangfei2009
Copy link
Contributor Author

@bors r=@tmandry

@bors
Copy link
Contributor

bors commented Aug 14, 2024

📌 Commit b368dcb has been approved by tmandry

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 15, 2024
…-stack-sanitizer, r=tmandry

Unconditionally allow shadow call-stack sanitizer for AArch64

It is possible to do so whenever `-Z fixed-x18` is applied.

cc `@Darksonn` for context

The reasoning is that, as soon as reservation on `x18` is forced through the flag `fixed-x18`, on AArch64 the option to instrument with [Shadow Call Stack sanitizer](https://clang.llvm.org/docs/ShadowCallStack.html) is then applicable regardless of the target configuration.

At the every least, we would like to relax the restriction on specifically `aarch64-unknonw-none`. For this option, we can include a documentation change saying that users of compiled objects need to ensure that they are linked to runtime with Shadow Call Stack instrumentation support.

Related: rust-lang#121972
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2024
…llaumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128348 (Unconditionally allow shadow call-stack sanitizer for AArch64)
 - rust-lang#128922 (rust-analyzer: use in-tree `pattern_analysis` crate)
 - rust-lang#128935 (More work on `zstd` compression)
 - rust-lang#129072 (Infer async closure args from `Fn` bound even if there is no corresponding `Future` bound on return)
 - rust-lang#129101 (Fix projections when parent capture is by-ref but child capture is by-value in the `ByMoveBody` pass)
 - rust-lang#129106 (Remove redundant type ops: `Eq`/`Subtype`)
 - rust-lang#129122 (Remove duplicated `Rustdoc::output` method from `run-make-support` lib)
 - rust-lang#129124 (rustdoc-json: Use FxHashMap from rustdoc_json_types)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128348 (Unconditionally allow shadow call-stack sanitizer for AArch64)
 - rust-lang#129065 (Use `impl PartialEq<TokenKind> for Token` more.)
 - rust-lang#129072 (Infer async closure args from `Fn` bound even if there is no corresponding `Future` bound on return)
 - rust-lang#129096 (Print more verbose error for commands that capture output)
 - rust-lang#129101 (Fix projections when parent capture is by-ref but child capture is by-value in the `ByMoveBody` pass)
 - rust-lang#129106 (Remove redundant type ops: `Eq`/`Subtype`)
 - rust-lang#129122 (Remove duplicated `Rustdoc::output` method from `run-make-support` lib)
 - rust-lang#129124 (rustdoc-json: Use FxHashMap from rustdoc_json_types)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3075644 into rust-lang:master Aug 15, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2024
Rollup merge of rust-lang#128348 - dingxiangfei2009:allow-shadow-call-stack-sanitizer, r=tmandry

Unconditionally allow shadow call-stack sanitizer for AArch64

It is possible to do so whenever `-Z fixed-x18` is applied.

cc ``@Darksonn`` for context

The reasoning is that, as soon as reservation on `x18` is forced through the flag `fixed-x18`, on AArch64 the option to instrument with [Shadow Call Stack sanitizer](https://clang.llvm.org/docs/ShadowCallStack.html) is then applicable regardless of the target configuration.

At the every least, we would like to relax the restriction on specifically `aarch64-unknonw-none`. For this option, we can include a documentation change saying that users of compiled objects need to ensure that they are linked to runtime with Shadow Call Stack instrumentation support.

Related: rust-lang#121972
@workingjubilee
Copy link
Member

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 16, 2024
@dingxiangfei2009 dingxiangfei2009 deleted the allow-shadow-call-stack-sanitizer branch August 17, 2024 13:50
@Darksonn
Copy link
Contributor

See #129316 for the riscv follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.