riscv64imac: allow shadow call stack sanitizer #129316

dingxiangfei2009 · 2024-08-20T15:37:49Z

cc @Darksonn for shadow call stack sanitizer support on RV64IMAC and RV64GC

rustbot · 2024-08-20T15:37:57Z

rustbot has assigned @nnethercote.
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

Darksonn · 2024-08-20T15:43:23Z

Some context: on aarch64 there is a requirement to pass the -Zfixed-x18, but there is no such flag on riscv. You just need to make sure the build is linked to a runtime with Shadow Call Stack instrumentation support.

See #128348 for the equivalent aarch64 change.

ilovepi · 2024-08-20T16:08:24Z

For SCS RISC-V needs gp/X3 to not be used for gp-relaxation. Not sure if rustc has a nob for that, but it may make sense to issue a warning if SCS and gp-relaxation are used together. I don't recall if we do that in clang, but you're going to have a bad time if you mix them. We're supposed to use the ELF attributes to make sure you don't mix them improperly and fail the link, but that work has been blocked on linker support across toolchains. Butgp/X3 is always reserved, so you're right that the equivalent check isn't needed.

Darksonn · 2024-08-20T16:15:06Z

I don't believe rustc has a nob for that. I've only ever seen it on linkers. It doesn't sound like there's anything we can do about it right now?

ilovepi · 2024-08-20T16:31:30Z

I don't believe rustc has a nob for that. I've only ever seen it on linkers. It doesn't sound like there's anything we can do about it right now?

