| Message ID | 20240225-arm32-cfi-v1-0-6943306f065b@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | CFI for ARM32 using LLVM | expand |
On Sun, Feb 25, 2024 at 09:08:09PM +0100, Linus Walleij wrote: > This is a first patch set to support CLANG CFI (Control Flow > Integrity) on ARM32. Yay! Is CONFIG_CFI_PERMISSIVE=y expected to work with this series? I wasn't able to build with CONFIG_FTRACE=y; I got this link error: ld.lld: error: undefined symbol: ftrace_stub_graph (FWIW, I'm building against v6.8-rc2, maybe I need a different base?) But yes, I can boot and prototype mismatches are caught. Whee! :) Tested-by: Kees Cook <keescook@chromium.org> -Kees
Hi Linus, On Sun, Feb 25, 2024 at 09:08:09PM +0100, Linus Walleij wrote: > This is a first patch set to support CLANG CFI (Control Flow > Integrity) on ARM32. > > For information about what CFI is, see: > https://clang.llvm.org/docs/ControlFlowIntegrity.html > > For the kernel KCFI flavor, see: > https://lwn.net/Articles/898040/ > > The base changes required to bring up KCFI on ARM32 was mostly > related to the use of custom vtables in the kernel, combined > with defines to call into these vtable members directly from > sites where they are used. > > The approach to all of these vtable+define issues has been > the same: instead of a define, wrap the call in a static inline > function that explicitly calls the vtable member. > > To runtime-test the patches: > - Enable CONFIG_LKDTM > - echo CFI_FORWARD_PROTO > /sys/kernel/debug/provoke-crash/DIRECT > > The patch set has been booted to userspace on the following > test platforms: > > - Arm Versatile (QEMU) > - Arm Versatile Express (QEMU) > - multi_v7 booted on Versatile Express (QEMU) > - Footbridge Netwinder (SA110 ARMv4) > - Ux500 (ARMv7 SMP) > > I am not saying there will not be corner cases that we need > to fix in addition to this, but it is enough to get started. > Looking at what was fixed for arm64 I am a bit weary that > e.g. BPF might need something to trampoline properly. > > But hopefullt people can get to testing it and help me fix > remaining issues before the final version, or we can fix it > in-tree. This is awesome, thanks a lot for working on this! I went to take this for a spin in QEMU using the virt platform. When booting via EFI using edk2 in a manner similar to [1] but with 32-bit ARM virtual firmware (which I implemented in our boot utility scripts at [2]), I get a panic on boot even with CONFIG_CFI_PERMISSIVE=y: [ 0.245260] Unhandled prefetch abort: breakpoint debug exception (0x002) at 0x00000000 [ 0.245924] Internal error: : 2 [#1] SMP ARM [ 0.246211] Modules linked in: [ 0.246486] CPU: 0 PID: 18 Comm: kworker/u2:1 Not tainted 6.8.0-rc6-00018-g7562486357bb #1 [ 0.246855] Hardware name: QEMU QEMU Virtual Machine, BIOS unknown 2/2/2022 [ 0.247244] Workqueue: efi_rts_wq efi_call_rts [ 0.247907] PC is at efi_call_rts+0x30c/0x330 [ 0.248158] LR is at efi_call_rts+0x1c/0x330 [ 0.248317] pc : [<c0fd9cb4>] lr : [<c0fd99c4>] psr: 000f0053 [ 0.248532] sp : e087df10 ip : 00000000 fp : c20f4205 [ 0.248728] r10: c20f4200 r9 : 600f0053 r8 : c036da10 [ 0.248906] r7 : 2017e01c r6 : e427e123 r5 : e080dbc0 r4 : c1e1ac5c [ 0.249182] r3 : 20101699 r2 : e080dbf8 r1 : e080dbf2 r0 : e080dbf4 [ 0.249468] Flags: nzcv IRQs on FIQs off Mode SVC_32 ISA ARM Segment none [ 0.249750] Control: 10c5387d Table: 4206406a DAC: 00000051 [ 0.250007] Register r0 information: 2-page vmalloc region starting at 0xe080c000 allocated at copy_process+0x14c/0xd24 [ 0.250574] Register r1 information: 2-page vmalloc region starting at 0xe080c000 allocated at copy_process+0x14c/0xd24 [ 0.250984] Register r2 information: 2-page vmalloc region starting at 0xe080c000 allocated at copy_process+0x14c/0xd24 [ 0.251387] Register r3 information: non-paged memory [ 0.251671] Register r4 information: non-slab/vmalloc memory [ 0.251963] Register r5 information: 2-page vmalloc region starting at 0xe080c000 allocated at copy_process+0x14c/0xd24 [ 0.252373] Register r6 information: vmalloc memory [ 0.252575] Register r7 information: non-paged memory [ 0.252777] Register r8 information: non-slab/vmalloc memory [ 0.252993] Register r9 information: non-paged memory [ 0.253189] Register r10 information: slab kmalloc-256 start c20f4200 pointer offset 0 size 256 [ 0.253832] Register r11 information: slab kmalloc-256 start c20f4200 pointer offset 5 size 256 [ 0.254199] Register r12 information: NULL pointer [ 0.254404] Process kworker/u2:1 (pid: 18, stack limit = 0x(ptrval)) [ 0.254838] Stack: (0xe087df10 to 0xe087e000) [ 0.255082] df00: 00000000 df7dec40 c20bb200 c20f4280 [ 0.255399] df20: c20f4205 c2005000 c20bb22c c1e1ac64 c20f4205 c036da10 c2005000 c20bb218 [ 0.255685] df40: c20bb250 00000040 c2005000 c20bb22c c20bb22c c2005020 c2049100 c20bb200 [ 0.255992] df60: 61c88647 c036e094 c20bb200 00000040 c2049784 c20b8654 c20b8640 c036dd54 [ 0.256289] df80: c20bb200 c2049100 00000000 c0374268 c20b8540 c0374130 00000000 00000000 [ 0.256581] dfa0: 00000000 00000000 00000000 c03001ac 00000000 00000000 00000000 00000000 [ 0.256883] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.257169] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [ 0.257986] efi_call_rts from process_scheduled_works+0x298/0x3d0 [ 0.258441] process_scheduled_works from worker_thread+0x340/0x514 [ 0.258688] worker_thread from kthread+0x138/0x164 [ 0.258888] kthread from ret_from_fork+0x14/0x28 [ 0.259096] Exception stack(0xe087dfb0 to 0xe087dff8) [ 0.259284] dfa0: 00000000 00000000 00000000 00000000 [ 0.259581] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.259875] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 0.260271] Code: e3a0109b e3a03000 ebcdcf7a eaffffd7 (e1200070) [ 0.260850] ---[ end trace 0000000000000000 ]--- It is not immediately obvious what EFI runtime call is triggering this, I assume that has something to do with the lack of traps due to the generic implementation. I'll see if I can dig more into this tomorrow but don't hesitate to jump in too :P [1]: https://mirrors.edge.kernel.org/pub/linux/kernel/people/will/docs/qemu/qemu-arm64-howto.html [2]: https://github.com/ClangBuiltLinux/boot-utils/pull/118 Cheers, Nathan
On Tue, Feb 27, 2024 at 2:06 AM Kees Cook <keescook@chromium.org> wrote: > On Sun, Feb 25, 2024 at 09:08:09PM +0100, Linus Walleij wrote: > > This is a first patch set to support CLANG CFI (Control Flow > > Integrity) on ARM32. > > Yay! > > Is CONFIG_CFI_PERMISSIVE=y expected to work with this series? I enable that and what happens when I trigger a crash is that the process shell is killed and I return to login prompt (busybox). I guess that is expected behaviour for permissive? > I wasn't able to build with CONFIG_FTRACE=y; I got this link error: > ld.lld: error: undefined symbol: ftrace_stub_graph > (FWIW, I'm building against v6.8-rc2, maybe I need a different base?) I get that too, from the buildbots, but I'm clueless about what it is. All of it is in generic code, nothing ARM32-specific, maybe some bug in v6.8-rc1 that was fixed (I wish...) Sami? > But yes, I can boot and prototype mismatches are caught. Whee! :) Sweet! Yours, Linus Walleij
On Tue, Feb 27, 2024 at 5:21 AM Nathan Chancellor <nathan@kernel.org> wrote: > I went to take this for a spin in QEMU using the virt platform. When > booting via EFI using edk2 in a manner similar to [1] but with 32-bit > ARM virtual firmware (which I implemented in our boot utility scripts > at [2]), I get a panic on boot even with CONFIG_CFI_PERMISSIVE=y: (...) > [ 0.247907] PC is at efi_call_rts+0x30c/0x330 > [ 0.248158] LR is at efi_call_rts+0x1c/0x330 (...) > [ 0.257986] efi_call_rts from process_scheduled_works+0x298/0x3d0 (...) > It is not immediately obvious what EFI runtime call is triggering this, Not to me either... > I assume that has something to do with the lack of traps due to the > generic implementation. I'll see if I can dig more into this tomorrow > but don't hesitate to jump in too :P I think it's Ard Biesheuvel territory, he's always on top of EFI stuff :) What I see is that efi_call_rts() calls arch_efi_call_virt_setup/efi_call_virt_save_flags/ efi_call_virt (dispatch to arch_efi_call_virt) /arch_efi_call_virt_teardown arch_efi_call_virt_setup and arch_efi_call_virt_teardown is there. So the default becomes arch_efi_call_virt from include/linux/efi.h which is: #define arch_efi_call_virt(p, f, args...) ((p)->f(args)) CFI is not going to like that because it just dereferences this random function pointer and calls it, I suppose? I *think* that in arch/arm/include/asm/efi.h we need something like... #undef arch_efi_call_virt #define arch_efi_call_virt(p, f, args...) \ __efi_arm_wrapper((p)->f, #f, args) Then __efi_arm_wrapper() can be a static inline to call the function f, with the right arguments, tagged __nocfi? The details beats me, I'm not good with variadic functions, but if the solution is not obvious to you or Ard I can try to dig into it. Yours, Linus Walleij
On Tue, Feb 27, 2024 at 1:48 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Feb 27, 2024 at 2:06 AM Kees Cook <keescook@chromium.org> wrote: > > > I wasn't able to build with CONFIG_FTRACE=y; I got this link error: > > ld.lld: error: undefined symbol: ftrace_stub_graph > > (FWIW, I'm building against v6.8-rc2, maybe I need a different base?) > > I get that too, from the buildbots, but I'm clueless about what it is. > All of it is in generic code, nothing ARM32-specific, maybe some bug > in v6.8-rc1 that was fixed (I wish...) Sami? You need to just define a separate stub for graph tracing with KCFI to avoid type mismatches. Something like this: https://lore.kernel.org/all/20230710183544.999540-11-samitolvanen@google.com/ Sami
On Tue, Feb 27, 2024 at 02:48:13PM +0100, Linus Walleij wrote: > On Tue, Feb 27, 2024 at 2:06 AM Kees Cook <keescook@chromium.org> wrote: > > > On Sun, Feb 25, 2024 at 09:08:09PM +0100, Linus Walleij wrote: > > > This is a first patch set to support CLANG CFI (Control Flow > > > Integrity) on ARM32. > > > > Yay! > > > > Is CONFIG_CFI_PERMISSIVE=y expected to work with this series? > > I enable that and what happens when I trigger a crash is that the > process shell is killed and I return to login prompt (busybox). > I guess that is expected behaviour for permissive? No, permissive should just issue a WARN and continue running. I think you need wire up report_cfi_failure(), see cfi_handler() in arm64. But non-permissive is working great! :)
On Tue, Feb 27, 2024 at 6:44 PM Sami Tolvanen <samitolvanen@google.com> wrote: > On Tue, Feb 27, 2024 at 1:48 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Feb 27, 2024 at 2:06 AM Kees Cook <keescook@chromium.org> wrote: > > > > > I wasn't able to build with CONFIG_FTRACE=y; I got this link error: > > > ld.lld: error: undefined symbol: ftrace_stub_graph > > > (FWIW, I'm building against v6.8-rc2, maybe I need a different base?) > > > > I get that too, from the buildbots, but I'm clueless about what it is. > > All of it is in generic code, nothing ARM32-specific, maybe some bug > > in v6.8-rc1 that was fixed (I wish...) Sami? > > You need to just define a separate stub for graph tracing with KCFI to > avoid type mismatches. Something like this: > > https://lore.kernel.org/all/20230710183544.999540-11-samitolvanen@google.com/ I managed to fix this so all compiles work now! Thanks Sami. Yours, Linus Walleij
This is a first patch set to support CLANG CFI (Control Flow Integrity) on ARM32. For information about what CFI is, see: https://clang.llvm.org/docs/ControlFlowIntegrity.html For the kernel KCFI flavor, see: https://lwn.net/Articles/898040/ The base changes required to bring up KCFI on ARM32 was mostly related to the use of custom vtables in the kernel, combined with defines to call into these vtable members directly from sites where they are used. The approach to all of these vtable+define issues has been the same: instead of a define, wrap the call in a static inline function that explicitly calls the vtable member. To runtime-test the patches: - Enable CONFIG_LKDTM - echo CFI_FORWARD_PROTO > /sys/kernel/debug/provoke-crash/DIRECT The patch set has been booted to userspace on the following test platforms: - Arm Versatile (QEMU) - Arm Versatile Express (QEMU) - multi_v7 booted on Versatile Express (QEMU) - Footbridge Netwinder (SA110 ARMv4) - Ux500 (ARMv7 SMP) I am not saying there will not be corner cases that we need to fix in addition to this, but it is enough to get started. Looking at what was fixed for arm64 I am a bit weary that e.g. BPF might need something to trampoline properly. But hopefullt people can get to testing it and help me fix remaining issues before the final version, or we can fix it in-tree. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Linus Walleij (7): ARM: Support CLANG CFI ARM: tlbflush: Make TLB flushes into static inlines ARM: bugs: Check in the vtable instead of defined aliases ARM: proc: Use inlines instead of defines ARM: delay: Turn delay functions into static inlines ARM: turn CPU cache flush functions into static inlines ARM: page: Turn highpage accesses into static inlines arch/arm/Kconfig | 1 + arch/arm/common/mcpm_entry.c | 10 ++----- arch/arm/include/asm/cacheflush.h | 45 ++++++++++++++++++++++++------- arch/arm/include/asm/delay.h | 16 ++++++++--- arch/arm/include/asm/page.h | 36 ++++++++++++++++++++----- arch/arm/include/asm/proc-fns.h | 57 ++++++++++++++++++++++++++++++++------- arch/arm/include/asm/tlbflush.h | 18 ++++++++----- arch/arm/kernel/bugs.c | 2 +- arch/arm/mach-sunxi/mc_smp.c | 7 +---- arch/arm/mm/dma.h | 28 ++++++++++++++----- arch/arm/mm/proc-syms.c | 7 +---- arch/arm/mm/proc-v7-bugs.c | 4 +-- 12 files changed, 167 insertions(+), 64 deletions(-) --- base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d change-id: 20240115-arm32-cfi-65d60f201108 Best regards,