CFI: Abstract Closures and Coroutines

[maurer]

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]

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

[maurer]

r? @compiler-errors

[rustbot]

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]

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]

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]

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]

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

[compiler-errors]

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?

[maurer]

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.

[maurer]

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

[maurer]

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]

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

[compiler-errors]

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

[compiler-errors]

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

[compiler-errors]

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.

[maurer]

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.

[compiler-errors]

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

[bors]

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

[bors]

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

[compiler-errors]

yeet

@bors r+

[bors]

📌 Commit 8cc9a91 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

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

[bors]

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

[rust-timer]

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]

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]

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.