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: Skip non-passed arguments #122456

Merged
merged 1 commit into from
Mar 22, 2024
Merged

CFI: Skip non-passed arguments #122456

merged 1 commit into from
Mar 22, 2024

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Mar 13, 2024

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.

This PR was split off from #121962 as part of fixing the larger vtable compatibility issues.

r? @workingjubilee

@rustbot rustbot 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 Mar 13, 2024
@workingjubilee
Copy link
Member

cc @rcvalle since this affects CFI in general as well.

Can you cite an example of this reliance "in the wild"? For all I know there's already one in the rustc codebase, heh, just curious.

@maurer
Copy link
Contributor Author

maurer commented Mar 13, 2024

The attached test is one such example - it reifies a closure into a function pointer by way of knowing that the closure context is a ZST.

@workingjubilee
Copy link
Member

Huh, I didn't really think of the closure-to-fn-pointer decay that way, I suppose it makes sense. Can you add a note to the test that the ZST-ness of the captured context is part of what's relevant?

// CHECK: ![[TYPE137]] = !{i64 0, !"_ZTSFvu{{[0-9]+}}NtNvC{{[[:print:]]+}}_[[ITANIUMED_FILENAME]]3fn13QuxIu3i32Lu5usize32EES2_S2_E"}
// CHECK: ![[TYPE138]] = !{i64 0, !"_ZTSFvu{{[0-9]+}}NcNtNvC{{[[:print:]]+}}_[[ITANIUMED_FILENAME]]3fn15Quuux15{{[{}][{}]}}constructor{{[}][}]}}Iu6regionS_EE"}
// CHECK: ![[TYPE139]] = !{i64 0, !"_ZTSFvu{{[0-9]+}}NcNtNvC{{[[:print:]]+}}_[[ITANIUMED_FILENAME]]3fn15Quuux15{{[{}][{}]}}constructor{{[}][}]}}Iu6regionS_ES0_E"}
// CHECK: ![[TYPE140]] = !{i64 0, !"_ZTSFvu{{[0-9]+}}NcNtNvC{{[[:print:]]+}}_[[ITANIUMED_FILENAME]]3fn15Quuux15{{[{}][{}]}}constructor{{[}][}]}}Iu6regionS_ES0_S0_E"}
Copy link
Member

Choose a reason for hiding this comment

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

Why are you removing all these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are being removed because their ABI coding becomes equivalent. You're passing in a ZST, so they all collapse.

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. We're losing a lot of granularity there. This seems to be the result of incorrectly removing the ZSTs vs unclosure'ing the Closures.

//@ run-pass

pub fn main() {
let f: &fn() = &((|| ()) as _);
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the closure has parameters?

Copy link
Contributor Author

@maurer maurer Mar 14, 2024

Choose a reason for hiding this comment

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

Then the cast is invalid:

pub fn main() {
    let x: usize = 3;
    let f: &fn() -> usize = &((|| x) as _);
}

generates

error[E0605]: non-primitive cast: `{closure@src/main.rs:3:32: 3:34}` as `fn() -> usize`

That cast is only legal for closures with no captures.

Copy link
Member

Choose a reason for hiding this comment

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

I was referring to parameters, not captures.

Copy link
Member

Choose a reason for hiding this comment

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

Why would that matter, exactly?

Copy link
Member

Choose a reason for hiding this comment

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

I wanted to know how closures with parameters behave with these changes, but I guess considering all what I've seem and pointed out already this doesn't matter.

@rcvalle
Copy link
Member

rcvalle commented Mar 14, 2024

It seems this PR has an incomplete fix for a more general problem of allowing function items, closures, and Fn trait objects to be used interchangeably, which would then become a "complete" solution in your overall solution for KCFI with the addition of shims for it.

As I mentioned in #121962 and to you directly a couple weeks ago, I'm currently working on a fix for this for both CFI and KCFI using type metadata IDs/encoding only (see #116404). which follows the same design of the rest of the CFI implementation by transforming/coalescing types we want to be semantically equivalent into a single entity and passing them for encoding, so the shims aren't necessary in this case even for KCFI.

I'll resolve the merge conflicts and comments in #121962, confirm it fixes this issue, and try to get it merged next week if you don't mind waiting. If that doesn't fix this issue, we can have this merged (pending review).

@workingjubilee
Copy link
Member

workingjubilee commented Mar 14, 2024

@rcvalle Does C++ actually have a coherent ABI for zero-sized arguments? If so, why is that a "more general solution" to fixing an incoherency in argument encoding? If C++ cannot represent it, why should it be encoded? If we do, we are getting awfully close to, assuming we haven't already passed, abandoning any pretense of actually using the Itanium mangling/C++ logic, and are just making up stuff.

@rcvalle
Copy link
Member

rcvalle commented Mar 14, 2024

It's a more general solution for allowing function items, closures, and Fn trait objects to be used interchangeably that doesn't require shims. The ZSTs are gone in #116404 too when unclosure'ing the closures (see the else if is_closure_call in transform_fnabi there).

@workingjubilee
Copy link
Member

@rcvalle If both erase ZSTs, then why not just land this first?

@rcvalle
Copy link
Member

rcvalle commented Mar 14, 2024

Because this actually doesn't solve any problem completely or correctly. The problem isn't erasing ZSTs, the problem is properly unclosure'ing Closures and unwrapping dynamic Fn trait objects and encoding them correctly. This just partially fix the problem by erasing the ZSts from the Closures (instead of properly unclosure'ing them) for later "completing" the fix with shims/trampolines so they can be used interchangeably (also instead of encoding them correctly), which are not necessary.

