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

Disable CFI for weakly linked syscalls #138002

Merged
merged 2 commits into from
Mar 12, 2025

Conversation

1c3t3a
Copy link
Member

@1c3t3a 1c3t3a commented Mar 4, 2025

Currently, when enabling CFI via -Zsanitizer=cfi and executing e.g. std::sys::random::getrandom, we can observe a CFI violation. This is the case for all consumers of the std::sys::pal::weak::syscall macro, as it is defining weak functions which don't show up in LLVM IR metadata. CFI fails for all these functions.

Similar to other such cases in
#115199, this change stops emitting the CFI typecheck for consumers of the macro via the #[no_sanitize(cfi)] attribute.

r? @rcvalle

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 4, 2025
@bjorn3
Copy link
Member

bjorn3 commented Mar 4, 2025

There are a couple other places where the weak!() macro is used. Should those also get #[no_sanitize(cfi)]? And is there a way to do this automatically for every call of a weak function instead?

@rcvalle
Copy link
Member

rcvalle commented Mar 4, 2025

is there a way to do this automatically for every call of a weak function instead?

That would be the best approach for now if we could do this only for functions (i.e., call sites) that indirectly call weakly linked functions.

@Noratrieb
Copy link
Member

Is there a reason weak symbols don't get LLVM metadata?

@1c3t3a 1c3t3a force-pushed the fix-std-cfi-violation branch from 3ef2ae6 to ddfa9d3 Compare March 5, 2025 14:46
@1c3t3a
Copy link
Member Author

1c3t3a commented Mar 5, 2025

There are a couple other places where the weak!() macro is used. Should those also get #[no_sanitize(cfi)]?

Yes! I updated the PR to all callsites.

And is there a way to do this automatically for every call of a weak function instead?

There is no easy way of marking this as no_sanitize as part of the weak! macro, as it's currently just creating a member that is callable and has #[linkage = "extern_weak")]. The callsite of the resulting function pointer is not controlled by the macro. I think there are two alternatives to this if we don't want to manually go to the callsites (like this PR does it):

  • Don't emit the CFI type check at all for functions that are "extern weak". CFI would always be expected to fail in these cases.
  • Update the no_sanitize attribute to work well with #[linkage = "extern_weak")] statics.

But I am not very familiar with this, so I am happy to learn about a good alternative!

@rcvalle
Copy link
Member

rcvalle commented Mar 7, 2025

Is there a reason weak symbols don't get LLVM metadata?

