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

CFI: Abstract Closures and Coroutines #123106

Merged
merged 2 commits into from
Mar 30, 2024
Merged

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Mar 26, 2024

This will abstract coroutines in a moment, it's just abstracting closures for now to show @rcvalle

This uses the same principal as the methods on traits - figure out the dyn type representing the fn trait, instantiate it, and attach that alias set. We're essentially just computing how we would be called in a dynamic context, and attaching that.

@rustbot
Copy link
Collaborator

rustbot commented Mar 26, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
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 Mar 26, 2024
@maurer
Copy link
Contributor Author

maurer commented Mar 26, 2024

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned TaKO8Ki Mar 26, 2024
@maurer maurer marked this pull request as ready for review March 26, 2024 21:47
@rustbot
Copy link
Collaborator

rustbot commented Mar 26, 2024

Some changes occurred in compiler/rustc_symbol_mangling/src/typeid

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

Some changes occurred in tests/ui/sanitizer

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

@rcvalle
Copy link
Member

rcvalle commented Mar 27, 2024

How does this handle Fn subtraits? More specifically:

trait FnSubtrait: Fn() {}
impl<T: Fn()> FnSubtrait for T {}

fn call_dynamic_fn_subtrait(f: &dyn FnSubtrait) {
     f();
}

fn main() {
    call_dynamic_fn_subtrait(&|| {});
}

And how does this handle Fn subtraits of multiple Fn supertraits, like the example you gave at #123082 (comment) (i.e., trait Foo: Fn() + Fn(i32) -> u8 {})?

@maurer
Copy link
Contributor Author

maurer commented Mar 27, 2024

How does this handle Fn subtraits? More specifically:

trait FnSubtrait: Fn() {}
impl<T: Fn()> FnSubtrait for T {}

fn call_dynamic_fn_subtrait(f: &dyn FnSubtrait) {
     f();
}

fn main() {
    call_dynamic_fn_subtrait(&|| {});
}

And how does this handle Fn subtraits of multiple Fn supertraits, like the example you gave at #123082 (comment) (i.e., trait Foo: Fn() + Fn(i32) -> u8 {})?

