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

Please respect nakedness of functions for CFG/CFI/IBT/BTI/etc. #56369

Open
workingjubilee opened this issue Jul 4, 2022 · 6 comments
Open

Please respect nakedness of functions for CFG/CFI/IBT/BTI/etc. #56369

workingjubilee opened this issue Jul 4, 2022 · 6 comments
Labels
backend:AArch64 backend:X86 bug Indicates an unexpected problem or unintended behavior

Comments

@workingjubilee
Copy link
Contributor

workingjubilee commented Jul 4, 2022

Hello, this is a follow up from rust-lang/rust#98768

Currently, rustc will, if asked to use options like AArch64's BTI or Intel's CET, emit module-level attributes, following the overall "module, then function override" logic that has been used for these suites of security features. However, we also have people wanting to combine these with naked functions. The definition we have adopted for naked functions is pretty strict on having no prologue at all, and we do emit the naked function attribute, which the LangRef asserts is supposed to strip prologues and epilogues. The result looks kiiinda like so in LLVM IR (tried to minimize cruft a little):

target triple = "aarch64-unknown-linux-gnu"

define void @_hlt() unnamed_addr #0 {
  call void asm sideeffect alignstack "hlt #1", "~{cc},~{memory}"()
  unreachable
}

attributes #0 = { naked nocf_check noinline noreturn "target-cpu"="generic" }

!llvm.module.flags = !{!0, !1, !2, !3, !4, !5}

!0 = !{i32 7, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}
!2 = !{i32 1, !"branch-target-enforcement", i32 1}
!3 = !{i32 1, !"sign-return-address", i32 0}
!4 = !{i32 1, !"sign-return-address-all", i32 0}
!5 = !{i32 1, !"sign-return-address-with-bkey", i32 0}

However, the generated assembly looks like:

_hlt:                                   // @_hlt
        hint    #34
        hlt     #0x1

That is very much a prologue right there!

Now, I think it's reasonable to ask whether this is "intended behavior" for naked functions... absolutely no prologue is intended in Rust, but naked functions are fairly underspecified anyways. One could possibly even question what the definition of a "prologue" is anyways. But the nocf_check attribute is there because while we have a similar problem with the x86-64 equivalent for Intel CET (AKA IBT) coming with endbr64 prologues, nocf_check strips them away. This is pretty convenient, and doing a bit of extra legwork to make sure they're gone is fine. However, AArch64's BTI doesn't respect this and instead requires "branch-target-enforcement"=false be set on the function to override it.

This seems like a logical bug. Naked functions should probably not have prologues, even these "security prologues". But if they do, then either nocf_check should strip all such branch control prologues or none of them. It would be nice if either or both consistently removed these instructions, so as to allow compilers to toggle them off in an architecture-neutral manner, rather than having to emit a distinct negation for each architecture.

@dtcxzyw dtcxzyw added question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead! backend:AArch64 backend:X86 and removed new issue labels Oct 1, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 1, 2023

@llvm/issue-subscribers-backend-aarch64

Hello, this is a follow up from https://github.com/rust-lang/rust/issues/98768

Currently, rustc will, if asked to use options like AArch64's BTI or Intel's CET, emit module-level attributes, following the overall "module, then function override" logic that has been used for these suites of security features. However, we also have people wanting to combine these with naked functions. The definition we have adopted for naked functions is pretty strict on having no prologue at all, and we do emit the naked function attribute, which the LangRef asserts is supposed to strip prologues and epilogues. The result looks kiiinda like so in LLVM IR (tried to minimize cruft a little):

target triple = "aarch64-unknown-linux-gnu"

define void @<!-- -->_hlt() unnamed_addr #<!-- -->0 {
  call void asm sideeffect alignstack "hlt #<!-- -->1", "~{cc},~{memory}"()
  unreachable
}

attributes #<!-- -->0 = { naked nocf_check noinline noreturn "target-cpu"="generic" }

!llvm.module.flags = !{!0, !1, !2, !3, !4, !5}

!0 = !{i32 7, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}
!2 = !{i32 1, !"branch-target-enforcement", i32 1}
!3 = !{i32 1, !"sign-return-address", i32 0}
!4 = !{i32 1, !"sign-return-address-all", i32 0}
!5 = !{i32 1, !"sign-return-address-with-bkey", i32 0}

However, the generated assembly looks like:

_hlt:                                   // @<!-- -->_hlt
        hint    #<!-- -->34
        hlt     #<!-- -->0x1

That is very much a prologue right there!

Now, I think it's reasonable to ask whether this is "intended behavior" for naked functions... absolutely no prologue is intended in Rust, but naked functions are fairly underspecified anyways. One could possibly even question what the definition of a "prologue" is anyways. But the nocf_check attribute is there because while we have a similar problem with the x86-64 equivalent for Intel CET (AKA IBT) coming with endbr64 prologues, nocf_check strips them away. This is pretty convenient, and doing a bit of extra legwork to make sure they're gone is fine. However, AArch64's BTI doesn't respect this and instead requires "branch-target-enforcement"=false be set on the function to override it.

This seems like a logical bug. Naked functions should probably not have prologues, even these "security prologues". But if they do, then either nocf_check should strip all such branch control prologues or none of them. It would be nice if either or both consistently removed these instructions, so as to allow compilers to toggle them off in an architecture-neutral manner, rather than having to emit a distinct negation for each architecture.

@llvmbot
Copy link
Member

llvmbot commented Oct 1, 2023

@llvm/issue-subscribers-backend-x86

Hello, this is a follow up from https://github.com/rust-lang/rust/issues/98768

