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 v3:
- moved kfunc set registration to kernel/bpf/helpers.c
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/helpers.c | 21 +++++++++++
kernel/bpf/verifier.c | 5 +++
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.37.1
Add KF_DESTRUCTIVE flag for destructive functions. Functions with this
flag set will require CAP_SYS_BOOT capabilities.
Signed-off-by: Artem Savkov <[email protected]>
---
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.37.1
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 <[email protected]>
---
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.37.1
On Mon, Aug 08, 2022 at 02:14:33PM +0200, Kumar Kartikeya Dwivedi wrote:
> On Mon, 8 Aug 2022 at 11:48, Artem Savkov <[email protected]> wrote:
> >
> > Add KF_DESTRUCTIVE flag for destructive functions. Functions with this
> > flag set will require CAP_SYS_BOOT capabilities.
> >
> > Signed-off-by: Artem Savkov <[email protected]>
> > ---
> > 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 */
> >
>
> Please also document this flag in Documentation/bpf/kfuncs.rst.
Ok, will do.
> And maybe instead of KF_DESTRUCTIVE, it might be more apt to call this
> KF_CAP_SYS_BOOT. While it is true you had a destructive flag for
> programs being loaded earlier, so there was a mapping between the two
> UAPI and kfunc flags, what it has boiled down to is that this flag
> just requires CAP_SYS_BOOT (in addition to other capabilities) during
> load. So that name might express the intent a bit better. We might
> soon have similar flags encoding requirements of other capabilities on
> load.
>
> The flag rename is just a suggestion, up to you.
This makes sense right now, but if going forward we'll add stricter
signing requirements or other prerequisites we'll either have to rename
the flag back, or add those as separate flags. I guess the decision here
depends on whether some of non-destructive bpf programs might ever require
CAP_SYS_BOOT capabilities or not.
--
Artem
On Mon, 8 Aug 2022 at 11:48, Artem Savkov <[email protected]> wrote:
>
> Add KF_DESTRUCTIVE flag for destructive functions. Functions with this
> flag set will require CAP_SYS_BOOT capabilities.
>
> Signed-off-by: Artem Savkov <[email protected]>
> ---
> 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 */
>
Please also document this flag in Documentation/bpf/kfuncs.rst.
And maybe instead of KF_DESTRUCTIVE, it might be more apt to call this
KF_CAP_SYS_BOOT. While it is true you had a destructive flag for
programs being loaded earlier, so there was a mapping between the two
UAPI and kfunc flags, what it has boiled down to is that this flag
just requires CAP_SYS_BOOT (in addition to other capabilities) during
load. So that name might express the intent a bit better. We might
soon have similar flags encoding requirements of other capabilities on
load.
The flag rename is just a suggestion, up to you.
> 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.37.1
>
On Mon, 8 Aug 2022 at 14:41, Artem Savkov <[email protected]> wrote:
>
> On Mon, Aug 08, 2022 at 02:14:33PM +0200, Kumar Kartikeya Dwivedi wrote:
> > On Mon, 8 Aug 2022 at 11:48, Artem Savkov <[email protected]> wrote:
> > >
> > > Add KF_DESTRUCTIVE flag for destructive functions. Functions with this
> > > flag set will require CAP_SYS_BOOT capabilities.
> > >
> > > Signed-off-by: Artem Savkov <[email protected]>
> > > ---
> > > 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 */
> > >
> >
> > Please also document this flag in Documentation/bpf/kfuncs.rst.
>
> Ok, will do.
>
> > And maybe instead of KF_DESTRUCTIVE, it might be more apt to call this
> > KF_CAP_SYS_BOOT. While it is true you had a destructive flag for
> > programs being loaded earlier, so there was a mapping between the two
> > UAPI and kfunc flags, what it has boiled down to is that this flag
> > just requires CAP_SYS_BOOT (in addition to other capabilities) during
> > load. So that name might express the intent a bit better. We might
> > soon have similar flags encoding requirements of other capabilities on
> > load.
> >
> > The flag rename is just a suggestion, up to you.
>
> This makes sense right now, but if going forward we'll add stricter
> signing requirements or other prerequisites we'll either have to rename
> the flag back, or add those as separate flags. I guess the decision here
IMO we should do that when the time comes, for now it should reflect
the current state.
To me this helper requiring cap_sys_boot is just like how some
existing stable helpers are gated behind bpf_capable or
perfmon_capable.
When it requires that the program calling it be signed, we can revisit this.
> depends on whether some of non-destructive bpf programs might ever require
> CAP_SYS_BOOT capabilities or not.
These are just internal kernel flags, so refactoring/renaming is not a
big deal when it is needed. E.g. we've changed just how kfuncs are
registered twice since the support was added not long ago :).
>
> --
> Artem
>
On Mon, Aug 8, 2022 at 6:33 AM Kumar Kartikeya Dwivedi <[email protected]> wrote:
>
> On Mon, 8 Aug 2022 at 14:41, Artem Savkov <[email protected]> wrote:
> >
> > On Mon, Aug 08, 2022 at 02:14:33PM +0200, Kumar Kartikeya Dwivedi wrote:
> > > On Mon, 8 Aug 2022 at 11:48, Artem Savkov <[email protected]> wrote:
> > > >
> > > > Add KF_DESTRUCTIVE flag for destructive functions. Functions with this
> > > > flag set will require CAP_SYS_BOOT capabilities.
> > > >
> > > > Signed-off-by: Artem Savkov <[email protected]>
> > > > ---
> > > > 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 */
> > > >
> > >
> > > Please also document this flag in Documentation/bpf/kfuncs.rst.
> >
> > Ok, will do.
> >
> > > And maybe instead of KF_DESTRUCTIVE, it might be more apt to call this
> > > KF_CAP_SYS_BOOT. While it is true you had a destructive flag for
> > > programs being loaded earlier, so there was a mapping between the two
> > > UAPI and kfunc flags, what it has boiled down to is that this flag
> > > just requires CAP_SYS_BOOT (in addition to other capabilities) during
> > > load. So that name might express the intent a bit better. We might
> > > soon have similar flags encoding requirements of other capabilities on
> > > load.
> > >
> > > The flag rename is just a suggestion, up to you.
> >
> > This makes sense right now, but if going forward we'll add stricter
> > signing requirements or other prerequisites we'll either have to rename
> > the flag back, or add those as separate flags. I guess the decision here
>
> IMO we should do that when the time comes, for now it should reflect
> the current state.
But names should be also semantically meaningful, so KF_DESTRUCTIVE
explains that kfunc can do destructive operations, which is better
than just declaring that kfunc needs CAP_SYS_BOOT, as the latter is
current implementation detail which has no bearing on kfunc definition
itself.
Unless we anticipate we'll have another "destructive" kfunc not using
KF_DESTRUCTIVE and instead we'll add some other
KF_CAP_SYS_WHATEVERELSE?
> To me this helper requiring cap_sys_boot is just like how some
> existing stable helpers are gated behind bpf_capable or
> perfmon_capable.
> When it requires that the program calling it be signed, we can revisit this.
>
> > depends on whether some of non-destructive bpf programs might ever require
> > CAP_SYS_BOOT capabilities or not.
>
> These are just internal kernel flags, so refactoring/renaming is not a
> big deal when it is needed. E.g. we've changed just how kfuncs are
> registered twice since the support was added not long ago :).
>
> >
> > --
> > Artem
> >
On Tue, 9 Aug 2022 at 02:37, Andrii Nakryiko <[email protected]> wrote:
>
> On Mon, Aug 8, 2022 at 6:33 AM Kumar Kartikeya Dwivedi <[email protected]> wrote:
> >
> > On Mon, 8 Aug 2022 at 14:41, Artem Savkov <[email protected]> wrote:
> > >
> > > On Mon, Aug 08, 2022 at 02:14:33PM +0200, Kumar Kartikeya Dwivedi wrote:
> > > > On Mon, 8 Aug 2022 at 11:48, Artem Savkov <[email protected]> wrote:
> > > > >
> > > > > Add KF_DESTRUCTIVE flag for destructive functions. Functions with this
> > > > > flag set will require CAP_SYS_BOOT capabilities.
> > > > >
> > > > > Signed-off-by: Artem Savkov <[email protected]>
> > > > > ---
> > > > > 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 */
> > > > >
> > > >
> > > > Please also document this flag in Documentation/bpf/kfuncs.rst.
> > >
> > > Ok, will do.
> > >
> > > > And maybe instead of KF_DESTRUCTIVE, it might be more apt to call this
> > > > KF_CAP_SYS_BOOT. While it is true you had a destructive flag for
> > > > programs being loaded earlier, so there was a mapping between the two
> > > > UAPI and kfunc flags, what it has boiled down to is that this flag
> > > > just requires CAP_SYS_BOOT (in addition to other capabilities) during
> > > > load. So that name might express the intent a bit better. We might
> > > > soon have similar flags encoding requirements of other capabilities on
> > > > load.
> > > >
> > > > The flag rename is just a suggestion, up to you.
> > >
> > > This makes sense right now, but if going forward we'll add stricter
> > > signing requirements or other prerequisites we'll either have to rename
> > > the flag back, or add those as separate flags. I guess the decision here
> >
> > IMO we should do that when the time comes, for now it should reflect
> > the current state.
>
> But names should be also semantically meaningful, so KF_DESTRUCTIVE
> explains that kfunc can do destructive operations, which is better
> than just declaring that kfunc needs CAP_SYS_BOOT, as the latter is
> current implementation detail which has no bearing on kfunc definition
> itself.
>
> Unless we anticipate we'll have another "destructive" kfunc not using
> KF_DESTRUCTIVE and instead we'll add some other
> KF_CAP_SYS_WHATEVERELSE?
>
I just found it a bit odd that KF_DESTRUCTIVE would require
CAP_SYS_BOOT. When thinking about what one would write in the docs:
just that KF_DESTRUCTIVE kfuncs can do destructive operations? That
doesn't really capture what the flag ends up doing to the kfunc (it
limits use to those who have a certain cap on program load). There can
be several destructive operations (e.g. a frequently mentioned socket
kill helper that may be considered equally destructive for some
workload) but would probably require CAP_NET_ADMIN instead.
But anyway, I didn't really want to bikeshed over this :), we can give
it a better name next time something like this is added, and just go
with KF_DESTRUCTIVE for now.
> > To me this helper requiring cap_sys_boot is just like how some
> > existing stable helpers are gated behind bpf_capable or
> > perfmon_capable.
> > When it requires that the program calling it be signed, we can revisit this.
> >
> > > depends on whether some of non-destructive bpf programs might ever require
> > > CAP_SYS_BOOT capabilities or not.
> >
> > These are just internal kernel flags, so refactoring/renaming is not a
> > big deal when it is needed. E.g. we've changed just how kfuncs are
> > registered twice since the support was added not long ago :).
> >
> > >
> > > --
> > > Artem
> > >