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

Jump to bottom

Add kernel-address sanitizer support for freestanding targets #99679

Merged
merged 1 commit into from
Feb 18, 2023
Merged

Add kernel-address sanitizer support for freestanding targets #99679

merged 1 commit into from
Feb 18, 2023

Conversation

repnop
Copy link
Contributor

@repnop repnop commented Jul 24, 2022

This PR adds support for KASan (kernel address sanitizer) instrumentation in freestanding targets. I included the minimal set of x86_64-unknown-none, riscv64{imac, gc}-unknown-none-elf, and aarch64-unknown-none but there's likely other targets it can be added to. (linux_kernel_base.rs?) KASan uses the address sanitizer attributes but has the CompileKernel parameter set to true in the pass creation.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 24, 2022
@rust-highfive
Copy link
Collaborator

r? @cuviper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Andy-Python-Programmer
Copy link
Contributor

Amazing! 🎉

cuviper
cuviper requested changes Jul 28, 2022
View reviewed changes
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Are there any tests we can add for this? Perhaps add to the sanitizer tests in src/test/codegen/?

How about some documentation in src/doc/unstable-book/src/compiler-flags/sanitizer.md?

Should this be added to Session::emit_lifetime_markers?

compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp Outdated Show resolved Hide resolved
compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp Outdated Show resolved Hide resolved
compiler/rustc_session/src/options.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented Jul 28, 2022

but there's likely other targets it can be added to. (linux_kernel_base.rs?)

That sounds reasonable -- is there any reason not to add it there?

@repnop
Copy link
Contributor Author

repnop commented Aug 2, 2022

Are there any tests we can add for this? Perhaps add to the sanitizer tests in src/test/codegen/?

it uses the same instrumentation as Address Sanitizer so I think it should be pretty simple to integrate with those, I'll take a look!

How about some documentation in src/doc/unstable-book/src/compiler-flags/sanitizer.md?

didn't even think of that! 😅 good catch

Should this be added to Session::emit_lifetime_markers?

hmm, I'm not familiar with that area, but I'll see what I can find out!

but there's likely other targets it can be added to. (linux_kernel_base.rs?)

That sounds reasonable -- is there any reason not to add it there?

I thought so too, but I can't say I'm 100% confident if it should be. @alex you added the original x86_64-linux-kernel target, do you know if there'd be any issues with enabling KASan for it? my thought would be that since it needs explicit kernel support, it would probably only make sense if the kernel its running on has KASan enabled?

as for other targets, I was hoping to get some feedback from someone more familiar with the set of targets that would actually support this -- I'm mainly unsure of the various bare-metal ARM targets, but I'll also see if I can dig up what clang supports the flag on and use that as a reference 👍

@bors
Copy link
Contributor

bors commented Sep 1, 2022

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

@pnkfelix
Copy link
Member

pnkfelix commented Sep 8, 2022

based on the previous comment from @repnop, I assume this is waiting on @repnop to integrate the review feedback provided by @cuviper

@rustbot label: -S-waiting-on-review S-waiting-on-author

@rustbot rustbot 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 Sep 8, 2022
@repnop repnop force-pushed the kernel-address-sanitizer branch 2 times, most recently from c91f6c2 to b705dfb Compare September 12, 2022 01:00
@repnop
Copy link
Contributor Author

repnop commented Sep 12, 2022

@cuviper I believe all of the changes you requested are now done. I split out the tests where appropriate since the targets with the other sanitizers won't support this. I ran both of them locally with ./x.py test --force-rerun src/test/ui/sanitize/cfg-kasan.rs src/test/codegen/sanitizer-kasan-emits-instrumentation.rs --target=riscv64imac-unknown-none-elf --host=x86_64-unknown-linux-gnu and got a pass for each, so I think things are looking good there. I did find out that I had to modify the cfg setting code because KASAN is really just a different ASAN compilation method, so I figure the attributes should be the same for both.

As for the x86_64-unknown-none-linuxkernel target that I had mentioned previously, I dug around and I cannot find a single project in the wild except for one in a google search for that target that hasn't been updated in 2 years, the Rust for Linux uses a custom target.json for their builds with a different LLVM target, and as far as I can tell, its nigh identical to the x86_64-unknown-none target, so I'm going to ignore it now as I don't think its likely to exist for long unless there's a very good reason for it to.

@repnop repnop requested a review from cuviper September 12, 2022 01:06
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@repnop
Copy link
Contributor Author

repnop commented Sep 13, 2022

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 13, 2022
@bors
Copy link
Contributor

bors commented Sep 25, 2022

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

@wesleywiser
Copy link
Member

Hey @cuviper, it looks like this PR is ready for another round of review. Thanks! 🙂

@wesleywiser
Copy link
Member

@repnop would you mind rebasing and resolving the merge conflicts? That will help shorten the review cycle.

@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jan 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 29, 2023

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

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jan 29, 2023

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

@repnop
Copy link
Contributor Author

repnop commented Jan 31, 2023

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2023
cuviper
cuviper requested changes Feb 8, 2023
View reviewed changes
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Just a couple typos for riscv min-llvm-version.

tests/codegen/sanitizer-kasan-emits-instrumentation.rs Outdated Show resolved Hide resolved
tests/codegen/sanitizer-kasan-emits-instrumentation.rs Outdated Show resolved Hide resolved
@repnop repnop requested a review from cuviper February 12, 2023 22:52
@bors
Copy link
Contributor

bors commented Feb 15, 2023

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

cuviper
cuviper approved these changes Feb 15, 2023
View reviewed changes
Copy link
Member

@cuviper cuviper left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@bors r+

(bah, bors doesn't see reviews...)

@cuviper
Copy link
Member

cuviper commented Feb 15, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Feb 15, 2023

📌 Commit 1971438 has been approved by cuviper

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 Feb 15, 2023
ckxkexing added a commit to ckxkexing/rust that referenced this pull request Feb 17, 2023
@ckxkexing
Rollup merge of rust-lang#99679 - repnop:kernel-address-sanitizer, r=…
808e21e 
…cuviper

Add `kernel-address` sanitizer support for freestanding targets

This PR adds support for KASan (kernel address sanitizer) instrumentation in freestanding targets. I included the minimal set of `x86_64-unknown-none`, `riscv64{imac, gc}-unknown-none-elf`, and `aarch64-unknown-none` but there's likely other targets it can be added to. (`linux_kernel_base.rs`?) KASan uses the address sanitizer attributes but has the `CompileKernel` parameter set to `true` in the pass creation.
@ckxkexing ckxkexing mentioned this pull request Feb 17, 2023
Closed
@bors
Copy link
Contributor

bors commented Feb 18, 2023

⌛ Testing commit 1971438 with merge fabfd1f...

@bors
Copy link
Contributor

bors commented Feb 18, 2023

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing fabfd1f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 18, 2023
@bors bors merged commit fabfd1f into rust-lang:master Feb 18, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 18, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fabfd1f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.1% [0.1%, 0.1%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -3.4% [-3.4%, -3.4%] 2
All ❌✅ (primary) 0.1% [0.1%, 0.1%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 5.2% [4.3%, 5.9%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper approved these changes

Assignees

@cuviper cuviper

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.