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

[PATCH bpf-next v5 0/2] 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 v4:

 - Remove changes of check_member.

 - Remove checks of the pointers in cfi_stubs[].

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.

v4: https://lore.kernel.org/all/20240221075213.2071454-1-thinker.li@gmail.com/
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 (2):
  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                   |  5 ++
 tools/testing/selftests/bpf/Makefile          | 10 ++-
 .../selftests/bpf/bpf_test_no_cfi/Makefile    | 19 +++++
 .../bpf/bpf_test_no_cfi/bpf_test_no_cfi.c     | 84 +++++++++++++++++++
 .../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 +
 7 files changed, 159 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

-- 
2.34.1

[PATCH bpf-next v5 1/2] 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 0d7be97a2411..796cec701708 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) {
-- 
2.34.1

[PATCH bpf-next v5 2/2] 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 cif_stubs of NULL and non-NULL value.
The NULL one should fail, and the non-NULL one should success. 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     | 84 +++++++++++++++++++
 .../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, 154 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 9be69ff701ba..84cb5500e8ef 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
@@ -631,6 +637,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				\
@@ -759,6 +766,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..7a2c8c99987f
--- /dev/null
+++ b/tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c
@@ -0,0 +1,84 @@
+// 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 = {
+	.fn_1 = bpf_test_no_cfi_ops__fn_1,
+	.fn_2 = bpf_test_no_cfi_ops__fn_2,
+};
+
+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);
+	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..f16d4dcccacf
--- /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 without
+	 *  cfi_stubs and with cfi_stubs.
+	 *
+	 * The one without cfi_stub should fail. The module will be loaded
+	 * successfully only 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 v5 2/2] selftests/bpf: Test case for lacking CFI stub functions.

[Martin KaFai Lau]

On 2/21/24 6:11 PM, thinker.li@gmail.com wrote:
> +static struct bpf_test_no_cfi_ops __bpf_test_no_cfi_ops = {
> +	.fn_1 = bpf_test_no_cfi_ops__fn_1,
> +	.fn_2 = bpf_test_no_cfi_ops__fn_2,
> +};
> +
> +static struct bpf_struct_ops bpf_bpf_test_no_cif_ops = {

nit. I shortened this to 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);
> +	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..f16d4dcccacf
> --- /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);

Removed close(fd) here. Also fixed the above ASSERT_GT test. Applied.

The patchwork has picked up the submitter name as "Thinker Lee". I fixed that up 
this time for the cover letter (the merge commit). Not sure where it came from 
and could be due to the nameless sender "from" in the email.

Please write the full name when sending out patch, e.g. "git send-email 
--from="Kui-Feng Lee <thinker.li@gmail.com>" ..."



> +		return;
> +	}
> +
> +	/* The module will try to register a struct_ops type without
> +	 *  cfi_stubs and with cfi_stubs.
> +	 *
> +	 * The one without cfi_stub should fail. The module will be loaded
> +	 * successfully only 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");
> +}
> +

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Wed, 21 Feb 2024 18:11:03 -0800 you 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 properly during registration, these crashes can be avoided.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v5,1/2] bpf: Check cfi_stubs before registering a struct_ops type.
    https://git.kernel.org/bpf/bpf-next/c/3e0008336ae3
  - [bpf-next,v5,2/2] selftests/bpf: Test case for lacking CFI stub functions.
    https://git.kernel.org/bpf/bpf-next/c/e9bbda13a7b8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html