[PATCH bpf-next v2 0/3] destructive bpf_kfuncs

[PATCH bpf-next v2 0/3] destructive bpf_kfuncs

[Artem Savkov]

eBPF is often used for kernel debugging, and one of the widely used and
powerful debugging techniques is post-mortem debugging with a full memory dump.
Triggering a panic at exactly the right moment allows the user to get such a
dump and thus a better view at the system's state. Right now the only way to
do this in BPF is to signal userspace to trigger kexec/panic. This is
suboptimal as going through userspace requires context changes and adds
significant delays taking system further away from "the right moment". On a
single-cpu system the situation is even worse because BPF program won't even be
able to block the thread of interest.

This patchset tries to solve this problem by allowing properly marked tracing
bpf programs to call crash_kexec() kernel function. The only requirement for
now to run programs calling crash_kexec() or other destructive kfuncs is
CAP_SYS_BOOT capability. When signature checking for bpf programs is available
it is possible that stricter rules will be applied to programs utilizing
destructive kfuncs.

This is a continuation of bpf_panic patchset with initial feedback taken into
account.

Changes in v2:
 - BPF_PROG_LOAD flag dropped as it doesn't fully achieve it's aim of
   preventing accidental execution of destructive bpf programs
 - selftest moved to the end of patchset
 - switched to kfunc destructive flag instead of a separate set

Changes from RFC:
 - sysctl knob dropped
 - using crash_kexec() instead of panic()
 - using kfuncs instead of adding a new helper

Artem Savkov (3):
  bpf: add destructive kfunc flag
  bpf: export crash_kexec() as destructive kfunc
  selftests/bpf: add destructive kfunc test

 include/linux/btf.h                           |  1 +
 kernel/bpf/verifier.c                         |  5 +++
 kernel/kexec_core.c                           | 21 +++++++++++
 net/bpf/test_run.c                            |  5 +++
 .../selftests/bpf/prog_tests/kfunc_call.c     | 36 +++++++++++++++++++
 .../bpf/progs/kfunc_call_destructive.c        | 14 ++++++++
 6 files changed, 82 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/kfunc_call_destructive.c

-- 
2.35.3

[PATCH bpf-next v2 1/3] bpf: add destructive kfunc flag

[Artem Savkov]

Add KF_DESTRUCTIVE flag for destructive functions. Functions with this
flag set will require CAP_SYS_BOOT capabilities.

Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
 include/linux/btf.h   | 1 +
 kernel/bpf/verifier.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index cdb376d53238..51a0961c84e3 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -49,6 +49,7 @@
  * for this case.
  */
 #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer arguments */
+#define KF_DESTRUCTIVE  (1 << 5) /* kfunc performs destructive actions */
 
 struct btf;
 struct btf_member;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 096fdac70165..e52ca1631d3f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7584,6 +7584,11 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			func_name);
 		return -EACCES;
 	}
+	if (*kfunc_flags & KF_DESTRUCTIVE && !capable(CAP_SYS_BOOT)) {
+		verbose(env, "destructive kfunc calls require CAP_SYS_BOOT capabilities\n");
+		return -EACCES;
+	}
+
 	acq = *kfunc_flags & KF_ACQUIRE;
 
 	/* Check the arguments */
-- 
2.35.3

[PATCH bpf-next v2 2/3] bpf: export crash_kexec() as destructive kfunc

[Artem Savkov]

Allow properly marked bpf programs to call crash_kexec().

Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
 kernel/kexec_core.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 4d34c78334ce..9259ea3bd693 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -39,6 +39,8 @@
 #include <linux/hugetlb.h>
 #include <linux/objtool.h>
 #include <linux/kmsg_dump.h>
+#include <linux/btf.h>
+#include <linux/btf_ids.h>
 
 #include <asm/page.h>
 #include <asm/sections.h>
@@ -1238,3 +1240,22 @@ void __weak arch_kexec_protect_crashkres(void)
 
 void __weak arch_kexec_unprotect_crashkres(void)
 {}
+
+#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
+BTF_SET8_START(kexec_btf_ids)
+BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
+BTF_SET8_END(kexec_btf_ids)
+
+static const struct btf_kfunc_id_set kexec_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set   = &kexec_btf_ids,
+};
+
+static int __init crash_kfunc_init(void)
+{
+	register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &kexec_kfunc_set);
+	return 0;
+}
+
+subsys_initcall(crash_kfunc_init);
+#endif
-- 
2.35.3

[PATCH bpf-next v2 3/3] selftests/bpf: add destructive kfunc test

[Artem Savkov]

