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 initial support for DataFlowSanitizer #120761

Merged
merged 1 commit into from
Mar 3, 2024
Merged

Conversation

rcvalle
Copy link
Member

@rcvalle rcvalle commented Feb 8, 2024

Adds initial support for DataFlowSanitizer to the Rust compiler. It currently supports -Zsanitizer-dataflow-abilist. Additional options for it can be passed to LLVM command line argument processor via LLVM arguments using llvm-args codegen option (e.g., -Cllvm-args=-dfsan-combine-pointer-labels-on-load=false).

@rustbot
Copy link
Collaborator

rustbot commented Feb 8, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
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 A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 8, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 8, 2024

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

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

@rcvalle rcvalle added the PG-exploit-mitigations Project group: Exploit mitigations label Feb 8, 2024
@rcvalle
Copy link
Member Author

rcvalle commented Feb 8, 2024

r? @cuviper

@rustbot rustbot assigned cuviper and unassigned oli-obk Feb 8, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 8, 2024

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

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

@rust-log-analyzer

This comment has been minimized.

Copy link

@browneee browneee left a comment

Choose a reason for hiding this comment

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

Hello,
(I am DFSan maintainer in LLVM.)

Thank you for working on DFSan support for Rust!

DFSan support will also need DFSan APIs so the program can make DFSan do something useful. Imo it would be best to do this in this commit, so we can properly test the DFSan pass is working correctly.

compiler/rustc_codegen_llvm/src/back/write.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/write.rs Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

Building with DFSan enabled is a good step, but DFSan needs a bit more than other sanitizers. DFSan doesn't know when it should start taint tracking, and when it should stop taint tracking. There is an API for the program to tell DFSan what it should track and check where it went:
https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/sanitizer/dfsan_interface.h

Should have a basic test like this:
https://github.com/llvm/llvm-project/blob/main/compiler-rt/test/dfsan/basic.c

Other sanitizers have similar interface headers: https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/sanitizer/msan_interface.h
but they are less important (msan will do its thing without that header - you only need the header in rare cases where something unusual is happening and you need to override the behavior). DFSan needs the header to do anything useful.

Looking around rust the closest thing I found for other sanitizers was

msan_weak_symbols.push("__msan_keep_going");
but it is not clear to me if this is a useful example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the Rust compiler currently has any FFI bindings for any of the sanitizers interfaces. Since those are user interfaces, I think the best way to provide them would be to create and provide a sanitizers crate with those bindings. What do you think?

Choose a reason for hiding this comment

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

Ideally these user interfaces would be kept in sync with the compiler.
I don't know how this should best fit together for Rust repos/build/release.

I guess simplest thing (external crate) is probably fine for getting it working.

Would probably be best to ask Rust people how it should fit together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another possibility is to have this crate in the library/ directory along with core, std, and others. (Note that this crate is different than the rustc_sanitizers planned to be in the compiler/ directory because those are user interfaces.) @cuviper @wesleywiser 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 think it will make it a much bigger lift if we have to add to the standard library.

Can you sketch out what kind of calls the user code would need to make?

Copy link
Member Author

Choose a reason for hiding this comment

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

As pointed out to me by @cuviper and @wesleywiser, some of the GitHub workflows don't use the src/llvm-project LLVM, and since this test requires the new dfsan runtime library which is enabled by this PR to be built when the build sanitizers option is enabled for bootstrap, it will fail to link with the dfsan runtime library as it doesn't seem to be present in the LLVM used by the GitHub workflow.

This test passes locally using the src/llvm-project LLVM and is also part of the sanitizers crate so I'll remove it from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I fully understand the problem, but maybe you just need to update src/bootstrap/download-ci-llvm-stamp to force a rebuild?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh, I didn't know about that. Let me give it a try.

Copy link
Member Author

Choose a reason for hiding this comment

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

It worked! (It seems it also needed needs-sanitizer-support in the test file.) Thank you! I'll also adjust the path and file names to conform to #121811 which is in the queue to be merged. Then it should be good to go. I'll let you know! Thanks again!

Copy link
Member Author

Choose a reason for hiding this comment

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

#121811 was merged and I adjusted the path and file names. It should be good to go. Thanks again!

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rcvalle rcvalle force-pushed the rust-dfsan branch 2 times, most recently from c243d42 to cec8233 Compare February 14, 2024 18:18
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@nikic
Copy link
Contributor

nikic commented Feb 19, 2024

Do you have some specific use case in mind for this, or is this more for the sake of completeness? I'm always a bit fuzzy on what people actually use dfsan for.

@browneee
Copy link

Do you have some specific use case in mind for this, or is this more for the sake of completeness? I'm always a bit fuzzy on what people actually use dfsan for.

It helps us to do C++ and Rust interop. If a C++ codebase (that already uses DFSan) starts interoperating with Rust, the DFSan build will break - unless the Rust part can also support DFSan.

@bors
Copy link
Contributor

bors commented Feb 20, 2024

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

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

tests/ui/sanitize/sanitizer-dataflow-basic.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Adds initial support for DataFlowSanitizer to the Rust compiler. It
currently supports `-Zsanitizer-dataflow-abilist`. Additional options
for it can be passed to LLVM command line argument processor via LLVM
arguments using `llvm-args` codegen option (e.g.,
`-Cllvm-args=-dfsan-combine-pointer-labels-on-load=false`).
@nikic
Copy link
Contributor

nikic commented Mar 2, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 2, 2024

📌 Commit dee4e02 has been approved by nikic

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 Mar 2, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 2, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#120761 (Add initial support for DataFlowSanitizer)
 - rust-lang#121622 (Preserve same vtable pointer when cloning raw waker, to fix Waker::will_wake)
 - rust-lang#121716 (match lowering: Lower bindings in a predictable order)
 - rust-lang#121731 (Now that inlining, mir validation and const eval all use reveal-all, we won't be constraining hidden types here anymore)
 - rust-lang#121841 (`f16` and `f128` step 2: intrinsics)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8653c09 into rust-lang:master Mar 3, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2024
Rollup merge of rust-lang#120761 - rcvalle:rust-dfsan, r=nikic

Add initial support for DataFlowSanitizer

Adds initial support for DataFlowSanitizer to the Rust compiler. It currently supports `-Zsanitizer-dataflow-abilist`. Additional options for it can be passed to LLVM command line argument processor via LLVM arguments using `llvm-args` codegen option (e.g., `-Cllvm-args=-dfsan-combine-pointer-labels-on-load=false`).
@rcvalle rcvalle deleted the rust-dfsan branch April 22, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

8 participants