[PATCH bpf-next v4 0/3] Check cfi_stubs before registering a struct_ops type.

[PATCH bpf-next v4 0/3] Check cfi_stubs before registering a struct_ops type.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Recently, cfi_stubs were introduced. However, existing struct_ops
types that are not in the upstream may not be aware of this, resulting
in kernel crashes. By rejecting struct_ops types that do not provide
cfi_stubs properly during registration, these crashes can be avoided.

---
Changes from v3:

 - Remove CFI stub function for get_info.

 - Allow passing NULL prog arg to check_member of struct
   bpf_struct_ops type.

 - Call check_member to determines if a CFI stub function should be
   defined for an operator.

Changes from v2:

 - Add a stub function for get_info of struct tcp_congestion_ops.

Changes from v1:

 - Check *(void **)(cfi_stubs + moff) to make sure stub functions are
   provided for every operator.

 - Add a test case to ensure that struct_ops rejects incomplete
   cfi_stub.

v3: https://lore.kernel.org/all/20240216193434.735874-1-thinker.li@gmail.com/
v2: https://lore.kernel.org/all/20240216020350.2061373-1-thinker.li@gmail.com/
v1: https://lore.kernel.org/all/20240215022401.1882010-1-thinker.li@gmail.com/

Kui-Feng Lee (3):
  bpf, net: allow passing NULL prog to check_member.
  bpf: Check cfi_stubs before registering a struct_ops type.
  selftests/bpf: Test case for lacking CFI stub functions.

 kernel/bpf/bpf_struct_ops.c                   | 17 ++++
 net/bpf/bpf_dummy_struct_ops.c                |  2 +-
 tools/testing/selftests/bpf/Makefile          | 10 +-
 .../selftests/bpf/bpf_test_no_cfi/Makefile    | 19 ++++
 .../bpf/bpf_test_no_cfi/bpf_test_no_cfi.c     | 93 +++++++++++++++++++
 .../bpf/prog_tests/test_struct_ops_no_cfi.c   | 38 ++++++++
 tools/testing/selftests/bpf/testing_helpers.c |  4 +-
 tools/testing/selftests/bpf/testing_helpers.h |  2 +
 8 files changed, 181 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/bpf_test_no_cfi/Makefile
 create mode 100644 tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_no_cfi.c

-- 
2.34.1

[PATCH bpf-next v4 1/3] bpf, net: allow passing NULL prog to check_member.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

To reuse the check_member function of the bpf_struct_ops structure for
checking if an operator is supported, we permit passing a NULL value for
the "prog" argument in check_member. The check_member function of the
bpf_struct_ops structure will be utilized for checking cfi_stubs in a
subsequent patch when registering a struct_ops type.

netdev@vger.kernel.org
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 net/bpf/bpf_dummy_struct_ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 02de71719aed..5fe5461d3173 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -178,7 +178,7 @@ static int bpf_dummy_ops_check_member(const struct btf_type *t,
 	case offsetof(struct bpf_dummy_ops, test_sleepable):
 		break;
 	default:
-		if (prog->aux->sleepable)
+		if (prog && prog->aux->sleepable)
 			return -EINVAL;
 	}
 
-- 
2.34.1

[PATCH bpf-next v4 2/3] bpf: Check cfi_stubs before registering a struct_ops type.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Recently, cfi_stubs were introduced. However, existing struct_ops types
that are not in the upstream may not be aware of this, resulting in kernel
crashes. By rejecting struct_ops types that do not provide cfi_stubs during
registration, these crashes can be avoided.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/bpf/bpf_struct_ops.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 0d7be97a2411..c1c502caae08 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -302,6 +302,11 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
 	}
 	sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
 