Add a test checking that programs calling destructive kfuncs can only do
so if they have CAP_SYS_BOOT capabilities.

Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
 net/bpf/test_run.c                            |  5 +++
 .../selftests/bpf/prog_tests/kfunc_call.c     | 36 +++++++++++++++++++
 .../bpf/progs/kfunc_call_destructive.c        | 14 ++++++++
 3 files changed, 55 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/kfunc_call_destructive.c

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index cbc9cd5058cb..afa7125252f6 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -695,6 +695,10 @@ noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
 {
 }
 
+noinline void bpf_kfunc_call_test_destructive(void)
+{
+}
+
 __diag_pop();
 
 ALLOW_ERROR_INJECTION(bpf_modify_return_test, ERRNO);
@@ -719,6 +723,7 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
 BTF_SET8_END(test_sk_check_kfunc_ids)
 
 static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
index c00eb974eb85..351fafa006fb 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -5,6 +5,9 @@
 #include "kfunc_call_test.lskel.h"
 #include "kfunc_call_test_subprog.skel.h"
 #include "kfunc_call_test_subprog.lskel.h"
+#include "kfunc_call_destructive.skel.h"
+
+#include "cap_helpers.h"
 
 static void test_main(void)
 {
@@ -86,6 +89,36 @@ static void test_subprog_lskel(void)
 	kfunc_call_test_subprog_lskel__destroy(skel);
 }
 
+static int test_destructive_open_and_load(void)
+{
+	struct kfunc_call_destructive *skel;
+	int err;
+
+	skel = kfunc_call_destructive__open();
+	if (!ASSERT_OK_PTR(skel, "prog_open"))
+		return -1;
+
+	err = kfunc_call_destructive__load(skel);
+
+	kfunc_call_destructive__destroy(skel);
+
+	return err;
+}
+
+static void test_destructive(void)
+{
+	__u64 save_caps = 0;
+
+	ASSERT_OK(test_destructive_open_and_load(), "succesful_load");
+
+	if (!ASSERT_OK(cap_disable_effective(1ULL << CAP_SYS_BOOT, &save_caps), "drop_caps"))
+		return;
+
+	ASSERT_EQ(test_destructive_open_and_load(), -13, "no_caps_failure");
+
+	cap_enable_effective(save_caps, NULL);
+}
+
 void test_kfunc_call(void)
 {
 	if (test__start_subtest("main"))
@@ -96,4 +129,7 @@ void test_kfunc_call(void)
 
 	if (test__start_subtest("subprog_lskel"))
 		test_subprog_lskel();
+
+	if (test__start_subtest("destructive"))
+		test_destructive();
 }
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
new file mode 100644
index 000000000000..767472bc5a97
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+
+extern void bpf_kfunc_call_test_destructive(void) __ksym;
+
+SEC("tc")
+int kfunc_destructive_test(void)
+{
+	bpf_kfunc_call_test_destructive();
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.35.3

Re: [PATCH bpf-next v2 2/3] bpf: export crash_kexec() as destructive kfunc

[Jiri Olsa]

On Tue, Aug 02, 2022 at 11:10:29AM +0200, Artem Savkov wrote:
> Allow properly marked bpf programs to call crash_kexec().
> 
> Signed-off-by: Artem Savkov <asavkov@redhat.com>
> ---
>  kernel/kexec_core.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 4d34c78334ce..9259ea3bd693 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -39,6 +39,8 @@
>  #include <linux/hugetlb.h>
>  #include <linux/objtool.h>
>  #include <linux/kmsg_dump.h>
> +#include <linux/btf.h>
> +#include <linux/btf_ids.h>
>  
>  #include <asm/page.h>
>  #include <asm/sections.h>
> @@ -1238,3 +1240,22 @@ void __weak arch_kexec_protect_crashkres(void)
>  
>  void __weak arch_kexec_unprotect_crashkres(void)
>  {}
> +
> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> +BTF_SET8_START(kexec_btf_ids)
> +BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
> +BTF_SET8_END(kexec_btf_ids)
> +
> +static const struct btf_kfunc_id_set kexec_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set   = &kexec_btf_ids,
> +};
> +
> +static int __init crash_kfunc_init(void)
> +{
> +	register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &kexec_kfunc_set);
> +	return 0;

should we do 'return register_btf_kfunc_id_set(...' in here?

jirka

> +}
> +
> +subsys_initcall(crash_kfunc_init);
> +#endif
> -- 
> 2.35.3
> 

Re: [PATCH bpf-next v2 2/3] bpf: export crash_kexec() as destructive kfunc

[Artem Savkov]

