CFI: Repair vtables without altering types #122573
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This query computes the trait object, complete with associated type projections for its supertraits, from a trait ref. This is intended for use by CFI shimming.
In preparation to add recursive instance_defs, move this logic to its own convenience method.
Factored out to minimize the amount of noise in the main CfiShim defining patch.
|
Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in tests/ui/sanitizer cc @rust-lang/project-exploit-mitigations, @rcvalle Some changes occurred in tests/codegen/sanitizer cc @rust-lang/project-exploit-mitigations, @rcvalle Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred in compiler/rustc_symbol_mangling/src/typeid cc @rust-lang/project-exploit-mitigations, @rcvalle |
rustbot
added
PG-exploit-mitigations
|
As a note, unlike the main PR, I have not:
I have only ensured the tests are working. If this approach is selected I will validate it further. |
This comment has been minimized.
This comment has been minimized.
We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances.
Indirect calls through vtables (trait objects or drop_in_place) expect to have an alias set based on `dyn Trait` at the call-site. The actual implementations have aslias sets based on `MyImplType`. These shims create a separate `InstanceDef`, allowing a different type to be assigned. These function for both CFI and KCFI, as they have a single principal type.
Fixes: 118761
Rust will occasionally rely on fn((), X) -> Y being compatible with fn(X) -> Y, since () is a non-passed argument. Relax CFI by choosing not to encode non-passed arguments.
Current `transform_ty` attempts to avoid cycles when normalizing
`#[repr(transparent)]` types to their interior, but runs afoul of this
pattern used in `self_cell`:
```
struct X<T> {
x: u8,
p: PhantomData<T>,
}
#[repr(transparent)]
struct Y(X<Y>);
```
When attempting to normalize Y, it will still cycle indefinitely. By
using a types-visited list, this will instead get expanded exactly
one layer deep to X<Y>, and then stop, not attempting to normalize `Y`
any further.
CFI shimming means they're not gauranteed to be pre-generated. Traditionally, the base vtable has all the elements of the supertrait vtable, and so visiting the base vtable implies you don't need to visit the supertrait vtable. However, with CFI the base vtable entries will have invocation type `dyn Child`, and the parent vtable will have invocation type `dyn Parent`, so they aren't actually the same instance, and both must be visited.
Additional trait bounds beyond the principal trait and its implications are not possible in the vtable. This means that if a receiver is `&dyn Foo + Send`, the function will only be expecting `&dyn Foo`. This strips those auto traits off before CFI encoding.
In user-facing Rust, `dyn` always has at least one predicate following it. Unfortunately, because we filter out marker traits and `dyn Sync` is, for example, legal, this results in us having `dyn` types with no predicates on occasion. This patch handles cases where there are no predicates in a `dyn` type.
|
☔ The latest upstream changes (presumably #122375) made this pull request unmergeable. Please resolve the merge conflicts. |
|
For anyone that commented on this PR and also anyone new to this PR, see my comments in #116404 (comment). |
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Mar 23, 2024
…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Mar 24, 2024
…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Mar 24, 2024
…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Mar 24, 2024
…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Mar 24, 2024
…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 24, 2024
Rollup merge of rust-lang#122879 - maurer:callsite-instances, r=workingjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.
RenjiSann
pushed a commit
to RenjiSann/rust
that referenced
this pull request
Mar 25, 2024
…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.
rustbot
added
S-waiting-on-author
|
I'm closing this PR as I'm landing smaller pieces of this one by one with some variations. The branch will still be there for reference until we finish fixing the overall vtable issues in case we need to crib solutions from it. |
rcvalle
added a commit
to rcvalle/rust
that referenced
this pull request
Mar 29, 2024
Since we're now using an approach which is a variant of rust-lang#123082 (that transforms closures into dynamic Fn traits but isolating it to the Fn call methods only) instead of rust-lang#121962 or rust-lang#122573, skipping non-passed arguments shouldn't be necessary KCFI anymore and we can claim back the reduced granularity. This reverts commit f2f0d25.
rcvalle
added a commit
to rcvalle/rust
that referenced
this pull request
Mar 29, 2024
Since we're now using an approach which is a variant of rust-lang#123082 (that transforms closures into dynamic Fn traits but isolating it to the Fn call methods only) instead of rust-lang#121962 or rust-lang#122573, skipping non-passed arguments shouldn't be necessary for KCFI anymore and we can claim back the reduced granularity. This reverts commit f2f0d25.
maurer
pushed a commit
to maurer/rust
that referenced
this pull request
Apr 4, 2024
Since we're now using an approach which is a variant of rust-lang#123082 (that transforms closures into dynamic Fn traits but isolating it to the Fn call methods only) instead of rust-lang#121962 or rust-lang#122573, skipping non-passed arguments shouldn't be necessary for KCFI anymore and we can claim back the reduced granularity. This reverts commit f2f0d25.
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an alternative to #121962 intended to showcase two possible design alternatives discussed in the doc there:
Instances so that they fit the principally-typed expectations of KCFI, we feed theInstanceinformation into the CFI typeinfo generator and it performs the adjustments.Note that this does not remove the new Instance variant generation. I am not convinced it is possible to support KCFI without generating variant instances or something equivalent, as argued in the design doc.
r? @workingjubilee