arm64: Supporting DYNAMIC_FTRACE_WITH_CALL_OPS with CLANG_CFI

I recently worked on allowing BPF programs to work properly on CLANG_CFI enabled kernels [1]. While doing this I found that fentry programs are failing to attach because DYNAMIC_FTRACE_WITH_CALL_OPS doesn't work with CLANG_CFI.

Mark told me that the problem is that clang CFI places the type hash immediately before any pre-function NOPs, and so where some functions have pre-function NOPs and others do not, the type hashes are not at a consistent offset (and effectively the functions have different ABIs and cannot call one another)

I tried enabling both Clang CFI and -fpatchable-function-entry=4,2 to see the behaviour and where this could fail. Here is an example:

This is the disassembly of jump_label_cmp() that has two pre-function nops and the CFI hash before them. So, the hash is at (addr - 12).

ffff80008033e9b0:       16c516ce        [kCFI hash for 'static int jump_label_cmp(const void *a, const void *b)']
ffff80008033e9b4:       d503201f        nop
ffff80008033e9b8:       d503201f        nop
ffff80008033e9bc <jump_label_cmp>:
ffff80008033e9bc:       d503245f        bti     c
ffff80008033e9c0:       d503201f        nop
ffff80008033e9c4:       d503201f        nop

[.....]

The following is the disassembly of the sort_r() function that makes an indirect call to jump_label_cmp() but loads the CFI hash from (addr - 4) rather than
(addr - 12). So, it is loading the nop instruction and not the hash.

ffff80008084e19c <sort_r>:
[.....]