+	if (!st_ops->cfi_stubs) {
+		pr_warn("struct %s has no cfi_stubs\n", st_ops->name);
+		return -EINVAL;
+	}
+
 	type_id = btf_find_by_name_kind(btf, st_ops->name,
 					BTF_KIND_STRUCT);
 	if (type_id < 0) {
@@ -339,6 +344,7 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
 
 	for_each_member(i, t, member) {
 		const struct btf_type *func_proto;
+		u32 moff;
 
 		mname = btf_name_by_offset(btf, member->name_off);
 		if (!*mname) {
@@ -361,6 +367,17 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
 		if (!func_proto)
 			continue;
 
+		moff = __btf_member_bit_offset(t, member) / 8;
+		err = st_ops->check_member ?
+			st_ops->check_member(t, member, NULL) : 0;
+
+		if (!err && !*(void **)(st_ops->cfi_stubs + moff)) {
+			pr_warn("member %s in struct %s has no cfi stub function\n",
+				mname, st_ops->name);
+			err = -EINVAL;
+			goto errout;
+		}
+
 		if (btf_distill_func_proto(log, btf,
 					   func_proto, mname,
 					   &st_ops->func_models[i])) {
-- 
2.34.1

[PATCH bpf-next v4 3/3] selftests/bpf: Test case for lacking CFI stub functions.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Ensure struct_ops rejects the registration of struct_ops types without
proper CFI stub functions.

bpf_test_no_cfi.ko is a module that attempts to register a struct_ops type
called "bpf_test_no_cfi_ops" with varying levels of cfi_stubs. It starts
with a NULL cfi_stub and ends with a fully complete cfi_stub. Only the
fully complete cfi_stub should be accepted by struct_ops. The module can
only be loaded successfully if these registrations yield the expected
results.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/testing/selftests/bpf/Makefile          | 10 +-
 .../selftests/bpf/bpf_test_no_cfi/Makefile    | 19 ++++
 .../bpf/bpf_test_no_cfi/bpf_test_no_cfi.c     | 93 +++++++++++++++++++
 .../bpf/prog_tests/test_struct_ops_no_cfi.c   | 38 ++++++++
 tools/testing/selftests/bpf/testing_helpers.c |  4 +-
 tools/testing/selftests/bpf/testing_helpers.h |  2 +
 6 files changed, 163 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/bpf_test_no_cfi/Makefile
 create mode 100644 tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_no_cfi.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index dbb8c5f94f34..c219da5e60e6 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -132,7 +132,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
 	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
 	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
 	xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
-	xdp_features
+	xdp_features bpf_test_no_cfi.ko
 
 TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
 
@@ -254,6 +254,12 @@ $(OUTPUT)/bpf_testmod.ko: $(VMLINUX_BTF) $(RESOLVE_BTFIDS) $(wildcard bpf_testmo
 	$(Q)$(MAKE) $(submake_extras) RESOLVE_BTFIDS=$(RESOLVE_BTFIDS) -C bpf_testmod
 	$(Q)cp bpf_testmod/bpf_testmod.ko $@
 
+$(OUTPUT)/bpf_test_no_cfi.ko: $(VMLINUX_BTF) $(RESOLVE_BTFIDS) $(wildcard bpf_test_no_cfi/Makefile bpf_test_no_cfi/*.[ch])
+	$(call msg,MOD,,$@)
+	$(Q)$(RM) bpf_test_no_cfi/bpf_test_no_cfi.ko # force re-compilation
+	$(Q)$(MAKE) $(submake_extras) RESOLVE_BTFIDS=$(RESOLVE_BTFIDS) -C bpf_test_no_cfi
+	$(Q)cp bpf_test_no_cfi/bpf_test_no_cfi.ko $@
+
 DEFAULT_BPFTOOL := $(HOST_SCRATCH_DIR)/sbin/bpftool
 ifneq ($(CROSS_COMPILE),)
 CROSS_BPFTOOL := $(SCRATCH_DIR)/sbin/bpftool
@@ -628,6 +634,7 @@ TRUNNER_EXTRA_SOURCES := test_progs.c		\
 			 flow_dissector_load.h	\
 			 ip_check_defrag_frags.h
 TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko	\
+		       $(OUTPUT)/bpf_test_no_cfi.ko			\
 		       $(OUTPUT)/liburandom_read.so			\
 		       $(OUTPUT)/xdp_synproxy				\
 		       $(OUTPUT)/sign-file				\
@@ -756,6 +763,7 @@ EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)			\
 	feature bpftool							\
 	$(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h *.subskel.h	\
 			       no_alu32 cpuv4 bpf_gcc bpf_testmod.ko	\
+			       bpf_test_no_cfi.ko			\
 			       liburandom_read.so)
 
 .PHONY: docs docs-clean
diff --git a/tools/testing/selftests/bpf/bpf_test_no_cfi/Makefile b/tools/testing/selftests/bpf/bpf_test_no_cfi/Makefile
new file mode 100644
index 000000000000..ed5143b79edf
--- /dev/null
+++ b/tools/testing/selftests/bpf/bpf_test_no_cfi/Makefile
@@ -0,0 +1,19 @@
+BPF_TEST_NO_CFI_DIR := $(realpath $(dir $(abspath $(lastword $(MAKEFILE_LIST)))))
+KDIR ?= $(abspath $(BPF_TEST_NO_CFI_DIR)/../../../../..)
+
+ifeq ($(V),1)
+Q =
+else
+Q = @
+endif
+
+MODULES = bpf_test_no_cfi.ko
+
+obj-m += bpf_test_no_cfi.o
+
+all:
+	+$(Q)make -C $(KDIR) M=$(BPF_TEST_NO_CFI_DIR) modules
+
+clean:
+	+$(Q)make -C $(KDIR) M=$(BPF_TEST_NO_CFI_DIR) clean
+
diff --git a/tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c b/tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c
new file mode 100644
index 000000000000..0fb63feecb31
--- /dev/null
+++ b/tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <linux/bpf.h>
+#include <linux/btf.h>
+#include <linux/init.h>
+#include <linux/module.h>
+
+struct bpf_test_no_cfi_ops {
+	void (*fn_1)(void);
+	void (*fn_2)(void);
+};
+
+static int dummy_init(struct btf *btf)
+{
+	return 0;
+}
+
+static int dummy_init_member(const struct btf_type *t,
+			     const struct btf_member *member,
+			     void *kdata, const void *udata)
+{
+	return 0;
+}
+
+static int dummy_reg(void *kdata)
+{
+	return 0;
+}
+
+static void dummy_unreg(void *kdata)
+{
+}
+
+static const struct bpf_verifier_ops dummy_verifier_ops;
+
+static void bpf_test_no_cfi_ops__fn_1(void)
+{
+}
+
+static void bpf_test_no_cfi_ops__fn_2(void)
+{
+}
+
+static struct bpf_test_no_cfi_ops __bpf_test_no_cfi_ops;
+
+static struct bpf_struct_ops bpf_bpf_test_no_cif_ops = {
+	.verifier_ops = &dummy_verifier_ops,
+	.init = dummy_init,
+	.init_member = dummy_init_member,
+	.reg = dummy_reg,
+	.unreg = dummy_unreg,
+	.name = "bpf_test_no_cfi_ops",
+	.owner = THIS_MODULE,
+};
+
+static int bpf_test_no_cfi_init(void)
+{
+	int ret;
+
+	ret = register_bpf_struct_ops(&bpf_bpf_test_no_cif_ops,
+				      bpf_test_no_cfi_ops);
+	if (!ret)
+		return -EINVAL;
+
+	bpf_bpf_test_no_cif_ops.cfi_stubs = &__bpf_test_no_cfi_ops;
+	ret = register_bpf_struct_ops(&bpf_bpf_test_no_cif_ops,
+				      bpf_test_no_cfi_ops);
+	if (!ret)
+		return -EINVAL;
+
+	__bpf_test_no_cfi_ops.fn_1 = bpf_test_no_cfi_ops__fn_1;
+	ret = register_bpf_struct_ops(&bpf_bpf_test_no_cif_ops,
+				      bpf_test_no_cfi_ops);
+	if (!ret)
+		return -EINVAL;
+
+	__bpf_test_no_cfi_ops.fn_2 = bpf_test_no_cfi_ops__fn_2;
+	ret = register_bpf_struct_ops(&bpf_bpf_test_no_cif_ops,
+				      bpf_test_no_cfi_ops);
+	return ret;
+}
+
+static void bpf_test_no_cfi_exit(void)
+{
+}
+
+module_init(bpf_test_no_cfi_init);
+module_exit(bpf_test_no_cfi_exit);
+
+MODULE_AUTHOR("Kuifeng Lee");
+MODULE_DESCRIPTION("BPF no cfi_stubs test module");
+MODULE_LICENSE("Dual BSD/GPL");
+
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_no_cfi.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_no_cfi.c
new file mode 100644
index 000000000000..19703e250549
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_no_cfi.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <testing_helpers.h>
+
+static void load_bpf_test_no_cfi(void)
+{
+	int fd;
+	int err;
+
+	fd = open("bpf_test_no_cfi.ko", O_RDONLY);
+	if (!ASSERT_GT(fd, 0, "open")) {
+		close(fd);
+		return;
+	}
+
+	/* The module will try to register a struct_ops type with
+	 * no cfi_stubs, incomplete cfi_stubs, and full cfi_stubs.
+	 *
+	 * Only full cfi_stubs should be allowed. The module will be loaded
+	 * successfully if the result of the registration is as expected,
+	 * or it fails.
+	 */
+	err = finit_module(fd, "", 0);
+	close(fd);
+	if (!ASSERT_OK(err, "finit_module"))
+		return;
+
+	err = delete_module("bpf_test_no_cfi", 0);
+	ASSERT_OK(err, "delete_module");
+}
+
+void test_struct_ops_no_cfi(void)
+{
+	if (test__start_subtest("load_bpf_test_no_cfi"))
+		load_bpf_test_no_cfi();
+}
+
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index a59e56d804ee..28b6646662af 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -356,12 +356,12 @@ __u64 read_perf_max_sample_freq(void)
 	return sample_freq;
 }
 
-static int finit_module(int fd, const char *param_values, int flags)
+int finit_module(int fd, const char *param_values, int flags)
 {
 	return syscall(__NR_finit_module, fd, param_values, flags);
 }
 
-static int delete_module(const char *name, int flags)
+int delete_module(const char *name, int flags)
 {
 	return syscall(__NR_delete_module, name, flags);
 }
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index d14de81727e6..d55f6ab12433 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -36,6 +36,8 @@ __u64 read_perf_max_sample_freq(void);
 int load_bpf_testmod(bool verbose);
 int unload_bpf_testmod(bool verbose);
 int kern_sync_rcu(void);
+int finit_module(int fd, const char *param_values, int flags);
+int delete_module(const char *name, int flags);
 
 static inline __u64 get_time_ns(void)
 {
-- 
2.34.1

Re: [PATCH bpf-next v4 2/3] bpf: Check cfi_stubs before registering a struct_ops type.

[Martin KaFai Lau]

On 2/20/24 11:52 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Recently, cfi_stubs were introduced. However, existing struct_ops types
> that are not in the upstream may not be aware of this, resulting in kernel
> crashes. By rejecting struct_ops types that do not provide cfi_stubs during
> registration, these crashes can be avoided.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   kernel/bpf/bpf_struct_ops.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 0d7be97a2411..c1c502caae08 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -302,6 +302,11 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
>   	}
>   	sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
>   
> +	if (!st_ops->cfi_stubs) {
> +		pr_warn("struct %s has no cfi_stubs\n", st_ops->name);
> +		return -EINVAL;
> +	}
> +
>   	type_id = btf_find_by_name_kind(btf, st_ops->name,
>   					BTF_KIND_STRUCT);
>   	if (type_id < 0) {
> @@ -339,6 +344,7 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
>   
>   	for_each_member(i, t, member) {
>   		const struct btf_type *func_proto;
> +		u32 moff;
>   
>   		mname = btf_name_by_offset(btf, member->name_off);
>   		if (!*mname) {
> @@ -361,6 +367,17 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
>   		if (!func_proto)
>   			continue;
>   
> +		moff = __btf_member_bit_offset(t, member) / 8;
> +		err = st_ops->check_member ?
> +			st_ops->check_member(t, member, NULL) : 0;

I don't think it is necessary to make check_member more complicated by taking
NULL prog. The struct_ops implementer then needs to handle this extra NULL
prog case.

Have you thought about Alexei's earlier suggestion in v3 to reuse the NULL
member in cfi_stubs to flag unsupported member and remove the unsupported_ops[]
from bpf_tcp_ca.c?

If the verifier can consistently reject loading unsupported bpf prog, it will
not reach the bpf_struct_ops_map_update_elem and then hits the NULL member
in cfi_stubs during map_update_elem.

Untested code:

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 011d54a1dc53..c57cb0e2a8a7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20370,6 +20370,7 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
  	u32 btf_id, member_idx;
  	struct btf *btf;
  	const char *mname;
+	u32 moff;
  
  	if (!prog->gpl_compatible) {
  		verbose(env, "struct ops programs must have a GPL compatible license\n");
@@ -20417,11 +20418,18 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
  		return -EINVAL;
  	}
  
+	moff = __btf_member_bit_offset(t, member) / 8;
+	if (!*(void **)(st_ops->cfi_stubs + moff)) {
+		verbose(env, "attach to unsupported member %s of struct %s\n",
+			mname, st_ops->name);
+		return -ENOTSUPP;
+	}
+
  	if (st_ops->check_member) {
  		int err = st_ops->check_member(t, member, prog);
  
  		if (err) {
-			verbose(env, "attach to unsupported member %s of struct %s\n",
+			verbose(env, "cannot attach to member %s of struct %s\n",
  				mname, st_ops->name);
  			return err;
  		}
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 7f518ea5f4ac..bcb1fcd00973 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -14,10 +14,6 @@
  /* "extern" is to avoid sparse warning.  It is only used in bpf_struct_ops.c. */
  static struct bpf_struct_ops bpf_tcp_congestion_ops;
  
-static u32 unsupported_ops[] = {
-	offsetof(struct tcp_congestion_ops, get_info),
-};
-
  static const struct btf_type *tcp_sock_type;
  static u32 tcp_sock_id, sock_id;
  static const struct btf_type *tcp_congestion_ops_type;
@@ -45,18 +41,6 @@ static int bpf_tcp_ca_init(struct btf *btf)
  	return 0;
  }
  
-static bool is_unsupported(u32 member_offset)
-{
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(unsupported_ops); i++) {
-		if (member_offset == unsupported_ops[i])
-			return true;
-	}
-
-	return false;
-}
-
  static bool bpf_tcp_ca_is_valid_access(int off, int size,
  				       enum bpf_access_type type,
  				       const struct bpf_prog *prog,
@@ -248,15 +232,6 @@ static int bpf_tcp_ca_init_member(const struct btf_type *t,
  	return 0;
  }
  
-static int bpf_tcp_ca_check_member(const struct btf_type *t,
-				   const struct btf_member *member,
-				   const struct bpf_prog *prog)
-{
-	if (is_unsupported(__btf_member_bit_offset(t, member) / 8))
-		return -ENOTSUPP;
-	return 0;
-}
-
  static int bpf_tcp_ca_reg(void *kdata)
  {
  	return tcp_register_congestion_control(kdata);
@@ -350,7 +325,6 @@ static struct bpf_struct_ops bpf_tcp_congestion_ops = {
  	.reg = bpf_tcp_ca_reg,
  	.unreg = bpf_tcp_ca_unreg,
  	.update = bpf_tcp_ca_update,
-	.check_member = bpf_tcp_ca_check_member,
  	.init_member = bpf_tcp_ca_init_member,
  	.init = bpf_tcp_ca_init,
  	.validate = bpf_tcp_ca_validate,
-- 
2.34.1




> +
> +		if (!err && !*(void **)(st_ops->cfi_stubs + moff)) {
> +			pr_warn("member %s in struct %s has no cfi stub function\n",
> +				mname, st_ops->name);
> +			err = -EINVAL;
> +			goto errout;
> +		}
> +
>   		if (btf_distill_func_proto(log, btf,
>   					   func_proto, mname,
>   					   &st_ops->func_models[i])) {

Re: [PATCH bpf-next v4 2/3] bpf: Check cfi_stubs before registering a struct_ops type.

[Kui-Feng Lee]

On 2/21/24 10:25, Martin KaFai Lau wrote:
> On 2/20/24 11:52 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Recently, cfi_stubs were introduced. However, existing struct_ops types
>> that are not in the upstream may not be aware of this, resulting in 
>> kernel
>> crashes. By rejecting struct_ops types that do not provide cfi_stubs 
>> during
>> registration, these crashes can be avoided.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/bpf_struct_ops.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 0d7be97a2411..c1c502caae08 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -302,6 +302,11 @@ int bpf_struct_ops_desc_init(struct 
>> bpf_struct_ops_desc *st_ops_desc,
>>       }
>>       sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
>> +    if (!st_ops->cfi_stubs) {
>> +        pr_warn("struct %s has no cfi_stubs\n", st_ops->name);
>> +        return -EINVAL;
>> +    }
>> +
>>       type_id = btf_find_by_name_kind(btf, st_ops->name,
>>                       BTF_KIND_STRUCT);
>>       if (type_id < 0) {
>> @@ -339,6 +344,7 @@ int bpf_struct_ops_desc_init(struct 
>> bpf_struct_ops_desc *st_ops_desc,
>>       for_each_member(i, t, member) {
>>           const struct btf_type *func_proto;
>> +        u32 moff;
>>           mname = btf_name_by_offset(btf, member->name_off);
>>           if (!*mname) {
>> @@ -361,6 +367,17 @@ int bpf_struct_ops_desc_init(struct 
>> bpf_struct_ops_desc *st_ops_desc,
>>           if (!func_proto)
>>               continue;
>> +        moff = __btf_member_bit_offset(t, member) / 8;
>> +        err = st_ops->check_member ?
>> +            st_ops->check_member(t, member, NULL) : 0;
> 
> I don't think it is necessary to make check_member more complicated by 
> taking
> NULL prog. The struct_ops implementer then needs to handle this extra NULL
> prog case.
> 
> Have you thought about Alexei's earlier suggestion in v3 to reuse the NULL
> member in cfi_stubs to flag unsupported member and remove the 
> unsupported_ops[]
> from bpf_tcp_ca.c?
> 
> If the verifier can consistently reject loading unsupported bpf prog, it 
> will
> not reach the bpf_struct_ops_map_update_elem and then hits the NULL member
> in cfi_stubs during map_update_elem.
> 

Ok! I misunderstood previously. I will go this way.

Re: [PATCH bpf-next v4 2/3] bpf: Check cfi_stubs before registering a struct_ops type.

[Kui-Feng Lee]

On 2/21/24 15:13, Kui-Feng Lee wrote:
> 
> 
> On 2/21/24 10:25, Martin KaFai Lau wrote:
>> On 2/20/24 11:52 PM, thinker.li@gmail.com wrote:
>>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>>
>>> Recently, cfi_stubs were introduced. However, existing struct_ops types
>>> that are not in the upstream may not be aware of this, resulting in 
>>> kernel
>>> crashes. By rejecting struct_ops types that do not provide cfi_stubs 
>>> during
>>> registration, these crashes can be avoided.
>>>
>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>> ---
>>>   kernel/bpf/bpf_struct_ops.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>> index 0d7be97a2411..c1c502caae08 100644
>>> --- a/kernel/bpf/bpf_struct_ops.c
>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>> @@ -302,6 +302,11 @@ int bpf_struct_ops_desc_init(struct 
>>> bpf_struct_ops_desc *st_ops_desc,
>>>       }
>>>       sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
>>> +    if (!st_ops->cfi_stubs) {
>>> +        pr_warn("struct %s has no cfi_stubs\n", st_ops->name);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>>       type_id = btf_find_by_name_kind(btf, st_ops->name,
>>>                       BTF_KIND_STRUCT);
>>>       if (type_id < 0) {
>>> @@ -339,6 +344,7 @@ int bpf_struct_ops_desc_init(struct 
>>> bpf_struct_ops_desc *st_ops_desc,
>>>       for_each_member(i, t, member) {
>>>           const struct btf_type *func_proto;
>>> +        u32 moff;
>>>           mname = btf_name_by_offset(btf, member->name_off);
>>>           if (!*mname) {
>>> @@ -361,6 +367,17 @@ int bpf_struct_ops_desc_init(struct 
>>> bpf_struct_ops_desc *st_ops_desc,
>>>           if (!func_proto)
>>>               continue;
>>> +        moff = __btf_member_bit_offset(t, member) / 8;
>>> +        err = st_ops->check_member ?
>>> +            st_ops->check_member(t, member, NULL) : 0;
>>
>> I don't think it is necessary to make check_member more complicated by 
>> taking
>> NULL prog. The struct_ops implementer then needs to handle this extra 
>> NULL
>> prog case.
>>
>> Have you thought about Alexei's earlier suggestion in v3 to reuse the 
>> NULL
>> member in cfi_stubs to flag unsupported member and remove the 
>> unsupported_ops[]
>> from bpf_tcp_ca.c?
>>
>> If the verifier can consistently reject loading unsupported bpf prog, 
>> it will
>> not reach the bpf_struct_ops_map_update_elem and then hits the NULL 
>> member
>> in cfi_stubs during map_update_elem.
>>
> 
> Ok! I misunderstood previously. I will go this way.
> 

According to the off-line discussion, the changes for unsupported_ops[]
should be in a separate patchset. The check of (void
**)(st_ops->cfi_stubs + moff)) will be removed. Changes of check_member
should be removed as well.