Currently, rustc will, if asked to use options like AArch64's BTI or Intel's CET, emit module-level attributes, following the overall "module, then function override" logic that has been used for these suites of security features. However, we also have people wanting to combine these with naked functions. The definition we have adopted for naked functions is pretty strict on having no prologue at all, and we do emit the naked function attribute, which the LangRef asserts is supposed to strip prologues and epilogues. The result looks kiiinda like so in LLVM IR (tried to minimize cruft a little):

target triple = "aarch64-unknown-linux-gnu"

define void @<!-- -->_hlt() unnamed_addr #<!-- -->0 {
  call void asm sideeffect alignstack "hlt #<!-- -->1", "~{cc},~{memory}"()
  unreachable
}

attributes #<!-- -->0 = { naked nocf_check noinline noreturn "target-cpu"="generic" }

!llvm.module.flags = !{!0, !1, !2, !3, !4, !5}

!0 = !{i32 7, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}
!2 = !{i32 1, !"branch-target-enforcement", i32 1}
!3 = !{i32 1, !"sign-return-address", i32 0}
!4 = !{i32 1, !"sign-return-address-all", i32 0}
!5 = !{i32 1, !"sign-return-address-with-bkey", i32 0}

However, the generated assembly looks like:

_hlt:                                   // @<!-- -->_hlt
        hint    #<!-- -->34
        hlt     #<!-- -->0x1

That is very much a prologue right there!

Now, I think it's reasonable to ask whether this is "intended behavior" for naked functions... absolutely no prologue is intended in Rust, but naked functions are fairly underspecified anyways. One could possibly even question what the definition of a "prologue" is anyways. But the nocf_check attribute is there because while we have a similar problem with the x86-64 equivalent for Intel CET (AKA IBT) coming with endbr64 prologues, nocf_check strips them away. This is pretty convenient, and doing a bit of extra legwork to make sure they're gone is fine. However, AArch64's BTI doesn't respect this and instead requires "branch-target-enforcement"=false be set on the function to override it.

This seems like a logical bug. Naked functions should probably not have prologues, even these "security prologues". But if they do, then either nocf_check should strip all such branch control prologues or none of them. It would be nice if either or both consistently removed these instructions, so as to allow compilers to toggle them off in an architecture-neutral manner, rather than having to emit a distinct negation for each architecture.

@workingjubilee
Copy link
Contributor Author

This is either a bug report or an enhancement request. The only sense in which it is a "question" is which one. All uses of "ask" or "question" in my original statement are rhetorical, because I am attempting to acknowledge there are multiple possible valid implementations that fix/improve this situation, like a good compiler engineer would, instead of simply brashly demanding a particular fix.

@aemerson
Copy link
Contributor

aemerson commented Oct 2, 2023

@ostannard for BTI.

@ostannard
Copy link
Collaborator

GCC's documentation on this is vague, and clang's is non-existent, but I've always thought of "naked" as meaning "no instructions should be emitted, other than the inline assembly". Maybe an exception could be made for things like BTI, which only add NOPs to the start of a function, but GCC documents this as not emitting a prologue, and I'd certainly consider BTI to be part of the prologue. Therefore, i think we should consider this to be a bug.

@ostannard ostannard added bug Indicates an unexpected problem or unintended behavior and removed question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead! labels Oct 3, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 3, 2023

@llvm/issue-subscribers-bug

Hello, this is a follow up from https://github.com/rust-lang/rust/issues/98768

Currently, rustc will, if asked to use options like AArch64's BTI or Intel's CET, emit module-level attributes, following the overall "module, then function override" logic that has been used for these suites of security features. However, we also have people wanting to combine these with naked functions. The definition we have adopted for naked functions is pretty strict on having no prologue at all, and we do emit the naked function attribute, which the LangRef asserts is supposed to strip prologues and epilogues. The result looks kiiinda like so in LLVM IR (tried to minimize cruft a little):

target triple = "aarch64-unknown-linux-gnu"

define void @<!-- -->_hlt() unnamed_addr #<!-- -->0 {
  call void asm sideeffect alignstack "hlt #<!-- -->1", "~{cc},~{memory}"()
  unreachable
}

attributes #<!-- -->0 = { naked nocf_check noinline noreturn "target-cpu"="generic" }

!llvm.module.flags = !{!0, !1, !2, !3, !4, !5}

!0 = !{i32 7, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}
!2 = !{i32 1, !"branch-target-enforcement", i32 1}
!3 = !{i32 1, !"sign-return-address", i32 0}
!4 = !{i32 1, !"sign-return-address-all", i32 0}
!5 = !{i32 1, !"sign-return-address-with-bkey", i32 0}

However, the generated assembly looks like:

_hlt:                                   // @<!-- -->_hlt
        hint    #<!-- -->34
        hlt     #<!-- -->0x1

That is very much a prologue right there!

Now, I think it's reasonable to ask whether this is "intended behavior" for naked functions... absolutely no prologue is intended in Rust, but naked functions are fairly underspecified anyways. One could possibly even question what the definition of a "prologue" is anyways. But the nocf_check attribute is there because while we have a similar problem with the x86-64 equivalent for Intel CET (AKA IBT) coming with endbr64 prologues, nocf_check strips them away. This is pretty convenient, and doing a bit of extra legwork to make sure they're gone is fine. However, AArch64's BTI doesn't respect this and instead requires "branch-target-enforcement"=false be set on the function to override it.

This seems like a logical bug. Naked functions should probably not have prologues, even these "security prologues". But if they do, then either nocf_check should strip all such branch control prologues or none of them. It would be nice if either or both consistently removed these instructions, so as to allow compilers to toggle them off in an architecture-neutral manner, rather than having to emit a distinct negation for each architecture.

@ostannard ostannard removed their assignment Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 backend:X86 bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

5 participants