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.

Sign up for GitHub

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

Jump to bottom

CFI: Append debug location to CFI blocks #132702

Merged
merged 1 commit into from
Nov 13, 2024
Merged

CFI: Append debug location to CFI blocks #132702

merged 1 commit into from
Nov 13, 2024

Conversation

1c3t3a
Copy link
Contributor

@1c3t3a 1c3t3a commented Nov 6, 2024 •
edited
Loading

Currently we're not appending debug locations to the inserted CFI blocks. This shows up in #132615 and #100783. This change fixes that by passing down the debug location to the CFI type-test generation and appending it to the blocks.

Credits also belong to @jakos-sec who worked with me on this.

@rustbot
Copy link
Collaborator

rustbot commented Nov 6, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
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 Nov 6, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 6, 2024

Some changes occurred in tests/codegen/sanitizer

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

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@1c3t3a
Copy link
Contributor Author

1c3t3a commented Nov 6, 2024 •
edited
Loading

r? @rcvalle

@rustbot rustbot assigned rcvalle and unassigned BoxyUwU Nov 6, 2024
@rcvalle
Copy link
Member

rcvalle commented Nov 6, 2024

Thank you for your time and for working on this, @1c3t3a ! Much appreciated.

Alternatively, this could be implemented by passing back/returning the blocks created by CFI to rustc_codegen_ssa if any, but I'm OK with this implementation. I think invoke and call need some refactoring (and possibly being consolidated into one) anyway, and from past conversations with @bjorn3 seemed to agree, and might be interested in providing feedback here. Otherwise; LGTM (FYI @cuviper).

@1c3t3a
Copy link
Contributor Author

1c3t3a commented Nov 7, 2024 •
edited
Loading

but I'm OK with this implementation. I think invoke and call need some refactoring (and possibly being consolidated into one) anyway, and from past conversations with @bjorn3 seemed to agree, and might be interested in providing feedback here. Otherwise; LGTM (FYI @cuviper).

Another alternative would be to get the current debug information before appending the CFI blocks, remember it and append it to all inserted blocks. This would need to call LLVMGetCurrentDebugLocation2, which is currently not used by rustc. It would have the benefit that it doesn't need to change rustc_codegen_ssa's builder interface and it would keep all CFI logic inside rustc_codegen_llvm.

@rcvalle
Copy link
Member

rcvalle commented Nov 7, 2024

I think that would be preferred if you don't mind. You can add LLVMGetCurrentDebugLocation2 to rustc_llvm similarly to how LLVMRustIsNonGVFunctionPointerTy was added (also for CFI).

@1c3t3a 1c3t3a force-pushed the issue-132615 branch 2 times, most recently from 9ea79fe to b5e4f5a Compare November 8, 2024 08:55
@1c3t3a
Copy link
Contributor Author

1c3t3a commented Nov 8, 2024

I think that would be preferred if you don't mind. You can add LLVMGetCurrentDebugLocation2 to rustc_llvm similarly to how LLVMRustIsNonGVFunctionPointerTy was added (also for CFI).

Oke great! I updated the implementation accordingly :)

@rcvalle
Copy link
Member

rcvalle commented Nov 8, 2024

Thank you very much, @1c3t3a! It looks great! I've just two comments I left that after resolved it LGTM (FYI, @cuviper).

@rcvalle
Copy link
Member

rcvalle commented Nov 12, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Nov 12, 2024

📌 Commit c210225 has been approved by rcvalle

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 Nov 12, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 12, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 13, 2024
@bors
Auto merge of rust-lang#132972 - matthiaskrgr:rollup-456osr7, r=matth…
242f20d 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#132702 (CFI: Append debug location to CFI blocks)
 - rust-lang#132851 (Update the doc comment of `ASCII_CASE_MASK`)
 - rust-lang#132948 (stabilize const_unicode_case_lookup)
 - rust-lang#132950 (Use GNU ld on m68k-unknown-linux-gnu)
 - rust-lang#132962 (triagebot: add codegen reviewers)
 - rust-lang#132966 (stabilize const_option_ext)
 - rust-lang#132970 (Add tracking issue number to unsigned_nonzero_div_ceil feature)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bd79fe7 into rust-lang:master Nov 13, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 13, 2024
@rust-timer
Unrolled build for rust-lang#132702
22347b8 
Rollup merge of rust-lang#132702 - 1c3t3a:issue-132615, r=rcvalle

CFI: Append debug location to CFI blocks

Currently we're not appending debug locations to the inserted CFI blocks. This shows up in rust-lang#132615 and rust-lang#100783. This change fixes that by passing down the debug location to the CFI type-test generation and appending it to the blocks.

Credits also belong to `@jakos-sec` who worked with me on this.
This was referenced Nov 15, 2024
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcvalle rcvalle rcvalle left review comments

Assignees

@rcvalle rcvalle

Labels
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
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.
5 participants
@1c3t3a @rustbot @rcvalle @bors @BoxyUwU