CFI: Fix types that implement Fn, FnMut, or FnOnce

[rcvalle]

When looking for instances which could either be dynamically called through a vtable or through a concrete trait method, we missed FnPtrShim, instead only looking at Item and closure-likes. Fixes #144641.

cc @1c3t3a @Jakob-Koschel

[rustbot]

r? @SparrowLii

rustbot has assigned @SparrowLii.
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

[compiler-errors]

I'd like to see more explanation for the fix in this PR. Specifically, the issue and the PR don't actually describe what kind of CFI violation was being encountered here before, or how it's being fixed by this erasure?

The comment in #144641 (comment) mentions:

The only special handling in the code is for closures, coroutines, and coroutine closures, but not for arbitrary types that implement these traits.

Does this make the code that is special-cased for closure-like types redundant?

[compiler-errors]

@maurer wrote a lot of this erasure logic, I'd be interested if he has a take on this approach as well

[maurer]

My first reaction here is that I think this is a bug in implemented_method rather than something that should be fixed via an additional clause in transform_instance - implemented_method should detect the call_mut implementation as an implemented method, and so the first non-closure clause should be triggering. I'll take a more in depth look in an hour or two when I get in to work.

[maurer]

Specifically, d5bf25c seems to address this more simply - basically, it looks like I missed FnPtrShim(def_id, _). I'm still taking a look over the other instance kinds to make sure that not casing out on only Item and FnPtrShim won't accidentally apply somewhere it shouldn't, but this seems to fix it a bit more cleanly - the problem was an unaccounted-for instance kind, not an unaccounted-for trait.

Update: It looks like the adjustment I'm proposing would also cause:

• Virtual and VTableShim to get double-processed, because we've already dealt with them. The generalization is mostly a no-op, except that the InstanceKind for VTableShim will become Virtual, which is perhaps misleading but should have no effect and Virtual instances will get their method index zeroed out.
• ReifyShim would get erased into Virtual - this would be a problem for KCFI, except this code is guarded by USE_CONCRETE_SELF, which gets forcibly enabled for ReifyReason::FnPtr.
• ClosureOnceShim and ConstructCoroutineInClosureShim should trip this, and occur before the check if something is closure-like. I wasn't able to easily detect any differences in behavior here, but I didn't do deep tracing. I think handling these via the closure-like code vs via the default impl code normalizes them the same way. We still need the closure normalizing code because true closures can't be normalized this way.
• FutureDropPollShim, CloneShim, FnPtrAddrShim, and AsyncDropGlueCtorShim would all match initially, but then be discarded because the traits they implement are not object safe.

This looks OK to me, and has the added benefit of automatically picking up new shim types when added.

If we wanted to be more conservative, we could instead do 0026b9c which fixes the tests in question with a minimal change - it only re-normalizes FnPtrShim, and would require any future shims we want normalized to be explicitly added to the default_or_shim checker.

[rcvalle]

@maurer Thank you for your time looking at it! Much appreciated. I'm wondering whether we should change approach in this code and cover each case explicitly (like we do for encoding) so we could express our intent clearly and know when something is handled (or not) and the side effects of it (also clearly).

[rcvalle]

@maurer If you put a commit message on 0026b9c, I could cherry pick it into this PR, or if you have time to, feel free to send a PR for it. Thanks a lot! Much appreciated.

[maurer]

I added a commit message at https://github.com/maurer/rust/tree/cfi-fix - I hadn't bothered because I just expected you to squash it onto your commit if you agreed with the direction.

Re: being more explicit with casing out, I think maybe the way to do that (which should definitely be a future PR if at all) would be:

1. Split out InstanceKinds into
   a. Always vtable (or Virtual) (these should be unconditionally rewritten)
   b. Sometimes vtable (these should be rewritten only when the concrete flag isn't passed)
   c. Passthrough
2. Check to see if it would be possible to simplify the flow of our existing code based on that - right now, everything in the "always vtable" section still possibly falls through and gets processed by the "sometimes vtable" section when concrete isn't set, but I don't think that's load bearing, and probably just confuses things
3. Create an explicit enum that looks something like

enum RewriteKind { // Always rewrite Drop, Virtual(DefId), VTableShim(DefId), // Sometimes rewrite Default(DefId), Impl(DefId), Shim(DefId), Closure(InstanceKind), // Never rewrite Passthrough(InstanceKind), } 

4. We do the exhaustive careful conversion from InstanceKind to RewriteKind, and then have a separate method that takes the RewriteKind and actually does the (possible) rewrite.

I don't know how much maintenance this saves us, mostly depends on how often we're sprinkling new InstanceKinds in or whether we've missed any other InstanceKinds

[rcvalle]

@maurer Just picked it into this PR. Thank you! (I had to fix some compilation errors, but I believe the PR is going to be squashed before being merged anyway so it should be fine.)

I'll take a look at covering each case explicitly and your suggestion, and send a separate PR and CC you when I've something. Thanks again!

[rcvalle]

@rustbot ready

[rcvalle]

r? @compiler-errors

[rustbot]

compiler-errors is not on the review rotation at the moment.
They may take a while to respond.

[compiler-errors]

r? compiler

[rcvalle]

so if I understand this correctly, the issue here is that CFI doesn't allow calling methods which are not part of the the allowed list of typeids?

So anything that intentionally enables dynamic calls, i.e. trait objects, needs to be converted to get a single shared typeid.

transform_instance does seem questionable to me 🤔 the resulting instance must never be used except to get the fn_abi_of_instance 🤔

It checks that the function pointer to be called at a call site has the expected/same type (i.e., type id) its function definition, and this is a helper to perform type erasure for when it's expected at call sites.

I feel like we do want an exhaustive match on the InstanceKind here. We have to handle async drop shims the same way as normal drops, don't we?

Yes, agreed. I suggested the same above. I'll work on that on a subsequent PR.

r=me on the change in the PR itself, it seems innocent enough

Thank you very much for your time on this! Much appreciated.

[rustbot]

⚠️ Warning ⚠️

• This PR is based on an upstream commit that is older than 28 days.

  It's recommended to update your branch according to the rustc-dev-guide.

[rcvalle]

@bors r=lcnr

[bors]

📌 Commit ea08a7d has been approved by lcnr

It is now in the queue for this repository.

[matthiaskrgr]

@bors r-
#147765 (comment)

[rcvalle]

@bors r=lcnr

[bors]

📌 Commit 3821bac has been approved by lcnr

It is now in the queue for this repository.

[samueltardieu]

#147807 (comment)

@bors r-