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.

Sign up for GitHub

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
merged 1 commit into from
Aug 15, 2024
Merged

Unconditionally allow shadow call-stack sanitizer for AArch64 #128348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion compiler/rustc_session/src/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,7 +1187,12 @@ fn validate_commandline_args_with_session_available(sess: &Session) {

// Sanitizers can only be used on platforms that we know have working sanitizer codegen.
let supported_sanitizers = sess.target.options.supported_sanitizers;
let unsupported_sanitizers = sess.opts.unstable_opts.sanitizer - supported_sanitizers;
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 •
edited
Loading

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.

unsupported_sanitizers -= SanitizerSet::SHADOWCALLSTACK;
}
match unsupported_sanitizers.into_iter().count() {
0 => {}
1 => {
Expand Down
5 changes: 5 additions & 0 deletions src/doc/rustc/src/platform-support/android.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,8 @@ Currently the `riscv64-linux-android` target requires the following architecture
* `Zba` (address calculation instructions)
* `Zbb` (base instructions)
* `Zbs` (single-bit instructions)

### aarch64-linux-android on Nightly compilers

As soon as `-Zfixed-x18` compiler flag is supplied, the [`ShadowCallStack` sanitizer](https://releases.llvm.org/7.0.1/tools/clang/docs/ShadowCallStack.html)
instrumentation is also made avaiable by supplying the second compiler flag `-Zsanitizer=shadow-call-stack`.
7 changes: 6 additions & 1 deletion src/doc/unstable-book/src/compiler-flags/fixed-x18.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# `fixed-x18`

This option prevents the compiler from using the x18 register. It is only
supported on aarch64.
supported on `aarch64`.

From the [ABI spec][arm-abi]:

Expand All @@ -23,6 +23,11 @@ Currently, the `-Zsanitizer=shadow-call-stack` flag is only supported on
platforms that always treat x18 as a reserved register, and the `-Zfixed-x18`
flag is not required to use the sanitizer on such platforms. However, the
sanitizer may be supported on targets where this is not the case in the future.
One way to do so now on Nightly compilers is to explicitly supply this `-Zfixed-x18`
flag with `aarch64` targets, so that the sanitizer is available for instrumentation
on targets like `aarch64-unknown-none`, for instance. However, discretion is still
required to make sure that the runtime support is in place for this sanitizer
to be effective.

It is undefined behavior for `-Zsanitizer=shadow-call-stack` code to call into
code where x18 is a temporary register. On the other hand, when you are *not*
Expand Down
4 changes: 4 additions & 0 deletions src/doc/unstable-book/src/compiler-flags/sanitizer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, the `-Zfixed-x18` flag is also mandatory.

# ThreadSanitizer

ThreadSanitizer is a data race detection tool. It is supported on the following
Expand Down
19 changes: 19 additions & 0 deletions tests/codegen/sanitizer/aarch64-shadow-call-stack-with-fixed-x18.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//@ revisions: aarch64 android
//@[aarch64] compile-flags: --target aarch64-unknown-none -Zfixed-x18 -Zsanitizer=shadow-call-stack
//@[aarch64] needs-llvm-components: aarch64
//@[android] compile-flags: --target aarch64-linux-android -Zsanitizer=shadow-call-stack
//@[android] needs-llvm-components: aarch64

#![allow(internal_features)]
#![crate_type = "rlib"]
#![feature(no_core, lang_items)]
#![no_core]

#[lang = "sized"]
trait Sized {}

// CHECK: ; Function Attrs:{{.*}}shadowcallstack
#[no_mangle]
pub fn foo() {}

// CHECK: attributes #0 = {{.*}}shadowcallstack{{.*}}
15 changes: 15 additions & 0 deletions tests/ui/abi/shadow-call-stack-without-fixed-x18.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//@ compile-flags: --target aarch64-unknown-none -Zsanitizer=shadow-call-stack
//@ error-pattern: shadow-call-stack sanitizer is not supported for this target
//@ dont-check-compiler-stderr
//@ needs-llvm-components: aarch64

#![allow(internal_features)]
#![crate_type = "rlib"]
#![feature(no_core, lang_items)]
#![no_core]

#[lang = "sized"]
trait Sized {}

#[no_mangle]
pub fn foo() {}
Loading