I don't know. I think this is likely a bug and that the compiler should emit metadata for those. If there are no issues with it, I'll land this workaround (similarly to #115200) while we investigate and work on emitting metadata for those if possible.

@rcvalle
Copy link
Member

rcvalle commented Mar 7, 2025

@bors r+

@bors
Copy link
Contributor

bors commented Mar 7, 2025

📌 Commit ddfa9d3 has been approved by rcvalle

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 7, 2025
@Noratrieb
Copy link
Member

@bors r-

please add a comment with an explanation to every use of #[no_sanitize(cfi)]

@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 Mar 7, 2025
@rcvalle
Copy link
Member

rcvalle commented Mar 8, 2025

Maybe also point to #115199 for reference.

@bors
Copy link
Contributor

bors commented Mar 9, 2025

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

1c3t3a added 2 commits March 10, 2025 08:51
Currently, when enabling CFI via -Zsanitizer=cfi and executing e.g.
std::sys::random::getrandom, we can observe a CFI violation. This is
the case for all consumers of the std::sys::pal::weak::weak macro,
as it is defining weak functions which don't show up in LLVM IR
metadata. CFI fails for all these functions.

Similar to other such cases in
rust-lang#115199, this change stops
emitting the CFI typecheck for consumers of the macro via the
\#[no_sanitize(cfi)] attribute.
@1c3t3a 1c3t3a force-pushed the fix-std-cfi-violation branch from ddfa9d3 to e5dc1e3 Compare March 10, 2025 09:49
@1c3t3a
Copy link
Member Author

1c3t3a commented Mar 10, 2025

Fair point! I added a comment to every use of #[no_sanitize(cfi)].

@rcvalle
Copy link
Member

rcvalle commented Mar 10, 2025

Thank you, Bastian!

@rcvalle
Copy link
Member

rcvalle commented Mar 10, 2025

@bors r+

@bors
Copy link
Contributor

bors commented Mar 10, 2025

📌 Commit e5dc1e3 has been approved by rcvalle

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 10, 2025
@rcvalle rcvalle added PG-exploit-mitigations Project group: Exploit mitigations A-sanitizers Area: Sanitizers for correctness and code quality labels Mar 10, 2025
@rcvalle rcvalle added the A-control-flow-integrity Area: Control Flow Integrity (CFI) security mitigation label Mar 10, 2025
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 10, 2025
…valle

Disable CFI for weakly linked syscalls

Currently, when enabling CFI via -Zsanitizer=cfi and executing e.g. std::sys::random::getrandom, we can observe a CFI violation. This is the case for all consumers of the std::sys::pal::weak::syscall macro, as it is defining weak functions which don't show up in LLVM IR metadata. CFI fails for all these functions.

Similar to other such cases in
rust-lang#115199, this change stops emitting the CFI typecheck for consumers of the macro via the `#[no_sanitize(cfi)]` attribute.

r? `@rcvalle`
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 10, 2025
…valle

Disable CFI for weakly linked syscalls

Currently, when enabling CFI via -Zsanitizer=cfi and executing e.g. std::sys::random::getrandom, we can observe a CFI violation. This is the case for all consumers of the std::sys::pal::weak::syscall macro, as it is defining weak functions which don't show up in LLVM IR metadata. CFI fails for all these functions.

Similar to other such cases in
rust-lang#115199, this change stops emitting the CFI typecheck for consumers of the macro via the `#[no_sanitize(cfi)]` attribute.

r? ``@rcvalle``
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 11, 2025
…valle

Disable CFI for weakly linked syscalls

Currently, when enabling CFI via -Zsanitizer=cfi and executing e.g. std::sys::random::getrandom, we can observe a CFI violation. This is the case for all consumers of the std::sys::pal::weak::syscall macro, as it is defining weak functions which don't show up in LLVM IR metadata. CFI fails for all these functions.

Similar to other such cases in
rust-lang#115199, this change stops emitting the CFI typecheck for consumers of the macro via the `#[no_sanitize(cfi)]` attribute.

r? ```@rcvalle```
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2025
Rollup of 16 pull requests

Successful merges:

 - rust-lang#126856 (remove deprecated tool `rls`)
 - rust-lang#136932 (Reduce formatting `width` and `precision` to 16 bits)
 - rust-lang#137314 (change definitely unproductive cycles to error)
 - rust-lang#137612 (Update bootstrap to edition 2024)
 - rust-lang#138002 (Disable CFI for weakly linked syscalls)
 - rust-lang#138052 (strip `-Wlinker-messages` wrappers from `rust-lld` rmake test)
 - rust-lang#138063 (Improve `-Zunpretty=hir` for parsed attrs)
 - rust-lang#138109 (make precise capturing args in rustdoc Json typed)
 - rust-lang#138147 (Add maintainers for powerpc64le-unknown-linux-gnu)
 - rust-lang#138245 (stabilize `ci_rustc_if_unchanged_logic` test for local environments)
 - rust-lang#138296 (Remove `AdtFlags::IS_ANONYMOUS` and `Copy`/`Clone` condition for anonymous ADT)
 - rust-lang#138300 (add tracking issue for unqualified_local_imports)
 - rust-lang#138307 (Allow specifying glob patterns for try jobs)
 - rust-lang#138313 (Update books)
 - rust-lang#138315 (use next_back() instead of last() on DoubleEndedIterator)
 - rust-lang#138318 (Rustdoc: remove a bunch of `@ts-expect-error` from main.js)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 11, 2025
…valle

Disable CFI for weakly linked syscalls

Currently, when enabling CFI via -Zsanitizer=cfi and executing e.g. std::sys::random::getrandom, we can observe a CFI violation. This is the case for all consumers of the std::sys::pal::weak::syscall macro, as it is defining weak functions which don't show up in LLVM IR metadata. CFI fails for all these functions.

Similar to other such cases in
rust-lang#115199, this change stops emitting the CFI typecheck for consumers of the macro via the `#[no_sanitize(cfi)]` attribute.

r? ````@rcvalle````
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2025
Rollup of 18 pull requests

Successful merges:

 - rust-lang#126856 (remove deprecated tool `rls`)
 - rust-lang#137314 (change definitely unproductive cycles to error)
 - rust-lang#137504 (Move methods from Map to TyCtxt, part 4.)
 - rust-lang#137701 (Convert `ShardedHashMap` to use `hashbrown::HashTable`)
 - rust-lang#137967 ([AIX] Fix hangs during testing)
 - rust-lang#138002 (Disable CFI for weakly linked syscalls)
 - rust-lang#138052 (strip `-Wlinker-messages` wrappers from `rust-lld` rmake test)
 - rust-lang#138063 (Improve `-Zunpretty=hir` for parsed attrs)
 - rust-lang#138109 (make precise capturing args in rustdoc Json typed)
 - rust-lang#138147 (Add maintainers for powerpc64le-unknown-linux-gnu)
 - rust-lang#138245 (stabilize `ci_rustc_if_unchanged_logic` test for local environments)
 - rust-lang#138296 (Remove `AdtFlags::IS_ANONYMOUS` and `Copy`/`Clone` condition for anonymous ADT)
 - rust-lang#138300 (add tracking issue for unqualified_local_imports)
 - rust-lang#138307 (Allow specifying glob patterns for try jobs)
 - rust-lang#138313 (Update books)
 - rust-lang#138315 (use next_back() instead of last() on DoubleEndedIterator)
 - rust-lang#138318 (Rustdoc: remove a bunch of `@ts-expect-error` from main.js)
 - rust-lang#138330 (Remove unnecessary `[lints.rust]` sections.)

Failed merges:

 - rust-lang#137147 (Add exclude to config.toml)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 11, 2025
…valle

Disable CFI for weakly linked syscalls

Currently, when enabling CFI via -Zsanitizer=cfi and executing e.g. std::sys::random::getrandom, we can observe a CFI violation. This is the case for all consumers of the std::sys::pal::weak::syscall macro, as it is defining weak functions which don't show up in LLVM IR metadata. CFI fails for all these functions.

Similar to other such cases in
rust-lang#115199, this change stops emitting the CFI typecheck for consumers of the macro via the `#[no_sanitize(cfi)]` attribute.

r? `````@rcvalle`````
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2025
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#137715 (Allow int literals for pattern types with int base types)
 - rust-lang#138002 (Disable CFI for weakly linked syscalls)
 - rust-lang#138051 (Add support for downloading GCC from CI)
 - rust-lang#138231 (Prevent ICE in autodiff validation by emitting user-friendly errors)
 - rust-lang#138245 (stabilize `ci_rustc_if_unchanged_logic` test for local environments)
 - rust-lang#138256 (Do not feed anon const a type that references generics that it does not have)
 - rust-lang#138284 (Do not write user type annotation for const param value path)
 - rust-lang#138296 (Remove `AdtFlags::IS_ANONYMOUS` and `Copy`/`Clone` condition for anonymous ADT)
 - rust-lang#138352 (miri native_calls: ensure we actually expose *mutable* provenance to the memory FFI can access)
 - rust-lang#138354 (remove redundant `body`  arguments)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9746ac5 into rust-lang:master Mar 12, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-control-flow-integrity Area: Control Flow Integrity (CFI) security mitigation A-sanitizers Area: Sanitizers for correctness and code quality O-unix Operating system: Unix-like 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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants