From: Matteo Croce <[email protected]>
As formerly discussed on the BPF mailing list:
https://lore.kernel.org/bpf/CAADnVQJDax2j0-7uyqdqFEnpB57om_z+Cqmi1O2QyLpHqkVKwA@mail.gmail.com/
changes from v2:
test the bpf_core_type_exists() return value, and check that the recursion
limit is enforced.
Matteo Croce (2):
bpf: limit bpf_core_types_are_compat() recursion
selftests/bpf: test maximum recursion depth for
bpf_core_types_are_compat()
include/linux/btf.h | 5 +
kernel/bpf/btf.c | 105 +++++++++++++++++-
tools/testing/selftests/bpf/Makefile | 2 +-
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 5 +
.../selftests/bpf/prog_tests/core_kern.c | 15 ++-
.../bpf/prog_tests/core_kern_overflow.c | 13 +++
tools/testing/selftests/bpf/progs/core_kern.c | 14 +++
.../selftests/bpf/progs/core_kern_overflow.c | 21 ++++
8 files changed, 177 insertions(+), 3 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/core_kern_overflow.c
create mode 100644 tools/testing/selftests/bpf/progs/core_kern_overflow.c
--
2.34.1
From: Matteo Croce <[email protected]>
In userspace, bpf_core_types_are_compat() is a recursive function which
can't be put in the kernel as is.
Limit the recursion depth to 2, to avoid potential stack overflows
in kernel.
Signed-off-by: Matteo Croce <[email protected]>
---
include/linux/btf.h | 5 +++
kernel/bpf/btf.c | 105 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 109 insertions(+), 1 deletion(-)
diff --git a/include/linux/btf.h b/include/linux/btf.h
index f6c43dd513fa..36bc09b8e890 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -327,6 +327,11 @@ static inline const struct btf_var_secinfo *btf_type_var_secinfo(
return (const struct btf_var_secinfo *)(t + 1);
}
+static inline struct btf_param *btf_params(const struct btf_type *t)
+{
+ return (struct btf_param *)(t + 1);
+}
+
#ifdef CONFIG_BPF_SYSCALL
struct bpf_prog;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index b983cee8d196..fcc3d9e45320 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6785,10 +6785,113 @@ int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
}
EXPORT_SYMBOL_GPL(register_btf_kfunc_id_set);
+#define MAX_TYPES_ARE_COMPAT_DEPTH 2
+
+static
+int __bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
+ const struct btf *targ_btf, __u32 targ_id,
+ int level)
+{
+ const struct btf_type *local_type, *targ_type;
+ int depth = 32; /* max recursion depth */
+
+ /* caller made sure that names match (ignoring flavor suffix) */
+ local_type = btf_type_by_id(local_btf, local_id);
+ targ_type = btf_type_by_id(targ_btf, targ_id);
+ if (btf_kind(local_type) != btf_kind(targ_type))
+ return 0;
+
+recur:
+ depth--;
+ if (depth < 0)
+ return -EINVAL;
+
+ local_type = btf_type_skip_modifiers(local_btf, local_id, &local_id);
+ targ_type = btf_type_skip_modifiers(targ_btf, targ_id, &targ_id);
+ if (!local_type || !targ_type)
+ return -EINVAL;
+
+ if (btf_kind(local_type) != btf_kind(targ_type))
+ return 0;
+
+ switch (btf_kind(local_type)) {
+ case BTF_KIND_UNKN:
+ case BTF_KIND_STRUCT:
+ case BTF_KIND_UNION:
+ case BTF_KIND_ENUM:
+ case BTF_KIND_FWD:
+ return 1;
+ case BTF_KIND_INT:
+ /* just reject deprecated bitfield-like integers; all other
+ * integers are by default compatible between each other
+ */
+ return btf_int_offset(local_type) == 0 && btf_int_offset(targ_type) == 0;
+ case BTF_KIND_PTR:
+ local_id = local_type->type;
+ targ_id = targ_type->type;
+ goto recur;
+ case BTF_KIND_ARRAY:
+ local_id = btf_array(local_type)->type;
+ targ_id = btf_array(targ_type)->type;
+ goto recur;
+ case BTF_KIND_FUNC_PROTO: {
+ struct btf_param *local_p = btf_params(local_type);
+ struct btf_param *targ_p = btf_params(targ_type);
+ __u16 local_vlen = btf_vlen(local_type);
+ __u16 targ_vlen = btf_vlen(targ_type);
+ int i, err;
+
+ if (local_vlen != targ_vlen)
+ return 0;
+
+ for (i = 0; i < local_vlen; i++, local_p++, targ_p++) {
+ if (level <= 1)
+ return -EINVAL;
+
+ btf_type_skip_modifiers(local_btf, local_p->type, &local_id);
+ btf_type_skip_modifiers(targ_btf, targ_p->type, &targ_id);
+ err = __bpf_core_types_are_compat(local_btf, local_id,
+ targ_btf, targ_id,
+ level - 1);
+ if (err <= 0)
+ return err;
+ }
+
+ /* tail recurse for return type check */
+ btf_type_skip_modifiers(local_btf, local_type->type, &local_id);
+ btf_type_skip_modifiers(targ_btf, targ_type->type, &targ_id);
+ goto recur;
+ }
+ default:
+ return 0;
+ }
+}
+
+/* Check local and target types for compatibility. This check is used for
+ * type-based CO-RE relocations and follow slightly different rules than
+ * field-based relocations. This function assumes that root types were already
+ * checked for name match. Beyond that initial root-level name check, names
+ * are completely ignored. Compatibility rules are as follows:
+ * - any two STRUCTs/UNIONs/FWDs/ENUMs/INTs are considered compatible, but
+ * kind should match for local and target types (i.e., STRUCT is not
+ * compatible with UNION);
+ * - for ENUMs, the size is ignored;
+ * - for INT, size and signedness are ignored;
+ * - for ARRAY, dimensionality is ignored, element types are checked for
+ * compatibility recursively;
+ * - CONST/VOLATILE/RESTRICT modifiers are ignored;
+ * - TYPEDEFs/PTRs are compatible if types they pointing to are compatible;
+ * - FUNC_PROTOs are compatible if they have compatible signature: same
+ * number of input args and compatible return and argument types.
+ * These rules are not set in stone and probably will be adjusted as we get
+ * more experience with using BPF CO-RE relocations.
+ */
int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
const struct btf *targ_btf, __u32 targ_id)
{
- return -EOPNOTSUPP;
+ return __bpf_core_types_are_compat(local_btf, local_id,
+ targ_btf, targ_id,
+ MAX_TYPES_ARE_COMPAT_DEPTH);
}
static bool bpf_core_is_flavor_sep(const char *s)
--
2.34.1
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <[email protected]>:
On Fri, 4 Feb 2022 01:55:17 +0100 you wrote:
> From: Matteo Croce <[email protected]>
>
> As formerly discussed on the BPF mailing list:
> https://lore.kernel.org/bpf/CAADnVQJDax2j0-7uyqdqFEnpB57om_z+Cqmi1O2QyLpHqkVKwA@mail.gmail.com/
>
> changes from v2:
> test the bpf_core_type_exists() return value, and check that the recursion
> limit is enforced.
>
> [...]
Here is the summary with links:
- [bpf-next,v3,1/2] bpf: limit bpf_core_types_are_compat() recursion
https://git.kernel.org/bpf/bpf-next/c/e70e13e7d4ab
- [bpf-next,v3,2/2] selftests/bpf: test maximum recursion depth for bpf_core_types_are_compat()
https://git.kernel.org/bpf/bpf-next/c/976a38e05a49
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
From: Matteo Croce <[email protected]>
bpf_core_types_are_compat() was limited to 2 recursion levels, which are
enough to parse a function prototype.
Add a test which checks the existence of a function prototype, so to
test the bpf_core_types_are_compat() code path.
The test for the recursion limit being hit is done in a separate object,
because the kernel failure makes the whole load to fail.
Sample run log with extra prints:
[ 5689.913751] bpf_core_apply_relo_insn:1200 cands->len: 2
[ 5689.913902] bpf_core_types_are_compat:6896: ret: 1
[ 5689.913994] bpf_core_types_are_compat:6896: ret: 0
[ 5689.914025] bpf_core_apply_relo_insn:1200 cands->len: 2
[ 5689.914141] bpf_core_types_are_compat:6896: ret: 0
[ 5689.914246] bpf_core_types_are_compat:6896: ret: 0
test_core_kern_lskel:PASS:open_and_load 0 nsec
test_core_kern_lskel:PASS:attach(core_relo_proto) 0 nsec
test_core_kern_lskel:PASS:bpf_core_type_exists 0 nsec
test_core_kern_lskel:PASS:!bpf_core_type_exists 0 nsec
#41 core_kern_lskel:OK
[ 5689.915267] bpf_core_apply_relo_insn:1200 cands->len: 2
[ 5689.915399] bpf_core_types_are_compat:6896: ret: 0
[ 5689.915504] bpf_core_types_are_compat:6896: ret: -22
test_core_kern_overflow_lskel:PASS:open_and_load 0 nsec
#42 core_kern_overflow_lskel:OK
Summary: 2/0 PASSED, 0 SKIPPED, 0 FAILED
Successfully unloaded bpf_testmod.ko.
Signed-off-by: Matteo Croce <[email protected]>
---
tools/testing/selftests/bpf/Makefile | 2 +-
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 5 +++++
.../selftests/bpf/prog_tests/core_kern.c | 15 ++++++++++++-
.../bpf/prog_tests/core_kern_overflow.c | 13 ++++++++++++
tools/testing/selftests/bpf/progs/core_kern.c | 14 +++++++++++++
.../selftests/bpf/progs/core_kern_overflow.c | 21 +++++++++++++++++++
6 files changed, 68 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/core_kern_overflow.c
create mode 100644 tools/testing/selftests/bpf/progs/core_kern_overflow.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 945f92d71db3..91ea729990da 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -330,7 +330,7 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h \
LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
- map_ptr_kern.c core_kern.c
+ map_ptr_kern.c core_kern.c core_kern_overflow.c
# Generate both light skeleton and libbpf skeleton for these
LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test_subprog.c
SKEL_BLACKLIST += $$(LSKELS)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 595d32ab285a..e5ba8d8a17da 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -13,6 +13,11 @@
#define CREATE_TRACE_POINTS
#include "bpf_testmod-events.h"
+typedef int (*func_proto_typedef___match)(long);
+typedef int (*func_proto_typedef___overflow)(func_proto_typedef___match);
+func_proto_typedef___match funcp = NULL;
+func_proto_typedef___overflow funcp_of = NULL;
+
DEFINE_PER_CPU(int, bpf_testmod_ksym_percpu) = 123;
noinline void
diff --git a/tools/testing/selftests/bpf/prog_tests/core_kern.c b/tools/testing/selftests/bpf/prog_tests/core_kern.c
index 561c5185d886..91493f5836ff 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_kern.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_kern.c
@@ -7,8 +7,21 @@
void test_core_kern_lskel(void)
{
struct core_kern_lskel *skel;
+ int link_fd;
skel = core_kern_lskel__open_and_load();
- ASSERT_OK_PTR(skel, "open_and_load");
+ if (!ASSERT_OK_PTR(skel, "open_and_load"))
+ return;
+
+ link_fd = core_kern_lskel__core_relo_proto__attach(skel);
+ if (!ASSERT_GT(link_fd, 0, "attach(core_relo_proto)"))
+ goto cleanup;
+
+ /* trigger tracepoints */
+ usleep(1);
+ ASSERT_TRUE(skel->bss->proto_out[0], "bpf_core_type_exists");
+ ASSERT_FALSE(skel->bss->proto_out[1], "!bpf_core_type_exists");
+
+cleanup:
core_kern_lskel__destroy(skel);
}
diff --git a/tools/testing/selftests/bpf/prog_tests/core_kern_overflow.c b/tools/testing/selftests/bpf/prog_tests/core_kern_overflow.c
new file mode 100644
index 000000000000..04cc145bc26a
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/core_kern_overflow.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "test_progs.h"
+#include "core_kern_overflow.lskel.h"
+
+void test_core_kern_overflow_lskel(void)
+{
+ struct core_kern_overflow_lskel *skel;
+
+ skel = core_kern_overflow_lskel__open_and_load();
+ if (!ASSERT_NULL(skel, "open_and_load"))
+ core_kern_overflow_lskel__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/core_kern.c b/tools/testing/selftests/bpf/progs/core_kern.c
index 13499cc15c7d..acabe4cb0480 100644
--- a/tools/testing/selftests/bpf/progs/core_kern.c
+++ b/tools/testing/selftests/bpf/progs/core_kern.c
@@ -101,4 +101,18 @@ int balancer_ingress(struct __sk_buff *ctx)
return 0;
}
+typedef int (*func_proto_typedef___match)(long);
+typedef void (*func_proto_typedef___doesnt_match)(char*);
+
+int proto_out[2];
+
+SEC("raw_tracepoint/sys_enter")
+int core_relo_proto(void *ctx)
+{
+ proto_out[0] = bpf_core_type_exists(func_proto_typedef___match);
+ proto_out[1] = bpf_core_type_exists(func_proto_typedef___doesnt_match);
+
+ return 0;
+}
+
char LICENSE[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/core_kern_overflow.c b/tools/testing/selftests/bpf/progs/core_kern_overflow.c
new file mode 100644
index 000000000000..70417413af55
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/core_kern_overflow.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
+
+typedef int (*func_proto_typedef___match)(long);
+typedef int (*func_proto_typedef___overflow)(func_proto_typedef___match);
+
+int proto_out;
+
+SEC("raw_tracepoint/sys_enter")
+int core_relo_proto(void *ctx)
+{
+ proto_out = bpf_core_type_exists(func_proto_typedef___overflow);
+
+ return 0;
+}
+
+char LICENSE[] SEC("license") = "GPL";
--
2.34.1
On Thu, Feb 3, 2022 at 4:55 PM Matteo Croce <[email protected]> wrote:
> +
> + for (i = 0; i < local_vlen; i++, local_p++, targ_p++) {
> + if (level <= 1)
> + return -EINVAL;
<= 1 is not correct, since it makes
MAX_TYPES_ARE_COMPAT_DEPTH 2
misleading.
With <= 1 the recursion depth is just 1.
So I've changed to to <=0 to make the actual depth of 2.