[PATCH bpf-next v3 0/2] limit bpf_core_types_are_compat recursion

[PATCH bpf-next v3 0/2] limit bpf_core_types_are_compat recursion

[Matteo Croce]

From: Matteo Croce <mcroce@microsoft.com>

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

[PATCH bpf-next v3 1/2] bpf: limit bpf_core_types_are_compat() recursion

[Matteo Croce]

From: Matteo Croce <mcroce@microsoft.com>

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 <mcroce@microsoft.com>
---
 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

[PATCH bpf-next v3 2/2] selftests/bpf: test maximum recursion depth for bpf_core_types_are_compat()

[Matteo Croce]

From: Matteo Croce <mcroce@microsoft.com>

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 <mcroce@microsoft.com>
---
 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

Re: [PATCH bpf-next v3 1/2] bpf: limit bpf_core_types_are_compat() recursion

[Alexei Starovoitov]

On Thu, Feb 3, 2022 at 4:55 PM Matteo Croce <mcroce@linux.microsoft.com> 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.

Re: [PATCH bpf-next v3 2/2] selftests/bpf: test maximum recursion depth for bpf_core_types_are_compat()

[Alexei Starovoitov]

On Thu, Feb 3, 2022 at 4:55 PM Matteo Croce <mcroce@linux.microsoft.com> wrote:
> --- 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);

There is no need for "___flavor" on the kernel side of type definition.
It makes the test confusing to read.

> +func_proto_typedef___match funcp = NULL;
> +func_proto_typedef___overflow funcp_of = NULL;

We have BTF_TYPE_EMIT() macro to avoid unnecessary declaration.

> +typedef int (*func_proto_typedef___match)(long);
> +typedef int (*func_proto_typedef___overflow)(func_proto_typedef___match);

With <=1 in the previous patch such single depth of func_proto
was reaching the recursion limit.
Hence the fix <=0 was necessary.
I've also changed this test to:

+typedef int (*func_proto_typedef)(long);
+typedef int (*func_proto_typedef_nested1)(func_proto_typedef);
+typedef int (*func_proto_typedef_nested2)(func_proto_typedef_nested1);

in bpf_testmod.c and in progs/core_kern_overflow.c
and
bpf_core_type_exists(func_proto_typedef_nested2);
to go above the limit.

Also added bpf_core_type_exists(func_proto_typedef_nested1)
to progs/core_kern.c to stay at the limit.

Please see the result in bpf-next.

Re: [PATCH bpf-next v3 0/2] limit bpf_core_types_are_compat recursion

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Fri,  4 Feb 2022 01:55:17 +0100 you wrote:
> From: Matteo Croce <mcroce@microsoft.com>
> 
> 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

Re: [PATCH bpf-next v3 2/2] selftests/bpf: test maximum recursion depth for bpf_core_types_are_compat()

[Matteo Croce]

On Fri, Feb 4, 2022 at 8:38 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Feb 3, 2022 at 4:55 PM Matteo Croce <mcroce@linux.microsoft.com> wrote:
> > --- 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);
>
> There is no need for "___flavor" on the kernel side of type definition.
> It makes the test confusing to read.
>
> > +func_proto_typedef___match funcp = NULL;
> > +func_proto_typedef___overflow funcp_of = NULL;
>
> We have BTF_TYPE_EMIT() macro to avoid unnecessary declaration.
>
> > +typedef int (*func_proto_typedef___match)(long);
> > +typedef int (*func_proto_typedef___overflow)(func_proto_typedef___match);
>
> With <=1 in the previous patch such single depth of func_proto
> was reaching the recursion limit.
> Hence the fix <=0 was necessary.
> I've also changed this test to:
>
> +typedef int (*func_proto_typedef)(long);
> +typedef int (*func_proto_typedef_nested1)(func_proto_typedef);
> +typedef int (*func_proto_typedef_nested2)(func_proto_typedef_nested1);
>
> in bpf_testmod.c and in progs/core_kern_overflow.c
> and
> bpf_core_type_exists(func_proto_typedef_nested2);
> to go above the limit.
>
> Also added bpf_core_type_exists(func_proto_typedef_nested1)
> to progs/core_kern.c to stay at the limit.
>
> Please see the result in bpf-next.

Awesome.
I've seen both patches in the repo, LGTM.

-- 
per aspera ad upstream