Yeah, I'm not all that surprised. But it does probably make sense to warn i they're used together to compile the same crate/module/TU. Not a problem this patch needs to solve today (I don't even think we do that in clang), but its probably nicer to do if we know its incompatible with another option.

nnethercote · 2024-08-21T04:21:34Z

I don't know anything about this stuff. I do see that #128348 added two tests and added text to several .md files -- should this PR do the same?

@tmandry reviewed the other one, so I will forward the review to him:
r? tmandry

ilovepi · 2024-08-21T15:53:25Z

Actualy, I'd suggest rewriting the bulk of the shadow stack documentation to more closely align w/ what's in LLVM: https://clang.llvm.org/docs/ShadowCallStack.html#id6. Before we added RISC-V support, our docs were also very Aarch64 centric, and it seems like the Rust versions were closely following the old version. I'd also suggest that at least some of the descriptions in the fixed-x18.md be moved to a shadwow-call-stack.md file, and discuss the overall picture of SCS, rather than the vagaries of the Aarch64 ABI. Those details have a place, but have equivalents in other architectures, like RISC-V, so I think it would be best to organize them accordingly. I'm happy to help w/ that, but I'm not sure I'll have time for a PR before next week.

dingxiangfei2009 · 2024-08-22T07:01:00Z

No worries @ilovepi I can help with moving the documentation. I will put it up later and see if we have time next week to walk it through. 👍

There is a sanitizer.md under unstable-book, where other sanitizers are documented. I feel that the discussion on the overall picture of SCS is better suited over there and the section needs some expansion and especially editing. Would that sounds reasonable?

Darksonn · 2024-08-22T13:14:07Z

Let's mention the target in src/doc/unstable-book/src/compiler-flags/sanitizer.md so we don't make #121966 worse. As for otherwise improving the documentation, I think it is a good idea, but I don't think it needs to block this PR.

I would be happy to commit to making sure that updating the documentation happens, but I won't have time this week or next week.

Would that work for you?

ilovepi · 2024-08-22T15:41:12Z

Agreed that the extra documentation shouldn’t be a blocker on this.

@dingxiangfei2009 @Darksonn Thanks for taking the lead on those updates 😄. Let me know if you’d like feedback, as I’m happy to help.

rustbot · 2024-08-23T18:32:16Z

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

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

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

dingxiangfei2009 · 2024-08-23T18:32:52Z

I need a bit more time to write a test. Let us move ahead and tweak the wording first.

Darksonn · 2024-08-26T14:25:50Z

For the test it looks like you could just copy tests/codegen/sanitizer/aarch64-shadow-call-stack-with-fixed-x18.rs and adjust the target name. Or even rename the test to not be aarch64 specific and add it inside the file.

rustbot · 2024-08-26T21:43:31Z

Some changes occurred in tests/codegen/sanitizer

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

dingxiangfei2009 · 2024-08-26T21:46:26Z

@rustbot ready

A FileCheck test is added. I have also independently verified the generated library artifacts locally and and it looks properly instrumented. If a further testing on the efficacy of the instrumentation, I have something offline but I need a bit time to figure out how to make it play with compiletest.
Documentation is live but I would need to tweak the wording. Suggestions are very much welcome!

ilovepi · 2024-08-26T22:55:08Z

src/doc/unstable-book/src/compiler-flags/sanitizer.md

+To that end, implementation of this sanitizer requires reservation of one of the registers on the target platform.
+Software support from the operating system and runtime may be required depending on the target platform which is detailed in the remaining section.
+See the [Clang ShadowCallStack documentation][clang-scs] for more details.


The current phrasing makes it sound like you could reserve any register and tell the compiler. Maybe something that's a mix of the old and new version would make that less ambiguous?

Suggested change

To that end, implementation of this sanitizer requires reservation of one of the registers on the target platform.

Software support from the operating system and runtime may be required depending on the target platform which is detailed in the remaining section.

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

The ShadowCallStack (SCS) requires that the platform reserves a register that could be used for the SCS. Aarch64 and RISC-V both have a platform register defined in their ABIs, which can optionally be reserved for this purpose (X18 and X3/GP respectively). Software support from the operating system and runtime may be required depending on the target platform which is detailed in the remaining section.

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

Applied. The wording about the register reservation was indeed a bit ambiguous.

ilovepi · 2024-08-26T23:02:41Z

src/doc/unstable-book/src/compiler-flags/sanitizer.md

+The following targets support ShadowCallStack.
+
+* `riscv64imac-unknown-none-elf`
+* `riscv64gc-unknown-none-elf`


you could include Fuchsia here as well.

ilovepi · 2024-08-26T23:04:25Z

src/doc/unstable-book/src/compiler-flags/sanitizer.md

+ShadowCallStack requires on this platform ABI reservation of the register `x18` as the instrumentation makes use of this register.
+When `x18` is not reserved on the target AArch64 platform and is availabe as a scratch register, enabling ShadowCallStack leds to undefined behaviour.
+In other words, code that is calling into or called by functions instrumented with ShadowCallStack must reserve the `x18` register or preserve its value.


Suggested change

ShadowCallStack requires on this platform ABI reservation of the register `x18` as the instrumentation makes use of this register.

When `x18` is not reserved on the target AArch64 platform and is availabe as a scratch register, enabling ShadowCallStack leds to undefined behaviour.

In other words, code that is calling into or called by functions instrumented with ShadowCallStack must reserve the `x18` register or preserve its value.

ShadowCallStack requires the use of the ABI defined platform register, `x18`, which is required for code generation purposes.

When `x18` is not reserved, and is instead used as a scratch register, enabling ShadowCallStack would lead to undefined behavior, due to corruption of the return addresses on the SCS or through clobbering the SCS register.

In other words, code that is calling into or called by functions instrumented with ShadowCallStack must reserve the `x18` register or preserve its value.

WDYT of this phrasing?

Applied with additional explanation on the source of UB.

ilovepi · 2024-08-26T23:08:44Z

tests/codegen/sanitizer/riscv64-shadow-call-stack.rs

+#[lang = "sized"]
+trait Sized {}
+
+// CHECK: ; Function Attrs:{{.*}}shadowcallstack


You probably want a check line for the definition of foo and the attribute #0 to match up w/ the check below.

Added. Now the test is matching foo with #0 attribute.

dingxiangfei2009 · 2024-08-28T12:20:37Z

@rustbot ready

The documentation has received improvement on wording.
FileCheck is updated to match the instrumented function with its attribute node.

nnethercote · 2024-08-28T22:44:16Z

@ilovepi: if you are satisfied with the latest version, then I am happy to r+ it.

ilovepi · 2024-08-29T00:57:08Z

LGTM. Depending on how complete you want the target info, Fuchsia also supports scs on Aarch64. Not a blocker in my opinion so feel free to land as is.

nnethercote · 2024-08-29T04:56:40Z

LGTM. Depending on how complete you want the target info, Fuchsia also supports scs on Aarch64. Not a blocker in my opinion so feel free to land as is.

@dingxiangfei2009: I will delegate-approve this so you can make that change if you like, and then approve this yourself.

@bors delegate=dingxiangfei2009

bors · 2024-08-29T04:56:43Z

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

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

dingxiangfei2009 · 2024-08-29T13:50:05Z

@ilovepi I added the AArch64 Fuchsia and went through the docs again. 🚀

@bors r=@nnethercote

bors · 2024-08-29T13:50:07Z

📌 Commit 9c29b33 has been approved by nnethercote

It is now in the queue for this repository.

…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#123940 (debug-fmt-detail option) - rust-lang#128166 (Improved `checked_isqrt` and `isqrt` methods) - rust-lang#128970 (Add `-Zlint-llvm-ir`) - rust-lang#129316 (riscv64imac: allow shadow call stack sanitizer) - rust-lang#129690 (Add `needs-unwind` compiletest directive to `libtest-thread-limit` and replace some `Path` with `path` in `run-make`) - rust-lang#129732 (Add `unreachable_pub`, round 3) - rust-lang#129743 (Fix rustdoc clippy lints) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129316 - dingxiangfei2009:riscv64-imac-scs, r=nnethercote riscv64imac: allow shadow call stack sanitizer cc `@Darksonn` for shadow call stack sanitizer support on RV64IMAC and RV64GC

rustbot assigned nnethercote Aug 20, 2024

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 Aug 20, 2024

Darksonn mentioned this pull request Aug 20, 2024

Unconditionally allow shadow call-stack sanitizer for AArch64 #128348

Merged

ojeda mentioned this pull request Aug 20, 2024

Rust unstable features needed for the kernel Rust-for-Linux/linux#2

Open

89 tasks

rustbot assigned tmandry and unassigned nnethercote Aug 21, 2024

dingxiangfei2009 force-pushed the riscv64-imac-scs branch from b4deea8 to 31c70df Compare August 23, 2024 18:31

rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label Aug 23, 2024

dingxiangfei2009 marked this pull request as ready for review August 23, 2024 18:32

dingxiangfei2009 force-pushed the riscv64-imac-scs branch from 31c70df to 133044e Compare August 26, 2024 21:43

ilovepi reviewed Aug 26, 2024

View reviewed changes

dingxiangfei2009 force-pushed the riscv64-imac-scs branch from 133044e to 930c043 Compare August 28, 2024 12:17

riscv64imac: allow shadow call stack sanitizer

9c29b33

dingxiangfei2009 force-pushed the riscv64-imac-scs branch from 930c043 to 9c29b33 Compare August 29, 2024 13:49

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 29, 2024

GuillaumeGomez mentioned this pull request Aug 29, 2024

Rollup of 7 pull requests #129750

Merged

bors merged commit a65404a into rust-lang:master Aug 29, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 29, 2024

dingxiangfei2009 deleted the riscv64-imac-scs branch August 31, 2024 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

riscv64imac: allow shadow call stack sanitizer #129316

riscv64imac: allow shadow call stack sanitizer #129316

dingxiangfei2009 commented Aug 20, 2024

rustbot commented Aug 20, 2024

Darksonn commented Aug 20, 2024

ilovepi commented Aug 20, 2024

Darksonn commented Aug 20, 2024

ilovepi commented Aug 20, 2024

nnethercote commented Aug 21, 2024

ilovepi commented Aug 21, 2024

dingxiangfei2009 commented Aug 22, 2024 •

edited

Loading

Darksonn commented Aug 22, 2024

ilovepi commented Aug 22, 2024

rustbot commented Aug 23, 2024

dingxiangfei2009 commented Aug 23, 2024 •

edited

Loading

Darksonn commented Aug 26, 2024

rustbot commented Aug 26, 2024

dingxiangfei2009 commented Aug 26, 2024

ilovepi Aug 26, 2024

dingxiangfei2009 Aug 28, 2024

ilovepi Aug 26, 2024

dingxiangfei2009 Aug 28, 2024

ilovepi Aug 26, 2024

dingxiangfei2009 Aug 28, 2024

ilovepi Aug 26, 2024

dingxiangfei2009 Aug 28, 2024

dingxiangfei2009 commented Aug 28, 2024

nnethercote commented Aug 28, 2024

ilovepi commented Aug 29, 2024 •

edited

Loading

nnethercote commented Aug 29, 2024

bors commented Aug 29, 2024

dingxiangfei2009 commented Aug 29, 2024

bors commented Aug 29, 2024

riscv64imac: allow shadow call stack sanitizer #129316

riscv64imac: allow shadow call stack sanitizer #129316

Conversation

dingxiangfei2009 commented Aug 20, 2024

rustbot commented Aug 20, 2024

Darksonn commented Aug 20, 2024

ilovepi commented Aug 20, 2024

Darksonn commented Aug 20, 2024

ilovepi commented Aug 20, 2024

nnethercote commented Aug 21, 2024

ilovepi commented Aug 21, 2024

dingxiangfei2009 commented Aug 22, 2024 • edited Loading

Darksonn commented Aug 22, 2024

ilovepi commented Aug 22, 2024

rustbot commented Aug 23, 2024

dingxiangfei2009 commented Aug 23, 2024 • edited Loading

Darksonn commented Aug 26, 2024

rustbot commented Aug 26, 2024

dingxiangfei2009 commented Aug 26, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingxiangfei2009 commented Aug 28, 2024

nnethercote commented Aug 28, 2024

ilovepi commented Aug 29, 2024 • edited Loading

nnethercote commented Aug 29, 2024

bors commented Aug 29, 2024

dingxiangfei2009 commented Aug 29, 2024

bors commented Aug 29, 2024

dingxiangfei2009 commented Aug 22, 2024 •

edited

Loading

dingxiangfei2009 commented Aug 23, 2024 •

edited

Loading

ilovepi commented Aug 29, 2024 •

edited

Loading