* [PATCH bpf-next v4 0/3] Support kCFI + BPF on arm64
@ 2024-05-13 14:08 Maxwell Bland
2024-05-13 14:10 ` [PATCH bpf-next v4 1/3] cfi: add C CFI type macro Maxwell Bland
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Maxwell Bland @ 2024-05-13 14:08 UTC (permalink / raw
To: open list:BPF [GENERAL] (Safe Dynamic Programs and Tools)
Cc: Catalin Marinas, Will Deacon, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Zi Shen Lim, Mark Rutland, Suzuki K Poulose,
Mark Brown, linux-arm-kernel, open list, Josh Poimboeuf,
Puranjay Mohan
For the BPF summit meeting tomorrow, I might as well have a mergable
version. I took a look back on BPF-CFI patches to check the status and
found that there had been no updates for around a month, so I went ahead
and made the fixes suggested in v2.
E.g.
ffff80008021d5a4 <reuseport_array_lookup_elem>:
ffff80008021d5a4: d503245f bti c
Potentially this should be replaced by a proper paciasp + autiasp, but I
suppose if we can assume the verifier provides back-edge integrity.
Changes in v3->v4
https://lore.kernel.org/all/fhdcjdzqdqnoehenxbipfaorseeamt3q7fbm7ghe6z5s2chif5@lrhtasolawud/
- Fix authorship attribution
Changes in v2->v3:
https://lore.kernel.org/all/20240324211518.93892-1-puranjay12@gmail.com/
- Simplify cfi_get_func_hash to avoid needless failure case
- Use DEFINE_CFI_TYPE as suggested by Mark Rutland
Changes in v1->v2:
https://lore.kernel.org/bpf/20240227151115.4623-1-puranjay12@gmail.com/
- Rebased on latest bpf-next/master
Mark Rutland (1):
cfi: add C CFI type macro
Maxwell Bland (1):
arm64/cfi,bpf: Use DEFINE_CFI_TYPE in arm64
Puranjay Mohan (1):
arm64/cfi,bpf: Support kCFI + BPF on arm64
arch/arm64/include/asm/cfi.h | 23 ++++++++++++++++++++++
arch/arm64/kernel/alternative.c | 18 +++++++++++++++++
arch/arm64/net/bpf_jit_comp.c | 18 +++++++++++++++--
arch/riscv/kernel/cfi.c | 34 ++------------------------------
arch/x86/kernel/alternative.c | 35 +++------------------------------
include/linux/cfi_types.h | 23 ++++++++++++++++++++++
6 files changed, 85 insertions(+), 66 deletions(-)
create mode 100644 arch/arm64/include/asm/cfi.h
base-commit: 329a6720a3ebbc041983b267981ab2cac102de93
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next v4 1/3] cfi: add C CFI type macro
2024-05-13 14:08 [PATCH bpf-next v4 0/3] Support kCFI + BPF on arm64 Maxwell Bland
@ 2024-05-13 14:10 ` Maxwell Bland
2024-05-13 14:12 ` [PATCH bpf-next v4 2/3] arm64/cfi,bpf: Support kCFI + BPF on arm64 Maxwell Bland
2024-05-13 14:14 ` [PATCH bpf-next v4 3/3] arm64/cfi,bpf: Use DEFINE_CFI_TYPE in arm64 Maxwell Bland
2 siblings, 0 replies; 6+ messages in thread
From: Maxwell Bland @ 2024-05-13 14:10 UTC (permalink / raw
To: open list:BPF [GENERAL] (Safe Dynamic Programs and Tools)
Cc: Catalin Marinas, Will Deacon, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Zi Shen Lim, Mark Rutland, Suzuki K Poulose,
Mark Brown, linux-arm-kernel, open list, Josh Poimboeuf,
Puranjay Mohan
From: Mark Rutland <mark.rutland@arm.com>
Currently x86 and riscv open-code 4 instances of the same logic to
define a u32 variable with the KCFI typeid of a given function.
Replace the duplicate logic with a common macro.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
Minor fix of authorship attribution by adding "From:" field!
arch/riscv/kernel/cfi.c | 34 ++--------------------------------
arch/x86/kernel/alternative.c | 35 +++--------------------------------
include/linux/cfi_types.h | 23 +++++++++++++++++++++++
3 files changed, 28 insertions(+), 64 deletions(-)
diff --git a/arch/riscv/kernel/cfi.c b/arch/riscv/kernel/cfi.c
index 64bdd3e1ab8c..b78a6f41df22 100644
--- a/arch/riscv/kernel/cfi.c
+++ b/arch/riscv/kernel/cfi.c
@@ -82,41 +82,11 @@ struct bpf_insn;
/* Must match bpf_func_t / DEFINE_BPF_PROG_RUN() */
extern unsigned int __bpf_prog_runX(const void *ctx,
const struct bpf_insn *insn);
-
-/*
- * Force a reference to the external symbol so the compiler generates
- * __kcfi_typid.
- */
-__ADDRESSABLE(__bpf_prog_runX);
-
-/* u32 __ro_after_init cfi_bpf_hash = __kcfi_typeid___bpf_prog_runX; */
-asm (
-" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
-" .type cfi_bpf_hash,@object \n"
-" .globl cfi_bpf_hash \n"
-" .p2align 2, 0x0 \n"
-"cfi_bpf_hash: \n"
-" .word __kcfi_typeid___bpf_prog_runX \n"
-" .size cfi_bpf_hash, 4 \n"
-" .popsection \n"
-);
+DEFINE_CFI_TYPE(cfi_bpf_hash, __bpf_prog_runX);
/* Must match bpf_callback_t */
extern u64 __bpf_callback_fn(u64, u64, u64, u64, u64);
-
-__ADDRESSABLE(__bpf_callback_fn);
-
-/* u32 __ro_after_init cfi_bpf_subprog_hash = __kcfi_typeid___bpf_callback_fn; */
-asm (
-" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
-" .type cfi_bpf_subprog_hash,@object \n"
-" .globl cfi_bpf_subprog_hash \n"
-" .p2align 2, 0x0 \n"
-"cfi_bpf_subprog_hash: \n"
-" .word __kcfi_typeid___bpf_callback_fn \n"
-" .size cfi_bpf_subprog_hash, 4 \n"
-" .popsection \n"
-);
+DEFINE_CFI_TYPE(cfi_bpf_subprog_hash, __bpf_callback_fn);
u32 cfi_get_func_hash(void *func)
{
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 45a280f2161c..a822699a40dd 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
#define pr_fmt(fmt) "SMP alternatives: " fmt
+#include <linux/cfi_types.h>
#include <linux/module.h>
#include <linux/sched.h>
#include <linux/perf_event.h>
@@ -918,41 +919,11 @@ struct bpf_insn;
/* Must match bpf_func_t / DEFINE_BPF_PROG_RUN() */
extern unsigned int __bpf_prog_runX(const void *ctx,
const struct bpf_insn *insn);
-
-/*
- * Force a reference to the external symbol so the compiler generates
- * __kcfi_typid.
- */
-__ADDRESSABLE(__bpf_prog_runX);
-
-/* u32 __ro_after_init cfi_bpf_hash = __kcfi_typeid___bpf_prog_runX; */
-asm (
-" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
-" .type cfi_bpf_hash,@object \n"
-" .globl cfi_bpf_hash \n"
-" .p2align 2, 0x0 \n"
-"cfi_bpf_hash: \n"
-" .long __kcfi_typeid___bpf_prog_runX \n"
-" .size cfi_bpf_hash, 4 \n"
-" .popsection \n"
-);
+DEFINE_CFI_TYPE(cfi_bpf_hash, __bpf_prog_runX);
/* Must match bpf_callback_t */
extern u64 __bpf_callback_fn(u64, u64, u64, u64, u64);
-
-__ADDRESSABLE(__bpf_callback_fn);
-
-/* u32 __ro_after_init cfi_bpf_subprog_hash = __kcfi_typeid___bpf_callback_fn; */
-asm (
-" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
-" .type cfi_bpf_subprog_hash,@object \n"
-" .globl cfi_bpf_subprog_hash \n"
-" .p2align 2, 0x0 \n"
-"cfi_bpf_subprog_hash: \n"
-" .long __kcfi_typeid___bpf_callback_fn \n"
-" .size cfi_bpf_subprog_hash, 4 \n"
-" .popsection \n"
-);
+DEFINE_CFI_TYPE(cfi_bpf_subprog_hash, __bpf_callback_fn);
u32 cfi_get_func_hash(void *func)
{
diff --git a/include/linux/cfi_types.h b/include/linux/cfi_types.h
index 6b8713675765..f510e62ca8b1 100644
--- a/include/linux/cfi_types.h
+++ b/include/linux/cfi_types.h
@@ -41,5 +41,28 @@
SYM_TYPED_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)
#endif
+#else /* __ASSEMBLY__ */
+
+#ifdef CONFIG_CFI_CLANG
+#define DEFINE_CFI_TYPE(name, func) \
+ /* \
+ * Force a reference to the function so the compiler generates \
+ * __kcfi_typeid_<func>. \
+ */ \
+ __ADDRESSABLE(func); \
+ /* u32 name = __kcfi_typeid_<func> */ \
+ extern u32 name; \
+ asm ( \
+ " .pushsection .data..ro_after_init,\"aw\",@progbits \n" \
+ " .type " #name ",@object \n" \
+ " .globl " #name " \n" \
+ " .p2align 2, 0x0 \n" \
+ #name ": \n" \
+ " .long __kcfi_typeid_" #func " \n" \
+ " .size " #name ", 4 \n" \
+ " .popsection \n" \
+ );
+#endif
+
#endif /* __ASSEMBLY__ */
#endif /* _LINUX_CFI_TYPES_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next v4 2/3] arm64/cfi,bpf: Support kCFI + BPF on arm64
2024-05-13 14:08 [PATCH bpf-next v4 0/3] Support kCFI + BPF on arm64 Maxwell Bland
2024-05-13 14:10 ` [PATCH bpf-next v4 1/3] cfi: add C CFI type macro Maxwell Bland
@ 2024-05-13 14:12 ` Maxwell Bland
2024-05-13 16:39 ` Puranjay Mohan
2024-05-13 14:14 ` [PATCH bpf-next v4 3/3] arm64/cfi,bpf: Use DEFINE_CFI_TYPE in arm64 Maxwell Bland
2 siblings, 1 reply; 6+ messages in thread
From: Maxwell Bland @ 2024-05-13 14:12 UTC (permalink / raw
To: open list:BPF [GENERAL] (Safe Dynamic Programs and Tools)
Cc: Catalin Marinas, Will Deacon, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Zi Shen Lim, Mark Rutland, Suzuki K Poulose,
Mark Brown, linux-arm-kernel, open list, Josh Poimboeuf,
Puranjay Mohan
From: Puranjay Mohan <puranjay12@gmail.com>
Currently, bpf_dispatcher_*_func() is marked with `__nocfi` therefore
calling BPF programs from this interface doesn't cause CFI warnings.
When BPF programs are called directly from C: from BPF helpers or
struct_ops, CFI warnings are generated.
Implement proper CFI prologues for the BPF programs and callbacks and
drop __nocfi for arm64. Fix the trampoline generation code to emit kCFI
prologue when a struct_ops trampoline is being prepared.
Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
Minor fix of authorship attribution by adding "From:" tag.
arch/arm64/include/asm/cfi.h | 23 ++++++++++++++
arch/arm64/kernel/alternative.c | 54 +++++++++++++++++++++++++++++++++
arch/arm64/net/bpf_jit_comp.c | 18 +++++++++--
3 files changed, 93 insertions(+), 2 deletions(-)
create mode 100644 arch/arm64/include/asm/cfi.h
diff --git a/arch/arm64/include/asm/cfi.h b/arch/arm64/include/asm/cfi.h
new file mode 100644
index 000000000000..670e191f8628
--- /dev/null
+++ b/arch/arm64/include/asm/cfi.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARM64_CFI_H
+#define _ASM_ARM64_CFI_H
+
+#ifdef CONFIG_CFI_CLANG
+#define __bpfcall
+static inline int cfi_get_offset(void)
+{
+ return 4;
+}
+#define cfi_get_offset cfi_get_offset
+extern u32 cfi_bpf_hash;
+extern u32 cfi_bpf_subprog_hash;
+extern u32 cfi_get_func_hash(void *func);
+#else
+#define cfi_bpf_hash 0U
+#define cfi_bpf_subprog_hash 0U
+static inline u32 cfi_get_func_hash(void *func)
+{
+ return 0;
+}
+#endif /* CONFIG_CFI_CLANG */
+#endif /* _ASM_ARM64_CFI_H */
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 8ff6610af496..1715da7df137 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -13,6 +13,7 @@
#include <linux/elf.h>
#include <asm/cacheflush.h>
#include <asm/alternative.h>
+#include <asm/cfi.h>
#include <asm/cpufeature.h>
#include <asm/insn.h>
#include <asm/module.h>
@@ -298,3 +299,56 @@ noinstr void alt_cb_patch_nops(struct alt_instr *alt, __le32 *origptr,
updptr[i] = cpu_to_le32(aarch64_insn_gen_nop());
}
EXPORT_SYMBOL(alt_cb_patch_nops);
+
+#ifdef CONFIG_CFI_CLANG
+struct bpf_insn;
+
+/* Must match bpf_func_t / DEFINE_BPF_PROG_RUN() */
+extern unsigned int __bpf_prog_runX(const void *ctx,
+ const struct bpf_insn *insn);
+
+/*
+ * Force a reference to the external symbol so the compiler generates
+ * __kcfi_typid.
+ */
+__ADDRESSABLE(__bpf_prog_runX);
+
+/* u32 __ro_after_init cfi_bpf_hash = __kcfi_typeid___bpf_prog_runX; */
+asm (
+" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
+" .type cfi_bpf_hash,@object \n"
+" .globl cfi_bpf_hash \n"
+" .p2align 2, 0x0 \n"
+"cfi_bpf_hash: \n"
+" .word __kcfi_typeid___bpf_prog_runX \n"
+" .size cfi_bpf_hash, 4 \n"
+" .popsection \n"
+);
+
+/* Must match bpf_callback_t */
+extern u64 __bpf_callback_fn(u64, u64, u64, u64, u64);
+
+__ADDRESSABLE(__bpf_callback_fn);
+
+/* u32 __ro_after_init cfi_bpf_subprog_hash = __kcfi_typeid___bpf_callback_fn; */
+asm (
+" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
+" .type cfi_bpf_subprog_hash,@object \n"
+" .globl cfi_bpf_subprog_hash \n"
+" .p2align 2, 0x0 \n"
+"cfi_bpf_subprog_hash: \n"
+" .word __kcfi_typeid___bpf_callback_fn \n"
+" .size cfi_bpf_subprog_hash, 4 \n"
+" .popsection \n"
+);
+
+u32 cfi_get_func_hash(void *func)
+{
+ u32 hash;
+
+ if (get_kernel_nofault(hash, func - cfi_get_offset()))
+ return 0;
+
+ return hash;
+}
+#endif
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 76b91f36c729..703247457409 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -17,6 +17,7 @@
#include <asm/asm-extable.h>
#include <asm/byteorder.h>
#include <asm/cacheflush.h>
+#include <asm/cfi.h>
#include <asm/debug-monitors.h>
#include <asm/insn.h>
#include <asm/patching.h>
@@ -162,6 +163,12 @@ static inline void emit_bti(u32 insn, struct jit_ctx *ctx)
emit(insn, ctx);
}
+static inline void emit_kcfi(u32 hash, struct jit_ctx *ctx)
+{
+ if (IS_ENABLED(CONFIG_CFI_CLANG))
+ emit(hash, ctx);
+}
+
/*
* Kernel addresses in the vmalloc space use at most 48 bits, and the
* remaining bits are guaranteed to be 0x1. So we can compose the address
@@ -337,6 +344,7 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf,
*
*/
+ emit_kcfi(is_main_prog ? cfi_bpf_hash : cfi_bpf_subprog_hash, ctx);
/* bpf function may be invoked by 3 instruction types:
* 1. bl, attached via freplace to bpf prog via short jump
* 2. br, attached via freplace to bpf prog via long jump
@@ -1806,9 +1814,9 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
jit_data->ro_header = ro_header;
}
- prog->bpf_func = (void *)ctx.ro_image;
+ prog->bpf_func = (void *)ctx.ro_image + cfi_get_offset();
prog->jited = 1;
- prog->jited_len = prog_size;
+ prog->jited_len = prog_size - cfi_get_offset();
if (!prog->is_func || extra_pass) {
int i;
@@ -2072,6 +2080,12 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
/* return address locates above FP */
retaddr_off = stack_size + 8;
+ if (flags & BPF_TRAMP_F_INDIRECT) {
+ /*
+ * Indirect call for bpf_struct_ops
+ */
+ emit_kcfi(cfi_get_func_hash(func_addr), ctx);
+ }
/* bpf trampoline may be invoked by 3 instruction types:
* 1. bl, attached to bpf prog or kernel function via short jump
* 2. br, attached to bpf prog or kernel function via long jump
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next v4 3/3] arm64/cfi,bpf: Use DEFINE_CFI_TYPE in arm64
2024-05-13 14:08 [PATCH bpf-next v4 0/3] Support kCFI + BPF on arm64 Maxwell Bland
2024-05-13 14:10 ` [PATCH bpf-next v4 1/3] cfi: add C CFI type macro Maxwell Bland
2024-05-13 14:12 ` [PATCH bpf-next v4 2/3] arm64/cfi,bpf: Support kCFI + BPF on arm64 Maxwell Bland
@ 2024-05-13 14:14 ` Maxwell Bland
2 siblings, 0 replies; 6+ messages in thread
From: Maxwell Bland @ 2024-05-13 14:14 UTC (permalink / raw
To: open list:BPF [GENERAL] (Safe Dynamic Programs and Tools)
Cc: Catalin Marinas, Will Deacon, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Zi Shen Lim, Mark Rutland, Suzuki K Poulose,
Mark Brown, linux-arm-kernel, open list, Josh Poimboeuf,
Puranjay Mohan
Corrects Puranjay Mohan's commit to adopt Mark Rutland's
suggestion of using a C CFI type macro in kCFI+BPF.
Signed-off-by: Maxwell Bland <mbland@motorola.com>
---
arch/arm64/kernel/alternative.c | 46 ++++-----------------------------
1 file changed, 5 insertions(+), 41 deletions(-)
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 1715da7df137..d7a58eca7665 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -8,6 +8,7 @@
#define pr_fmt(fmt) "alternatives: " fmt
+#include <linux/cfi_types.h>
#include <linux/init.h>
#include <linux/cpu.h>
#include <linux/elf.h>
@@ -302,53 +303,16 @@ EXPORT_SYMBOL(alt_cb_patch_nops);
#ifdef CONFIG_CFI_CLANG
struct bpf_insn;
-
/* Must match bpf_func_t / DEFINE_BPF_PROG_RUN() */
extern unsigned int __bpf_prog_runX(const void *ctx,
const struct bpf_insn *insn);
-
-/*
- * Force a reference to the external symbol so the compiler generates
- * __kcfi_typid.
- */
-__ADDRESSABLE(__bpf_prog_runX);
-
-/* u32 __ro_after_init cfi_bpf_hash = __kcfi_typeid___bpf_prog_runX; */
-asm (
-" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
-" .type cfi_bpf_hash,@object \n"
-" .globl cfi_bpf_hash \n"
-" .p2align 2, 0x0 \n"
-"cfi_bpf_hash: \n"
-" .word __kcfi_typeid___bpf_prog_runX \n"
-" .size cfi_bpf_hash, 4 \n"
-" .popsection \n"
-);
-
+DEFINE_CFI_TYPE(cfi_bpf_hash, __bpf_prog_runX);
/* Must match bpf_callback_t */
extern u64 __bpf_callback_fn(u64, u64, u64, u64, u64);
-
-__ADDRESSABLE(__bpf_callback_fn);
-
-/* u32 __ro_after_init cfi_bpf_subprog_hash = __kcfi_typeid___bpf_callback_fn; */
-asm (
-" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
-" .type cfi_bpf_subprog_hash,@object \n"
-" .globl cfi_bpf_subprog_hash \n"
-" .p2align 2, 0x0 \n"
-"cfi_bpf_subprog_hash: \n"
-" .word __kcfi_typeid___bpf_callback_fn \n"
-" .size cfi_bpf_subprog_hash, 4 \n"
-" .popsection \n"
-);
-
+DEFINE_CFI_TYPE(cfi_bpf_subprog_hash, __bpf_callback_fn);
u32 cfi_get_func_hash(void *func)
{
- u32 hash;
-
- if (get_kernel_nofault(hash, func - cfi_get_offset()))
- return 0;
-
- return hash;
+ u32 *hashp = func - cfi_get_offset();
+ return READ_ONCE(*hashp);
}
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v4 2/3] arm64/cfi,bpf: Support kCFI + BPF on arm64
2024-05-13 14:12 ` [PATCH bpf-next v4 2/3] arm64/cfi,bpf: Support kCFI + BPF on arm64 Maxwell Bland
@ 2024-05-13 16:39 ` Puranjay Mohan
2024-05-15 16:05 ` Maxwell Bland
0 siblings, 1 reply; 6+ messages in thread
From: Puranjay Mohan @ 2024-05-13 16:39 UTC (permalink / raw
To: Maxwell Bland,
open list:BPF [GENERAL] (Safe Dynamic Programs and Tools)
Cc: Catalin Marinas, Will Deacon, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Zi Shen Lim, Mark Rutland, Suzuki K Poulose,
Mark Brown, linux-arm-kernel, open list, Josh Poimboeuf
Maxwell Bland <mbland@motorola.com> writes:
This patch has a subtle difference from the patch that I sent in v2[1]
Unfortunately, you didn't test this. :(
It will break BPF on an ARM64 kernel compiled with CONFIG_CFI_CLANG=y
See below:
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 76b91f36c729..703247457409 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -17,6 +17,7 @@
> #include <asm/asm-extable.h>
> #include <asm/byteorder.h>
> #include <asm/cacheflush.h>
> +#include <asm/cfi.h>
> #include <asm/debug-monitors.h>
> #include <asm/insn.h>
> #include <asm/patching.h>
> @@ -162,6 +163,12 @@ static inline void emit_bti(u32 insn, struct jit_ctx *ctx)
> emit(insn, ctx);
> }
>
> +static inline void emit_kcfi(u32 hash, struct jit_ctx *ctx)
> +{
> + if (IS_ENABLED(CONFIG_CFI_CLANG))
> + emit(hash, ctx);
> +}
> +
> /*
> * Kernel addresses in the vmalloc space use at most 48 bits, and the
> * remaining bits are guaranteed to be 0x1. So we can compose the address
> @@ -337,6 +344,7 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf,
> *
> */
In my original patch the hunk here looked something like:
--- >8 ---
- const int idx0 = ctx->idx;
int cur_offset;
/*
@@ -332,6 +338,8 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf,
*
*/
+ emit_kcfi(is_subprog ? cfi_bpf_subprog_hash : cfi_bpf_hash, ctx);
+ const int idx0 = ctx->idx;
--- 8< ---
moving idx0 = ctx->idx; after emit_kcfi() is important because later
this 'idx0' is used like:
cur_offset = ctx->idx - idx0;
if (cur_offset != PROLOGUE_OFFSET) {
pr_err_once("PROLOGUE_OFFSET = %d, expected %d!\n",
cur_offset, PROLOGUE_OFFSET);
return -1;
}
With the current version, when I boot the kernel I get:
[ 0.499207] bpf_jit: PROLOGUE_OFFSET = 13, expected 12!
and now no BPF program can be JITed!
Please fix this in the next version and test it by running:
./tools/testing/selftests/bpf/test_progs
Pay attention to the `rbtree_success` and the `dummy_st_ops` tests, they
are the important ones for this change.
[1] https://lore.kernel.org/all/20240324211518.93892-2-puranjay12@gmail.com/
Thanks,
Puranjay
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v4 2/3] arm64/cfi,bpf: Support kCFI + BPF on arm64
2024-05-13 16:39 ` Puranjay Mohan
@ 2024-05-15 16:05 ` Maxwell Bland
0 siblings, 0 replies; 6+ messages in thread
From: Maxwell Bland @ 2024-05-15 16:05 UTC (permalink / raw
To: Puranjay Mohan
Cc: Catalin Marinas, Will Deacon, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Zi Shen Lim, Mark Rutland, Suzuki K Poulose,
Mark Brown, linux-arm-kernel, open list, Josh Poimboeuf
On Mon, May 13, 2024 at 04:39:28PM GMT, Puranjay Mohan wrote:
> Maxwell Bland <mbland@motorola.com> writes:
>
> This patch has a subtle difference from the patch that I sent in v2[1]
>
> Unfortunately, you didn't test this. :(
>
> Puranjay
Ugh, this is terrible news, I am sorry, it has been busy and I did the
merge without thinking, though that is no excuse.
I will respin, do more thorough testing, and resubmit.
Thank you for catching the index error.
Maxwell Bland
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-15 16:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-13 14:08 [PATCH bpf-next v4 0/3] Support kCFI + BPF on arm64 Maxwell Bland
2024-05-13 14:10 ` [PATCH bpf-next v4 1/3] cfi: add C CFI type macro Maxwell Bland
2024-05-13 14:12 ` [PATCH bpf-next v4 2/3] arm64/cfi,bpf: Support kCFI + BPF on arm64 Maxwell Bland
2024-05-13 16:39 ` Puranjay Mohan
2024-05-15 16:05 ` Maxwell Bland
2024-05-13 14:14 ` [PATCH bpf-next v4 3/3] arm64/cfi,bpf: Use DEFINE_CFI_TYPE in arm64 Maxwell Bland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).