Supertraits are entirely handled by #123012 - if the callsite tries to call a method on Fn(i32), then despite having a dyn Foo, it will check the vtable entry against a fn(&dyn Fn(i32), (i32)) -> u8. The dyn Foo vtable will look roughly like this:

  • fn drop_in_place(*mut ???) (encoding omitted, we're deciding this elsewhere)
  • fn foo(&self) encodes to fn foo(&dyn Foo)
  • fn call<()>(&self) encodes to fn call(&dyn Fn())
  • fn call<(i32)>(&self, (i32)) -> u8 encodes to fn call(&dyn Fn(i32) -> u8, (i32)) -> u8

At the callsite for each, #123012 would translate the Virtual call to a call through the defining trait, which would result in the same expected signatures you see in the encoding.

@rcvalle
Copy link
Member

rcvalle commented Mar 27, 2024

As I we discussed on Zulip, I had two observations about this approach:

  1. It doesn't seem to improve granularity if either one of the main or secondary type ids group them/put them all in the same group.
  2. There will be multiple Reifying of types (which is what you're planning to do to solve the method to function pointer casting for KCFI anyway in CFI: Support function pointers for trait methods #123052). Since we won't be doing this for CFI, I guess it'd require a secondary type id for all of these, which brings us back to the same broadness of checks that we've at CFI: Fix fn items, closures, and Fn trait objects #123082, but with diverging implementations for CFI and KCFI.

And as you noted:

It would improve:

  1. Closures with captures would no longer be valid destinations at call sites that call function pointers.
  2. Call sites that call concrete closures would only have the specific concrete closures as valid destinations, but these closures would still have the secondary type ids attached.

And as I noted:

  1. Because these closures (2) would still have the secondary type ids attached, they would still be valid destinations at other unintended call sites.

We've come a far and long way away from the original CfiShim proposal, which is great. If you understand the possible costs of these added levels of indirections for KCFI for cache coherence/locality and performance, possible introduction of gadgets or bypasses, and is willing to move forward with these on the Linux kernel (cc @ojeda), Android, and other embedded devices, I'm okay with moving forward with these changes. If this becomes an issue later, we can always revisit #123082, which works for both CFI and KCFI and doesn't require reifying types.

@bors
Copy link
Contributor

bors commented Mar 27, 2024

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

.expect("No call-family function on closure-like Fn trait?")
.def_id;

instance.def = ty::InstanceDef::Virtual(call, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I said this earlier but I don't think I got a response, so I'll say it again: Why are you using InstanceDef::Virtual over just using InstanceDef::Item? You have an associated item def id, so just use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I answered this on the previous change, but I also left a comment above it in the first half of this if statement when I set Virtual the first time explaining. The short version of that comment is that this is an adjusted type for the case where the callsite will have a Virtual. I wanted to make the callsite and the declaration match as closely as possible to make it harder to make a mistake.

This also means that fn_abi will use its special logic for unwrapping receivers in the case that the receiver is not already directly a pointer to a dyn Trait - it'll unwrap the ZST repeatedly until it gets to one. This is load-bearing in the top half of the if condition, because alternate receivers happen.

Finally, in the future we could choose to support the fact that Virtual &dyn Trait in receiver position is different from any other instance's &dyn Trait in receiver position, in that it's a single pointer instead of double-wide, and encode that differently (we don't do this today).

All that said, since none of the Fn-trait receivers use receivers that need unwrapping, I can replace this with Item if you really want me to.

tests/ui/sanitizer/cfi-closures.rs Show resolved Hide resolved
@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 Mar 27, 2024
@maurer
Copy link
Contributor Author

maurer commented Mar 27, 2024

I addressed some of the comments, but I don't think we need to generalize async closures because the AsyncFn trait family is not object safe. If I've misunderstood, lmk and I can adjust.

I wanted to add a test that we don't barf on async || () (my previous code would have), so I tacked the CoroutineWitness commit on beforehand in order to allow that test to work.

The HRTB test I added is pretty simple, but I expect that if we can handle any HRTB, it's likely we can handle all of it. If you think it needs to be more broad, I can look around the test suite for HRTB edge cases to add.

@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 Mar 27, 2024
@maurer maurer force-pushed the cfi-closures branch 2 times, most recently from befd136 to 6bce455 Compare March 27, 2024 22:38
@maurer
Copy link
Contributor Author

maurer commented Mar 27, 2024

I've made all the requested adjustments other than Virtual to Item, which I haven't heard back on my rationale for. As in that comment, if you would really prefer Item, I think I can do that for this half, but for the other one I think Virtual is load-bearing for alternate receivers.

@rustbot ready

@bors
Copy link
Contributor

bors commented Mar 28, 2024

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

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine to approve this after rebase. Anything I mentioned can be done as a follow-up.

let mut s = String::new();
let name = encode_ty_name(tcx, *def_id);
let _ = write!(s, "u{}{}", name.len(), &name);
let parent_args = tcx.mk_args(args.as_coroutine_closure().parent_args());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should do this for closures too; shouldn't change anything, but it means encoding fewer args for closures.

s.push_str(&encode_args(tcx, parent_args, dict, options));
compress(dict, DictKey::Ty(ty, TyQ::None), &mut s);
typeid.push_str(&s);
}

ty::Coroutine(def_id, args, ..) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For coroutines, we should actually also encode args.as_closure().kind_ty() in addition to the parent args. But if you don't want to do this, I could craft a test where this matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't object to doing it, but would prefer it in a follow up to keep things moving.

The other thing to note here is that lacking the kind_ty won't break anything, it will just make our alias sets not-as-good-as-they-could-be, since coroutines of different kinds will be swappable by an attacker.

Copy link
Member

@compiler-errors compiler-errors Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, making them distinguishable would be useful though. Follow-up.

@bors
Copy link
Contributor

bors commented Mar 30, 2024

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 30, 2024
KCFI: Require -C panic=abort

While the KCFI scheme is not incompatible with unwinding, LLVM's `invoke` instruction does not currently support KCFI bundles. While it likely will in the near future, we won't be able to assume that in Rust for a while.

We encountered this problem while [turning on closure support](rust-lang#123106 (comment)).

r? `@workingjubilee`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 30, 2024
KCFI: Require -C panic=abort

While the KCFI scheme is not incompatible with unwinding, LLVM's `invoke` instruction does not currently support KCFI bundles. While it likely will in the near future, we won't be able to assume that in Rust for a while.

We encountered this problem while [turning on closure support](rust-lang#123106 (comment)).

r? ``@workingjubilee``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 30, 2024
Rollup merge of rust-lang#123200 - maurer:kcfi-abort, r=compiler-errors

KCFI: Require -C panic=abort

While the KCFI scheme is not incompatible with unwinding, LLVM's `invoke` instruction does not currently support KCFI bundles. While it likely will in the near future, we won't be able to assume that in Rust for a while.

We encountered this problem while [turning on closure support](rust-lang#123106 (comment)).

r? ``@workingjubilee``
@bors
Copy link
Contributor

bors commented Mar 30, 2024

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

Similar to methods on a trait object, the most common way to indirectly
call a closure or coroutine is through the vtable on the appropriate
trait. This uses the same approach as we use for trait methods, after
backing out the trait arguments from the type.
@compiler-errors
Copy link
Member

yeet

@bors r+

@bors
Copy link
Contributor

bors commented Mar 30, 2024

📌 Commit 8cc9a91 has been approved by compiler-errors

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 30, 2024
@bors
Copy link
Contributor

bors commented Mar 30, 2024

⌛ Testing commit 8cc9a91 with merge 70714e3...

@bors
Copy link
Contributor

bors commented Mar 30, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 70714e3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 30, 2024
@bors bors merged commit 70714e3 into rust-lang:master Mar 30, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 30, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (70714e3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-2.9%, -2.3%] 2
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.0% [0.0%, 0.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Bootstrap: 667.603s -> 667.994s (0.06%)
Artifact size: 315.77 MiB -> 315.79 MiB (0.01%)

@rcvalle
Copy link
Member

rcvalle commented Apr 1, 2024

The compiler crashes with the following code with this PR:

use std::future::Future;
use std::pin::Pin;

async fn async_fn() {}

fn main() {
    let f: fn() -> Pin<Box<dyn Future<Output = ()>>> = || Box::pin(async_fn());
    let _ = async { f().await; };
}

I can take a look at it later if you don't have time to.

@maurer
Copy link
Contributor Author

maurer commented Apr 2, 2024

I was unaware that there is more than one variety of ty::Coroutine. There are in fact:

  • General coroutines (which is what my solution works for right now)
  • Async coroutines (what you've constructed)
  • Generator coroutines
  • Async generator coroutines

Since that value doesn't implement std::ops::Coroutine, generalization fails. I'm writing up a solution now that uses is_general_coroutine as an additional restriction on generalizing through Coroutine. Since async coroutines are supposed to directly implement Future, I'm generalizing them through Future instead.

See #123368 for an explicit solution to this issue, but it doesn't yet support generator coroutines or async generator coroutines, I'll find time to get those working soon.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 2, 2024
… r=compiler-errors

CFI: Support non-general coroutines

Previously, we assumed all `ty::Coroutine` were general coroutines and attempted to generalize them through the `Coroutine` trait. Select appropriate traits for each kind of coroutine.

I have this marked as a draft because it currently only fixes async coroutines, and I think it make sense to try to fix gen/async gen coroutines before this is merged.

If the issue [mentioned](rust-lang#123106 (comment)) in the original PR is actually affecting someone, we can land this as is to remedy it.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 2, 2024
… r=compiler-errors

CFI: Support non-general coroutines

Previously, we assumed all `ty::Coroutine` were general coroutines and attempted to generalize them through the `Coroutine` trait. Select appropriate traits for each kind of coroutine.

I have this marked as a draft because it currently only fixes async coroutines, and I think it make sense to try to fix gen/async gen coroutines before this is merged.

If the issue [mentioned](rust-lang#123106 (comment)) in the original PR is actually affecting someone, we can land this as is to remedy it.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 2, 2024
Rollup merge of rust-lang#123368 - maurer:cfi-non-general-coroutines, r=compiler-errors

CFI: Support non-general coroutines

Previously, we assumed all `ty::Coroutine` were general coroutines and attempted to generalize them through the `Coroutine` trait. Select appropriate traits for each kind of coroutine.

I have this marked as a draft because it currently only fixes async coroutines, and I think it make sense to try to fix gen/async gen coroutines before this is merged.

If the issue [mentioned](rust-lang#123106 (comment)) in the original PR is actually affecting someone, we can land this as is to remedy it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

8 participants