Skip to content

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

Closed
rcvalle wants to merge 5 commits into
rust-lang:masterfrom
rcvalle:rust-cfi-fix-115199-3
Closed

CFI: Fix fn items, closures, and Fn trait objects, and more#116404
rcvalle wants to merge 5 commits into
rust-lang:masterfrom
rcvalle:rust-cfi-fix-115199-3

Conversation

@rcvalle

@rcvallercvalle commented Oct 3, 2023

Copy link
Copy Markdown
Member

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). (ResolvesCFI: 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.
  • FixesICE: 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. (FixCFI breaks on trait Address Taken functions #115953 and resolvescfi: 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
Copy Markdown
Collaborator

r? @oli-obk

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

@rustbotrustbot 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
@rcvallercvalle added the PG-exploit-mitigations Project group: Exploit mitigations label Oct 3, 2023
@rcvalle

Copy link
Copy Markdown
MemberAuthor

r? @compiler-errors

@rcvalle
rcvalleforce-pushed the rust-cfi-fix-115199-3 branch from 5157709 to 256cab2CompareOctober 3, 2023 22:58
@rust-log-analyzer

This comment has been minimized.

@bors

bors commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

@rcvalle
rcvalleforce-pushed the rust-cfi-fix-115199-3 branch from a25171b to d676343CompareOctober 24, 2023 20:49
@rustbotrustbot 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
Copy Markdown
Collaborator

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rcvalle
rcvalleforce-pushed the rust-cfi-fix-115199-3 branch from d676343 to df8c3a8CompareOctober 24, 2023 20:59
Comment threadcompiler/rustc_middle/src/ty/sty.rs Outdated
@compiler-errors

Copy link
Copy Markdown
Contributor

One nit @rustbot author

@rustbotrustbot 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
rcvalleforce-pushed the rust-cfi-fix-115199-3 branch from df8c3a8 to dd04959CompareOctober 25, 2023 19:54
Comment threadcompiler/rustc_middle/src/ty/sty.rs Outdated
@rcvalle
rcvalleforce-pushed the rust-cfi-fix-115199-3 branch 3 times, most recently from d5b79c1 to 3e28128CompareOctober 26, 2023 16:39
@rcvalle

Copy link
Copy Markdown
MemberAuthor

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

@rustbotrustbot 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

bors commented Nov 16, 2023

Copy link
Copy Markdown
Collaborator

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

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

This comment has been minimized.

@rcvalle
rcvalleforce-pushed the rust-cfi-fix-115199-3 branch from 4abea48 to f28ac45CompareMarch 22, 2024 23:08
Fix casting between function items, closures, and Fn trait objects by
transforming function items, closures, and Fn trait objects into
function pointers for encoding.
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.
Fixrust-lang#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.
@rcvalle
rcvalleforce-pushed the rust-cfi-fix-115199-3 branch from f28ac45 to 66b2708CompareMarch 23, 2024 00:52
@bors

bors commented Mar 24, 2024

Copy link
Copy Markdown
Collaborator

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

@compiler-errors

Copy link
Copy Markdown
Contributor

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
Copy Markdown
MemberAuthor

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 27, 2024
…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
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
…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
…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`
@Jules-Bertholet

Copy link
Copy Markdown
Contributor

@rustbot label A-sanitizers

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

Copy link
Copy Markdown
Member

Closing this as it's being split into multiple prs

@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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-sanitizersArea: Sanitizers for correctness and code qualityPG-exploit-mitigationsProject group: Exploit mitigationsS-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author.T-bootstrapRelevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)T-compilerRelevant 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.

ICE: cfi: encode_ty: unexpected 'CoroutineWitness' CFI breaks on trait Address Taken functions CFI: core and std have explict CFI violations

11 participants

@rcvalle@rustbot@rust-log-analyzer@bors@compiler-errors@JohnCSimon@maurer@Jules-Bertholet@Dylan-DPC@workingjubilee@oli-obk