0xffff80008084e454 <+696>:   ldur    w16, [x8, #-4] (#-4 here should be #-12)
0xffff80008084e458 <+700>:   movk    w17, #0x16ce
0xffff80008084e45c <+704>:   movk    w17, #0x16c5, lsl #16
0xffff80008084e460 <+708>:   cmp     w16, w17
0xffff80008084e464 <+712>:   b.eq    0xffff80008084e46c <sort_r+720>  // b.none
0xffff80008084e468 <+716>:   brk     #0x8228
0xffff80008084e46c <+720>:   blr     x8

This would cause a cfi exception.

As I haven't spent more time trying to understand this, I am not aware how the compiler emits 2 nops before some functions and none for others.

I would propose the following changes to the compiler that could fix this issue:

1. The kCFI hash should always be generated at func_addr - 4, this would make the calling code consistent.

2. The two(n) nops should be generated before the kCFI hash. We would modify the ftrace code to look for these nops at (fun_addr - 12) and (func_addr - 8) when CFI is enabled, and (func_addr - 8), (func_addr -
4) when CFI is disabled.

The generated code could then look like:

ffff80008033e9b0:       d503201f        nop
ffff80008033e9b4:       d503201f        nop
ffff80008033e9b8:       16c516ce        kCFI hash
ffff80008033e9bc <jump_label_cmp>:
ffff80008033e9bc:       d503245f        bti     c
ffff80008033e9c0:       d503201f        nop
ffff80008033e9c4:       d503201f        nop

[.....]

Note: I am overlooking the alignment requirements here, we might need to add another nop above the hash to make sure the top two nops are aligned at 8 bytes.

I am not sure how useful this solution is, looking forward to hear from others who know more about this topic.

Thanks,
Puranjay

[1] https://lore.kernel.org/bpf/[email protected]/

I hacked some patches and tried the above approach:

Both CFI and DIRECT CALLS are enabled:

[root@localhost ~]# zcat /proc/config.gz | grep DIRECT_CALLS CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS=y CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS=y [root@localhost ~]# zcat /proc/config.gz | grep CONFIG_CFI CONFIG_CFI_CLANG=y
CONFIG_CFI_PERMISSIVE=y CONFIG_CFI_WITHOUT_STATIC_CALL=y

Running some BPF selftests that use this feature.

./test_progs -a "*fentry*,*fexit*,tracing_struct" -b "fentry_test/fentry_many_args" -b "fexit_test/fexit_many_args"

#77      fentry_attach_btf_presence:OK
#78      fentry_fexit:OK
#79/1    fentry_test/fentry:OK
#79      fentry_test:OK
#80/1    fexit_bpf2bpf/target_no_callees:OK
#80/2    fexit_bpf2bpf/target_yes_callees:OK
#80/3    fexit_bpf2bpf/func_replace:OK
#80/4    fexit_bpf2bpf/func_replace_verify:OK
#80/5    fexit_bpf2bpf/func_sockmap_update:OK
#80/6    fexit_bpf2bpf/func_replace_return_code:OK
#80/7    fexit_bpf2bpf/func_map_prog_compatibility:OK
#80/8    fexit_bpf2bpf/func_replace_unreliable:OK
#80/9    fexit_bpf2bpf/func_replace_multi:OK
#80/10   fexit_bpf2bpf/fmod_ret_freplace:OK
#80/11   fexit_bpf2bpf/func_replace_global_func:OK
#80/12   fexit_bpf2bpf/fentry_to_cgroup_bpf:OK
#80/13   fexit_bpf2bpf/func_replace_progmap:OK
#80      fexit_bpf2bpf:OK
#81      fexit_sleep:OK
#82      fexit_stress:OK
#83/1    fexit_test/fexit:OK
#83      fexit_test:OK
#158     module_fentry_shadow:OK
#193/1   recursive_fentry/attach:OK
#193/2   recursive_fentry/load:OK
#193/3   recursive_fentry/detach:OK
#193     recursive_fentry:OK
#385     tracing_struct:OK

Summary: 10/18 PASSED, 0 SKIPPED, 0 FAILED

Running basic ftrace selftest:

[root@localhost ftrace]# ./ftracetest test.d/00basic/ === Ftrace unit tests ===

[1] Basic trace file check      [PASS]
[2] Basic test for tracers      [PASS]
[3] Basic trace clock test      [PASS]
[4] Basic event tracing check   [PASS]
[5] Change the ringbuffer size  [PASS]

[6] Change the ringbuffer sub-buffer size [PASS]
[7] Snapshot and tracing setting [PASS]
[8] Snapshot and tracing_cpumask [PASS]
[9] Test file and directory owership changes for eventfs [PASS]
[10] Basic tests on writing to trace_marker [PASS]
[11] trace_pipe and trace_marker [PASS]

[12] (instance)  Basic test for tracers [PASS]
[13] (instance)  Basic trace clock test [PASS]
[14] (instance)  Change the ringbuffer size     [PASS]
[15] (instance)  Change the ringbuffer sub-buffer size  [PASS]
[16] (instance)  Snapshot and tracing setting   [PASS]
[17] (instance)  Snapshot and tracing_cpumask   [PASS]
[18] (instance)  Basic tests on writing to trace_marker [PASS]
[19] (instance)  trace_pipe and trace_marker    [PASS]

# of passed:  19
# of failed:  0
# of unresolved:  0
# of untested:  0
# of unsupported:  0
# of xfailed:  0
# of undefined(test bug):  0

Here are the patches(RFC) that I created:

LLVM Patch:

--- >8 ---

index e89a1c26c..f1ca33c26 100644
@@ -982,9 +982,6 @@ void AsmPrinter::emitFunctionHeader() {
     }
   }
 
-  // Emit KCFI type information before patchable-function-prefix nops.
-  emitKCFITypeId(*MF);
-
   // Emit M NOPs for -fpatchable-function-entry=N,M where M>0. We arbitrarily
   // place prefix data before NOPs.
   unsigned PatchableFunctionPrefix = 0;

@@ -1006,6 +1003,9 @@ void AsmPrinter::emitFunctionHeader() {
     CurrentPatchableFunctionEntrySym = CurrentFnBegin;
   }
 
+  // Emit KCFI type information after patchable-function-prefix nops.
+  emitKCFITypeId(*MF);
+
   // Emit the function prologue data for the indirect call sanitizer.
   if (const MDNode *MD = F.getMetadata(LLVMContext::MD_func_sanitize)) {
     assert(MD->getNumOperands() == 2);

index 4fa719ad6..678a38a6a 100644
@@ -474,20 +474,11 @@ void AArch64AsmPrinter::LowerKCFI_CHECK(const MachineInstr &MI) {
     assert(ScratchRegs[0] != AddrReg && ScratchRegs[1] != AddrReg &&
            "Invalid scratch registers for KCFI_CHECK");
 
-    // Adjust the offset for patchable-function-prefix. This assumes that
-    // patchable-function-prefix is the same for all functions.
-    int64_t PrefixNops = 0;
-    (void)MI.getMF()
-        ->getFunction()
-        .getFnAttribute("patchable-function-prefix")
-        .getValueAsString()
-        .getAsInteger(10, PrefixNops);
-
     // Load the target function type hash.
     EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::LDURWi)
                                      .addReg(ScratchRegs[0])
                                      .addReg(AddrReg)
-                                     .addImm(-(PrefixNops * 4 + 4)));
+                                     .addImm(-4));
   }
 
   // Load the expected type hash.

--- 8< ---

Kernel Patch:

--- >8 ---

index aa7c1d435..979c290e6 100644
@@ -197,7 +197,7 @@ config ARM64
        select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS \
                if DYNAMIC_FTRACE_WITH_ARGS && DYNAMIC_FTRACE_WITH_CALL_OPS
        select HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS \
-               if (DYNAMIC_FTRACE_WITH_ARGS && !CFI_CLANG && \
+               if (DYNAMIC_FTRACE_WITH_ARGS && \
                    !CC_OPTIMIZE_FOR_SIZE)
        select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY \
                if DYNAMIC_FTRACE_WITH_ARGS

index f0c16640e..9be421583 100644
@@ -48,8 +48,15 @@ SYM_CODE_START(ftrace_caller)
         * alignment first as this allows us to fold the subtraction into the
         * LDR.
         */
+
+#ifdef CONFIG_CFI_CLANG
+       sub     x11, x30, #20
+       bic     x11, x11, 0x7
+       ldr     x11, [x11, 0]           // op
+#else
        bic     x11, x30, 0x7
        ldr     x11, [x11, #-(4 * AARCH64_INSN_SIZE)]           // op
+#endif
 
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
        /*

index a650f5e11..ed7a86b31 100644
@@ -125,6 +125,10 @@ unsigned long ftrace_call_adjust(unsigned long addr)
        /* Skip the NOPs placed before the function entry point */
        addr += 2 * AARCH64_INSN_SIZE;
 
+       /* Skip the hash in case of CLANG_CFI */
+       if (IS_ENABLED(CONFIG_CFI_CLANG))
+               addr += AARCH64_INSN_SIZE;
+
        /* Skip any BTI */
        if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL)) {
                u32 insn = le32_to_cpu(*(__le32 *)addr);

@@ -299,7 +303,11 @@ static const struct ftrace_ops *arm64_rec_get_ops(struct dyn_ftrace *rec)
 static int ftrace_rec_set_ops(const struct dyn_ftrace *rec,
                              const struct ftrace_ops *ops)
 {
+#ifdef CONFIG_CFI_CLANG
+       unsigned long literal = ALIGN_DOWN(rec->ip - 16, 8);
+#elif
        unsigned long literal = ALIGN_DOWN(rec->ip - 12, 8);
+#endif
        return aarch64_insn_write_literal_u64((void *)literal,
                                              (unsigned long)ops);
 }

--- 8< ---

I applied the kernel patch over my tree that has patches to fix kCFI with BPF and also has a static call related fix [1].

Thanks,
Puranjay

[1] https://github.com/puranjaymohan/linux/tree/kcfi_bpf

Hi Puranjay,

Did this LLVM patch ever make it upstream? I'd like to use CFI together with BPF LSM on arm64 and wondering what the current state of this effort is.

Thanks,
Clayton