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: Support different patchable-function-prefix values #1744

Open
samitolvanen opened this issue Oct 19, 2022 · 4 comments
Open

CFI: Support different patchable-function-prefix values #1744

samitolvanen opened this issue Oct 19, 2022 · 4 comments
Labels
[FEATURE] CFI Related to building the kernel with Clang Control Flow Integrity feature-request Not a bug per-se

Comments

@samitolvanen
Copy link
Member

KCFI currently emits the type hash before patchable-function-prefix nops, and assumes that all functions have the same number of prefix nops. Specifically, indirect call targets must have the same number of prefix nops as the callers. However, Mark Rutland has plans to use prefix nops in a way that breaks this assumption.

Based on an earlier IRC discussion, the preferred way of solving this would be to move the hash after the prefix nops, which would mean callers no longer have to adjust the read offset. This would simplify things, but emitting the hash between the nops and the function entry would break anyone planning to execute the prefix nops and fall through to the actual function, unless they explicitly jump over the hash.

Such use cases don't appear to exist in the kernel right now, but unless we make this architecture specific, moving the hash does mean some changes are needed to the retbleed/FineIBT patching on x86. We also need to figure out if the __cfi_function symbol still needs to be emitted before the nops.

Tests for the current KCFI + patchable-function-prefix behavior are here:

https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AArch64/kcfi-patchable-function-prefix.ll
https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/kcfi-patchable-function-prefix.ll

Another option would be to add a command line flag for specifying the hash offset, and ensuring that we always emit the hash at the specified offset no matter how many prefix nops are requested.

cc @kees @lvwr @nickdesaulniers @pcc @MaskRay

@samitolvanen samitolvanen added feature-request Not a bug per-se [FEATURE] CFI Related to building the kernel with Clang Control Flow Integrity labels Oct 19, 2022
@samitolvanen
Copy link
Member Author

@MaskRay
Copy link
Member

MaskRay commented Oct 22, 2022

Since the kcfi code expects the hash to appear in a specific location so that an instrumented indirect jump can reliably obtain the hash. For a translation unit -fpatchable-function-entry=N,M may be specified or not, and we want both to work. Therefore, I agree that a consistent hash location will help. This argument favors placing M nops before the hash. The downside is a restriction on how the M nops can be used. Previously if M>0, the runtime code needs to check whether a BTI exists to locate the N-M after-function-entry NOPs. If the hash appears after the M nops, the runtime code needs to additionally knows whether the hash exists. My question is how to reliably detect this.

If there is motivation using M>0, I'd like to know the concrete code sequence for -fpatchable-function-entry=N,M and how the runtime code detects the NOPs with optional hash and optional BTI.

@samitolvanen
Copy link
Member Author

@MaskRay it looks like the latest suggestion was to add a section argument to the patchable_function_entry attribute. Any thoughts about that?

https://lore.kernel.org/lkml/Y1QEzk%[email protected]/

@samitolvanen
Copy link
Member Author

A PR that implements the section argument: llvm/llvm-project#131230

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[FEATURE] CFI Related to building the kernel with Clang Control Flow Integrity feature-request Not a bug per-se
Projects
None yet
Development

No branches or pull requests

2 participants