I just updated #116404 resolving the merge conflicts, fixing and doing a few things pointed out by @compiler-errors, and also added the test from this PR there to demonstrate that it not only fix this case, but more generally the problem I described before (see tests/codegen/sanitizer/cfi-emit-type-metadata-id-itanium-cxx-abi-closures.rs which illustrates it there).

There are still a couple things left to do there that I'll take a look at next week.

@maurer
Copy link
Contributor Author

maurer commented Mar 14, 2024

Because this actually doesn't solve any problem completely or correctly.

I have the complete solution in my full patch stack, but I was just asked to start splitting off smaller pieces which point-fixed some issues. If you want a full solution, look at my primary PR.

The problem isn't erasing ZSTs, the problem is properly unclosure'ing Closures and unwrapping dynamic Fn trait objects and encoding them correctly

This is a separate problem from the ZSTs, and is addressed via the rewrite to ReifyShim in the cfi_shim function in my primary patch.

@rcvalle
Copy link
Member

rcvalle commented Mar 14, 2024

It's the exact same problem, but you're solving it by adding shims instead. I mentioned this to you a couple weeks ago when we discussed your solution and I asked you to take a look and rebase your changes on top of it, eliminating these unnecessary shims, which you didn't.

@workingjubilee
Copy link
Member

@rcvalle Setting aside the shim discussion, I am currently not persuaded that not erasing ZSTs is correct. All of the codegen backends erase ZSTs from fn arguments while emitting code, so why should the typechecking here be different? The languages we're concerned about checking indirect calls from/into don't even have ZSTs as a concept.

@rcvalle
Copy link
Member

rcvalle commented Mar 14, 2024

I'm not saying that not erasing ZSTs is correct, I'm saying that just doing it is (and also how it's done manually vs unclosure'ing). Doing it is part of a larger issue that I already explained and repeated here many times: allowing function items, closures, and Fn trait objects to be used interchangeably (see tests/codegen/sanitizer/cfi-emit-type-metadata-id-itanium-cxx-abi-closures.rs which illustrates it).

This PR doesn't solve it at all without adding unnecessary shims later. Just try tests/codegen/sanitizer/cfi-emit-type-metadata-id-itanium-cxx-abi-closures.rs with this PR without the rest of the changes that add the shims and you'll see).

@workingjubilee
Copy link
Member

@rcvalle As this is an incremental improvement only, split as I requested, I don't see why @maurer would object to reverting these changes if a more complete fix was offered later, and I certainly wouldn't. I would hope that is acceptable to you, also?

I realize that you would prefer shipping a more complete fix, but this entire work has been at least partly about feeling out better solutions and sometimes being surprised by what turns things take.

@rcvalle
Copy link
Member

rcvalle commented Mar 14, 2024

What I'm trying to avoid is reverting it a couple of weeks from now along with #116404, and @maurer having to rebase their proposed changes on top of it anyway. What merging this now accomplishes?

@workingjubilee
Copy link
Member

...I mean, for me, my git-fu is pretty strong, so I'm just kinda assuming "rebase changes on top" isn't actually a big deal.

@workingjubilee
Copy link
Member

...I mean, for me, my git-fu is pretty strong,

Unless it's a subtree, tbh.

@bors
Copy link
Contributor

bors commented Mar 19, 2024

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

@workingjubilee
Copy link
Member

I currently do not believe that it is possible for either form of CFI to realistically work without performing some ZST erasure. If it needs to be narrowed in the future from this, I feel that can be done later.

@maurer Can you rebase this? Thanks!

@workingjubilee
Copy link
Member

@maurer Also, when you do, please add a comment explaining that this ZST erasure is only sensical under PassMode::Ignore, and that this erasure is an implementation detail of how MIR is currently interpreted by rustc that is subject to change in the future.

@rcvalle
Copy link
Member

rcvalle commented Mar 20, 2024

If this is merged, it's going to decrease CFI granularity a lot. Basically, all groups of functions distinguished by ZSTs in Rust-compiled code will be gone.

@rustbot rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label Mar 21, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 21, 2024

Some changes occurred in compiler/rustc_symbol_mangling/src/typeid

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

Some changes occurred in tests/codegen/sanitizer

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

Some changes occurred in tests/ui/sanitizer

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 21, 2024
@workingjubilee
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2024

📌 Commit 331d856 has been approved by workingjubilee

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 21, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2024
CFI: Skip non-passed arguments

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.

This PR was split off from rust-lang#121962 as part of fixing the larger vtable compatibility issues.

r? `@workingjubilee`
@bors
Copy link
Contributor

bors commented Mar 21, 2024

⌛ Testing commit 331d856 with merge 83dadc0...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 21, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 21, 2024
@maurer
Copy link
Contributor Author

maurer commented Mar 21, 2024

The failure appears to be that OSX can't build CFI binaries. This patch doesn't adjust codegen, only labels, so if building is failing, it's an existing bug. It's one we should solve, but not a flaw in the patch. I'm going to adjust the test to be //@ only-linux. This is happening because it is the first test we have that actually builds a fully linked binary and executes it.

@workingjubilee
Copy link
Member

workingjubilee commented Mar 21, 2024

@maurer Please capture the PLT/reloc issue as a FIXME on the test.

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.
@workingjubilee
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2024

📌 Commit f2f0d25 has been approved by workingjubilee

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

bors commented Mar 22, 2024

⌛ Testing commit f2f0d25 with merge 7762adc...

@bors
Copy link
Contributor

bors commented Mar 22, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 7762adc to master...

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

Finished benchmarking commit (7762adc): 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)

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

Cycles

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.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 667.777s -> 669.214s (0.22%)
Artifact size: 315.07 MiB -> 314.92 MiB (-0.05%)

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.

7 participants