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: Store type erasure witness for Argument #115954

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Sep 19, 2023

This is a WIP that won't work until @rcvalle has fixed address-taken trait functions #115953 but I understand he has a fix for that on the way.

Once that's out of the way, this addresses #115199 point 2.

CFI/KCFI work by enforcing that indirect function calls are always called at the type they were defined at. The type erasure in Argument works by casting the reference to the value to be formatted to an Opaque and also casting the function to format it to take an Opaque reference.

While this is ABI safe, which is why we get away with it normally, it does transform the type that the function is called at. This means that at the call-site, CFI expects the type of the function to be fn(&Opaque, even though it is really fn(&T, for some particular T.

This patch avoids this by adding cast_stub, a witness to the type erasure that will cast the &Opaque and fn(&Opaque back to their original types before invoking the function.

This change is guarded by the enablement of CFI as it will require an additional pointer-sized value per Argument, and an additional jump during formatting, and we'd prefer not to pay that if we don't need the types to be correct at the indirect call invocation.

CFI/KCFI work by enforcing that indirect function calls are always
called at the type they were defined at. The type erasure in Argument
works by casting the reference to the value to be formatted to an Opaque
and also casting the function to format it to take an Opaque reference.

While this is *ABI* safe, which is why we get away with it normally, it
does transform the type that the function is called at. This means that
at the call-site, CFI expects the type of the function to be
`fn(&Opaque, ` even though it is really `fn(&T, ` for some particular
`T`.

This patch avoids this by adding `cast_stub`, a witness to the type
erasure that will cast the `&Opaque` and `fn(&Opaque` back to their
original types before invoking the function.

This change is guarded by the enablement of CFI as it will require an
additional pointer-sized value per `Argument`, and an additional jump
during formatting, and we'd prefer not to pay that if we don't need the
types to be correct at the indirect call invocation.
@rustbot
Copy link
Collaborator

rustbot commented Sep 19, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 19, 2023
@rcvalle rcvalle added the PG-exploit-mitigations Project group: Exploit mitigations label Sep 19, 2023
@Dylan-DPC Dylan-DPC added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2023
@bors
Copy link
Contributor

bors commented Apr 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PG-exploit-mitigations Project group: Exploit mitigations S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants