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

CFI: Support provided methods on traits #127295

Merged
merged 1 commit into from
Jul 19, 2024
Merged

CFI: Support provided methods on traits #127295

merged 1 commit into from
Jul 19, 2024

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Jul 3, 2024

Provided methods currently don't get type erasure performed on them because they are not in an impl block. If we are instantiating a method that is an associated item, but not in an impl block, treat it as a provided method instead.

@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2024

r? @estebank

rustbot has assigned @estebank.
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 PG-exploit-mitigations Project group: Exploit mitigations 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 Jul 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2024

Some changes occurred in tests/ui/sanitizer

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

Some changes occurred in compiler/rustc_sanitizers

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

@rcvalle
Copy link
Member

rcvalle commented Jul 9, 2024

LGTM (@estebank FYI). Thank you for your time and for working on this, @maurer! Much appreciated.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 12, 2024

📌 Commit 81b1f92 has been approved by estebank

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 Jul 12, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 12, 2024
@matthiaskrgr
Rollup merge of rust-lang#127295 - maurer:default-impl-cfi, r=estebank
d401c47 
CFI: Support provided methods on traits

Provided methods currently don't get type erasure performed on them because they are not in an `impl` block. If we are instantiating a method that is an associated item, but *not* in an impl block, treat it as a provided method instead.
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 12, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 12, 2024
@bors
Auto merge of rust-lang#127657 - matthiaskrgr:rollup-qtuuo4o, r=matth…
2d31345 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#126502 (Ignore allocation bytes in some mir-opt tests)
 - rust-lang#126606 (Guard against calling `libc::exit` multiple times on Linux.)
 - rust-lang#126922 (add lint for inline asm labels that look like binary)
 - rust-lang#127295 (CFI: Support provided methods on traits)
 - rust-lang#127310 (Fix import suggestion ice)
 - rust-lang#127535 (Fire unsafe_code lint on unsafe extern blocks)
 - rust-lang#127631 (Remove `fully_normalize`)
 - rust-lang#127632 (Implement `precise_capturing` support for rustdoc)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee
Copy link
Member

Failed in #127657 (comment)

@bors r-

@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 Jul 12, 2024
@maurer maurer force-pushed the default-impl-cfi branch 2 times, most recently from da3f7a2 to abf6aa7 Compare July 17, 2024 17:21
@maurer
CFI: Support provided methods on traits
2abdc4e 
Provided methods currently don't get type erasure performed on them
because they are not in an `impl` block. If we are instantiating a
method that is an associated item, but *not* in an impl block, treat it
as a provided method instead.
@maurer
Copy link
Contributor Author

maurer commented Jul 17, 2024

Non-items like VTableShims have an "associated item" of their corresponding trait method on their def_id. This was incorrectly triggering the "provided method" logic in cases like FnOnce because the self parameter was producing a shim.

This was only causing failures on KCFI because CFI would tag the method with both the erased and concrete types, and the concrete pass would get the correct logic running. It only happened on the nopt target because otherwise the optimizer was converting these closure calls into direct calls before CFI was applied.

I've restricted the provided method detection to only occur on InstanceKind::Item, as the others should not be used for provided methods.

@rustbot ready

@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 Jul 17, 2024
@rcvalle
Copy link
Member

rcvalle commented Jul 18, 2024

LGTM (@estebank @workingjubilee FYI).

@workingjubilee
Copy link
Member

isn't codegen wonderful? everything's so optimization-dependent.

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jul 19, 2024

📌 Commit 2abdc4e has been approved by estebank

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 Jul 19, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 19, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
@bors
Auto merge of rust-lang#127982 - matthiaskrgr:rollup-nzyvphj, r=matth…
ff4b398 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#127295 (CFI: Support provided methods on traits)
 - rust-lang#127814 (`C-cmse-nonsecure-call`: improved error messages)
 - rust-lang#127949 (fix: explain E0120 better cover cases when its raised)
 - rust-lang#127966 (Use structured suggestions for unconstrained generic parameters on impl blocks)
 - rust-lang#127976 (Lazy type aliases: Diagostics: Detect bivariant ty params that are only used recursively)
 - rust-lang#127978 (Avoid ref when using format! for perf)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6ae6f8b into rust-lang:master Jul 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
@rust-timer
Unrolled build for rust-lang#127295
6dff423 
Rollup merge of rust-lang#127295 - maurer:default-impl-cfi, r=estebank

CFI: Support provided methods on traits

Provided methods currently don't get type erasure performed on them because they are not in an `impl` block. If we are instantiating a method that is an associated item, but *not* in an impl block, treat it as a provided method instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@estebank estebank

Labels
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.
6 participants
@maurer @rustbot @rcvalle @estebank @bors @workingjubilee