On Tue, Aug 02, 2022 at 12:46:42PM +0200, Jiri Olsa wrote:
> On Tue, Aug 02, 2022 at 11:10:29AM +0200, Artem Savkov wrote:
> > +static int __init crash_kfunc_init(void)
> > +{
> > +	register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &kexec_kfunc_set);
> > +	return 0;
> 
> should we do 'return register_btf_kfunc_id_set(...' in here?

Maybe, but as far as I can tell the return value for init calls does
absolutely nothing except for showing up in a debug message. So I don't
think it will be worth a respin, but if there is one anyway I'll change
this.

-- 
 Artem

Re: [PATCH bpf-next v2 2/3] bpf: export crash_kexec() as destructive kfunc

[Alexei Starovoitov]

On Tue, Aug 2, 2022 at 2:10 AM Artem Savkov <asavkov@redhat.com> wrote:
>
> Allow properly marked bpf programs to call crash_kexec().
>
> Signed-off-by: Artem Savkov <asavkov@redhat.com>
> ---
>  kernel/kexec_core.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 4d34c78334ce..9259ea3bd693 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -39,6 +39,8 @@
>  #include <linux/hugetlb.h>
>  #include <linux/objtool.h>
>  #include <linux/kmsg_dump.h>
> +#include <linux/btf.h>
> +#include <linux/btf_ids.h>
>
>  #include <asm/page.h>
>  #include <asm/sections.h>
> @@ -1238,3 +1240,22 @@ void __weak arch_kexec_protect_crashkres(void)
>
>  void __weak arch_kexec_unprotect_crashkres(void)
>  {}
> +
> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> +BTF_SET8_START(kexec_btf_ids)
> +BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
> +BTF_SET8_END(kexec_btf_ids)
> +
> +static const struct btf_kfunc_id_set kexec_kfunc_set = {
> +       .owner = THIS_MODULE,
> +       .set   = &kexec_btf_ids,
> +};
> +
> +static int __init crash_kfunc_init(void)
> +{
> +       register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &kexec_kfunc_set);
> +       return 0;
> +}
> +
> +subsys_initcall(crash_kfunc_init);
> +#endif

It feels there will be a bunch of such boiler plate code
in different .c files in many places in the kernel
if we go with this approach.

Maybe we should do one call:
register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING
from kernel/bpf/helper.c
to register all tracing kfuncs ?

And gate
BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
with #ifdef CONFIG_KEXEC_CORE.

We have such a pattern in verifier.c already.

Re: [PATCH bpf-next v2 2/3] bpf: export crash_kexec() as destructive kfunc

[Artem Savkov]

On Thu, Aug 04, 2022 at 01:41:53PM -0700, Alexei Starovoitov wrote:
> On Tue, Aug 2, 2022 at 2:10 AM Artem Savkov <asavkov@redhat.com> wrote:
> >
> > Allow properly marked bpf programs to call crash_kexec().
> >
> > Signed-off-by: Artem Savkov <asavkov@redhat.com>
> > ---
> >  kernel/kexec_core.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index 4d34c78334ce..9259ea3bd693 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -39,6 +39,8 @@
> >  #include <linux/hugetlb.h>
> >  #include <linux/objtool.h>
> >  #include <linux/kmsg_dump.h>
> > +#include <linux/btf.h>
> > +#include <linux/btf_ids.h>
> >
> >  #include <asm/page.h>
> >  #include <asm/sections.h>
> > @@ -1238,3 +1240,22 @@ void __weak arch_kexec_protect_crashkres(void)
> >
> >  void __weak arch_kexec_unprotect_crashkres(void)
> >  {}
> > +
> > +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> > +BTF_SET8_START(kexec_btf_ids)
> > +BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
> > +BTF_SET8_END(kexec_btf_ids)
> > +
> > +static const struct btf_kfunc_id_set kexec_kfunc_set = {
> > +       .owner = THIS_MODULE,
> > +       .set   = &kexec_btf_ids,
> > +};
> > +
> > +static int __init crash_kfunc_init(void)
> > +{
> > +       register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &kexec_kfunc_set);
> > +       return 0;
> > +}
> > +
> > +subsys_initcall(crash_kfunc_init);
> > +#endif
> 
> It feels there will be a bunch of such boiler plate code
> in different .c files in many places in the kernel
> if we go with this approach.
> 
> Maybe we should do one call:
> register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING
> from kernel/bpf/helper.c
> to register all tracing kfuncs ?
> 
> And gate
> BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
> with #ifdef CONFIG_KEXEC_CORE.
> 
> We have such a pattern in verifier.c already.

Good idea, thanks for the pointers. I'll do that in next version.

-- 
 Artem