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: Fix fn items, closures, and Fn trait objects, and more #116404

Closed
wants to merge 5 commits into from
Closed

CFI: Fix fn items, closures, and Fn trait objects, and more #116404

wants to merge 5 commits into from

Conversation

rcvalle
Copy link
Member

@rcvalle rcvalle commented Oct 3, 2023 •
edited
Loading

This PR:

  • Finishes adding CFI support to the Rust Standard Library (i.e., programs build and run using the Cargo build-std feature, and all Rust Standard Library tests pass). (Resolves CFI: core and std have explict CFI violations #115199.)
  • Fixes casting between function items, closures, and Fn trait objects by transforming function items, closures, and Fn trait objects into function pointers for encoding.
  • Fixes drop and drop_in_place by transforming self of drop and drop_in_place methods into a Drop trait object and handling drop method calls separatedly (and also transforming self into a Drop trait object). This has the same effect of what CFI does for impl methods by transforming self into trait objects that match the trait objects in the calls after type erasure.
  • Fixes ICE: cfi: encode_ty: unexpected 'CoroutineWitness' #122705 by adding support for encoding ty:CoroutineClosure.
  • Fix casting between methods and function pointers by assigning a secondary type id to methods with their concrete self so they can be used as function pointers. (Fix CFI breaks on trait Address Taken functions #115953 and resolves cfi: Store type erasure witness for Argument #115954.)

This is a zero cost solution for both the Rust compiler and compiled programs (i.e., without generating any shims/trampolines or changing the compiler code generation at all) for the issues described and solutions proposed in #121962 and #122573.

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2023

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 Oct 3, 2023
@rcvalle rcvalle added the PG-exploit-mitigations Project group: Exploit mitigations label Oct 3, 2023
@rcvalle
Copy link
Member Author

rcvalle commented Oct 3, 2023

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned oli-obk Oct 3, 2023
@rcvalle rcvalle force-pushed the rust-cfi-fix-115199-3 branch from 5157709 to 256cab2 Compare October 3, 2023 22:58
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rcvalle rcvalle force-pushed the rust-cfi-fix-115199-3 branch from 256cab2 to a25171b Compare October 3, 2023 23:27
@rcvalle rcvalle mentioned this pull request Oct 4, 2023
Merged
@bors
Copy link
Contributor

bors commented Oct 11, 2023

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

@rcvalle rcvalle force-pushed the rust-cfi-fix-115199-3 branch from a25171b to d676343 Compare October 24, 2023 20:49
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 24, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 24, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rcvalle rcvalle force-pushed the rust-cfi-fix-115199-3 branch from d676343 to df8c3a8 Compare October 24, 2023 20:59
@compiler-errors
Copy link
Member

One nit @rustbot 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 Oct 25, 2023
@rcvalle rcvalle force-pushed the rust-cfi-fix-115199-3 branch from df8c3a8 to dd04959 Compare October 25, 2023 19:54
@rcvalle rcvalle force-pushed the rust-cfi-fix-115199-3 branch 3 times, most recently from d5b79c1 to 3e28128 Compare October 26, 2023 16:39
@rcvalle
Copy link
Member Author

rcvalle commented Oct 26, 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 Oct 26, 2023
@bors
Copy link
Contributor

bors commented Nov 16, 2023

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

@rcvalle rcvalle force-pushed the rust-cfi-fix-115199-3 branch 3 times, most recently from bc7ae20 to 6f146f5 Compare March 22, 2024 21:06
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rcvalle rcvalle force-pushed the rust-cfi-fix-115199-3 branch from 4abea48 to f28ac45 Compare March 22, 2024 23:08
rcvalle added 5 commits March 22, 2024 17:45
@rcvalle
CFI: Fix fn items, closures, and Fn trait objects
e301a29 
Fix casting between function items, closures, and Fn trait objects by
transforming function items, closures, and Fn trait objects into
function pointers for encoding.
@rcvalle
CFI: Fix drop and drop_in_place
4319fbe 
Fix drop and drop_in_place by transforming self of drop and
drop_in_place methods into a Drop trait object and handling drop method
calls separatedly (and also transforming self into a Drop trait object).
This has the same effect of what CFI does for impl methods by
transforming self into trait objects that match the trait objects in the
calls after type erasure.
@rcvalle
CFI: Fix encode_ty: unexpected 'CoroutineWitness'
56cf23c 
Fix rust-lang#122705 by adding support for encoding `ty:CoroutineClosure`.
@rcvalle
Revert "CFI: Skip non-passed arguments"
cbcb16d 
This reverts commit f2f0d25.
@rcvalle
CFI: Fix methods as function pointer cast
66b2708 
Fix casting between methods and function pointers by assigning a
secondary type id to methods with their concrete self so they can be
used as function pointers.
@rcvalle rcvalle force-pushed the rust-cfi-fix-115199-3 branch from f28ac45 to 66b2708 Compare March 23, 2024 00:52
@bors
Copy link
Contributor

bors commented Mar 24, 2024

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

@compiler-errors
Copy link
Member

Given that this PR is doing like 5+ different things, I'd like to see this broken up into several different PRs to be honest.

Like, there's no reason why the PR which changes coroutine encoding needs to go into the same PR which changes the way we encode the receiver for Drop.

@rcvalle
Copy link
Member Author

rcvalle commented Mar 25, 2024

Given that this PR is doing like 5+ different things, I'd like to see this broken up into several different PRs to be honest.

Like, there's no reason why the PR which changes coroutine encoding needs to go into the same PR which changes the way we encode the receiver for Drop.

Sure. I'll split it into smaller self-contained PRs.

@compiler-errors compiler-errors mentioned this pull request Mar 25, 2024
Closed
This was referenced Mar 26, 2024
Merged
Merged
Draft
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 27, 2024
@matthiaskrgr
Rollup merge of rust-lang#123075 - rcvalle:rust-cfi-fix-drop-drop-in-…
6464e5b 
…place, r=compiler-errors

CFI: Fix drop and drop_in_place

Fix drop and drop_in_place by transforming self of drop and drop_in_place methods into a Drop trait objects.

This was split off from rust-lang#116404.

cc `@compiler-errors` `@workingjubilee`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2024
@rust-timer
Unrolled build for rust-lang#123075
7db49d4 
Rollup merge of rust-lang#123075 - rcvalle:rust-cfi-fix-drop-drop-in-place, r=compiler-errors

CFI: Fix drop and drop_in_place

Fix drop and drop_in_place by transforming self of drop and drop_in_place methods into a Drop trait objects.

This was split off from rust-lang#116404.

cc `@compiler-errors` `@workingjubilee`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2024
@bors
Auto merge of rust-lang#123071 - rcvalle:rust-cfi-fix-method-fn-ptr-c…
30e054f 
…ast, r=compiler-errors

CFI: Fix methods as function pointer cast

Fix casting between methods and function pointers by assigning a secondary type id to methods with their concrete self so they can be used as function pointers.

This was split off from rust-lang#116404.

cc `@compiler-errors` `@workingjubilee`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 29, 2024
@bors
Auto merge of rust-lang#123071 - rcvalle:rust-cfi-fix-method-fn-ptr-c…
58dcd1f 
…ast, r=compiler-errors

CFI: Fix methods as function pointer cast

Fix casting between methods and function pointers by assigning a secondary type id to methods with their concrete self so they can be used as function pointers.

This was split off from rust-lang#116404.

cc `@compiler-errors` `@workingjubilee`
This was referenced Mar 29, 2024
Draft
Open
@Jules-Bertholet
Copy link
Contributor

@rustbot label A-sanitizers

@rustbot rustbot added the A-sanitizers Area: Sanitizers for correctness and code quality label Apr 8, 2024
@Dylan-DPC
Copy link
Member

Closing this as it's being split into multiple prs

@Dylan-DPC Dylan-DPC closed this Apr 8, 2024
@rcvalle rcvalle deleted the rust-cfi-fix-115199-3 branch April 22, 2024 23:07
@rcvalle rcvalle restored the rust-cfi-fix-115199-3 branch April 22, 2024 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee left review comments

@compiler-errors compiler-errors compiler-errors requested changes

Assignees

@compiler-errors compiler-errors

Labels
A-sanitizers Area: Sanitizers for correctness and code quality PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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
Milestone
No milestone