| Message ID | 20220720114652.3020467-2-asavkov@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | destructive bpf kfuncs (was: bpf_panic) | expand |
On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote: > > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program > + * will be able to perform destructive operations such as calling bpf_panic() > + * helper. > + */ > +#define BPF_F_DESTRUCTIVE (1U << 6) I don't understand what value this flag provides. bpf prog won't be using kexec accidentally. Requiring user space to also pass this flag seems pointless.
On Thu, Jul 21, 2022 at 07:02:07AM -0700, Alexei Starovoitov wrote: > On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote: > > > > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program > > + * will be able to perform destructive operations such as calling bpf_panic() > > + * helper. > > + */ > > +#define BPF_F_DESTRUCTIVE (1U << 6) > > I don't understand what value this flag provides. > > bpf prog won't be using kexec accidentally. > Requiring user space to also pass this flag seems pointless. bpf program likely won't. But I think it is not uncommon for people to run bpftrace scripts they fetched off the internet to run them without fully reading the code. So the idea was to provide intermediate tools like that with a common way to confirm user's intent without implementing their own guards around dangerous calls. If that is not a good enough of a reason to add the flag I can drop it.
On Thu, Jul 21, 2022 at 9:18 PM Artem Savkov <asavkov@redhat.com> wrote: > > On Thu, Jul 21, 2022 at 07:02:07AM -0700, Alexei Starovoitov wrote: > > On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote: > > > > > > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program > > > + * will be able to perform destructive operations such as calling bpf_panic() > > > + * helper. > > > + */ > > > +#define BPF_F_DESTRUCTIVE (1U << 6) > > > > I don't understand what value this flag provides. > > > > bpf prog won't be using kexec accidentally. > > Requiring user space to also pass this flag seems pointless. > > bpf program likely won't. But I think it is not uncommon for people to > run bpftrace scripts they fetched off the internet to run them without > fully reading the code. So the idea was to provide intermediate tools > like that with a common way to confirm user's intent without > implementing their own guards around dangerous calls. > If that is not a good enough of a reason to add the flag I can drop it. The intent makes sense, but bpftrace will set the flag silently. Since bpftrace compiles the prog it knows what helpers are being called, so it will have to pass that extra flag automatically anyway. You can argue that bpftrace needs to require a mandatory cmdline flag from users to run such scripts, but even if you convince the bpftrace community to do that everybody else might just ignore that request. Any tool (even libbpf) can scan the insns and provide flags. Long ago we added the 'kern_version' field to the prog_load command. The intent was to tie bpf prog with kernel version. Soon enough people started querying the kernel and put that version in there ignoring SEC("version") that bpf prog had. It took years to clean that up. BPF_F_DESTRUCTIVE flag looks similar to me. Good intent, but unlikely to achieve the goal. Do you have other ideas to achieve the goal: 'cannot run destructive prog by accident' ? If we had an UI it would be a question 'are you sure? please type: yes'. I hate to propose the following, since it will delay your patch for a long time, but maybe we should only allow signed bpf programs to be destructive?
On Thu, Jul 21, 2022 at 09:32:51PM -0700, Alexei Starovoitov wrote: > On Thu, Jul 21, 2022 at 9:18 PM Artem Savkov <asavkov@redhat.com> wrote: > > > > On Thu, Jul 21, 2022 at 07:02:07AM -0700, Alexei Starovoitov wrote: > > > On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote: > > > > > > > > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program > > > > + * will be able to perform destructive operations such as calling bpf_panic() > > > > + * helper. > > > > + */ > > > > +#define BPF_F_DESTRUCTIVE (1U << 6) > > > > > > I don't understand what value this flag provides. > > > > > > bpf prog won't be using kexec accidentally. > > > Requiring user space to also pass this flag seems pointless. > > > > bpf program likely won't. But I think it is not uncommon for people to > > run bpftrace scripts they fetched off the internet to run them without > > fully reading the code. So the idea was to provide intermediate tools > > like that with a common way to confirm user's intent without > > implementing their own guards around dangerous calls. > > If that is not a good enough of a reason to add the flag I can drop it. > > The intent makes sense, but bpftrace will set the flag silently. > Since bpftrace compiles the prog it knows what helpers are being > called, so it will have to pass that extra flag automatically anyway. > You can argue that bpftrace needs to require a mandatory cmdline flag > from users to run such scripts, but even if you convince the bpftrace > community to do that everybody else might just ignore that request. > Any tool (even libbpf) can scan the insns and provide flags. > > Long ago we added the 'kern_version' field to the prog_load command. > The intent was to tie bpf prog with kernel version. > Soon enough people started querying the kernel and put that > version in there ignoring SEC("version") that bpf prog had. > It took years to clean that up. > BPF_F_DESTRUCTIVE flag looks similar to me. > Good intent, but unlikely to achieve the goal. Good point, I only thought of those who would like to use this, not the ones who would try to work around it. > Do you have other ideas to achieve the goal: > 'cannot run destructive prog by accident' ? > > If we had an UI it would be a question 'are you sure? please type: yes'. > > I hate to propose the following, since it will delay your patch > for a long time, but maybe we should only allow signed bpf programs > to be destructive? Anything I can think of is likely to be as easily defeated as the flag, requirement for destructive programs to be signed is not. So I like the idea. However I think that if bpf program signature checking is disabled on the system then destructive programs should be able to run with just CAP_SYS_BOOT. So maybe we can treat everything as this case until we have the ability to sign bpf programs?
On Thu, Jul 21, 2022 at 09:32:51PM -0700, Alexei Starovoitov wrote: > On Thu, Jul 21, 2022 at 9:18 PM Artem Savkov <asavkov@redhat.com> wrote: > > > > On Thu, Jul 21, 2022 at 07:02:07AM -0700, Alexei Starovoitov wrote: > > > On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote: > > > > > > > > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program > > > > + * will be able to perform destructive operations such as calling bpf_panic() > > > > + * helper. > > > > + */ > > > > +#define BPF_F_DESTRUCTIVE (1U << 6) > > > > > > I don't understand what value this flag provides. > > > > > > bpf prog won't be using kexec accidentally. > > > Requiring user space to also pass this flag seems pointless. > > > > bpf program likely won't. But I think it is not uncommon for people to > > run bpftrace scripts they fetched off the internet to run them without > > fully reading the code. So the idea was to provide intermediate tools > > like that with a common way to confirm user's intent without > > implementing their own guards around dangerous calls. > > If that is not a good enough of a reason to add the flag I can drop it. > > The intent makes sense, but bpftrace will set the flag silently. > Since bpftrace compiles the prog it knows what helpers are being > called, so it will have to pass that extra flag automatically anyway. > You can argue that bpftrace needs to require a mandatory cmdline flag > from users to run such scripts, but even if you convince the bpftrace > community to do that everybody else might just ignore that request. > Any tool (even libbpf) can scan the insns and provide flags. FWIW I added --unsafe flag to bpftrace a while ago for situations/helpers such as these. So this load flag would work OK for bpftrace. [...] > Do you have other ideas to achieve the goal: > 'cannot run destructive prog by accident' ? > > If we had an UI it would be a question 'are you sure? please type: yes'. > > I hate to propose the following, since it will delay your patch > for a long time, but maybe we should only allow signed bpf programs > to be destructive? I don't have any opinion on the signing part but I do think it'd be nice if there was some sort of opt-in mechanism. It wouldn't be very nice if some arbitrary tracing tool panicked my machine. But I suppose tracing programs could already do some significant damage by bpf_send_signal()ing random processes. Thanks, Daniel
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index a5bf00649995e..7b404d0b80aef 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1044,6 +1044,7 @@ struct bpf_prog_aux { bool sleepable; bool tail_call_reachable; bool xdp_has_frags; + bool destructive; /* BTF_KIND_FUNC_PROTO for valid attach_btf_id */ const struct btf_type *attach_func_proto; /* function name for valid attach_btf_id */ diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 379e68fb866fc..ae81ad2e658dd 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1122,6 +1122,12 @@ enum bpf_link_type { */ #define BPF_F_XDP_HAS_FRAGS (1U << 5) +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program + * will be able to perform destructive operations such as calling bpf_panic() + * helper. + */ +#define BPF_F_DESTRUCTIVE (1U << 6) + /* link_create.kprobe_multi.flags used in LINK_CREATE command for * BPF_TRACE_KPROBE_MULTI attach type to create return probe. */ diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 83c7136c5788d..86927521d0ea2 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2467,7 +2467,8 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr) BPF_F_TEST_STATE_FREQ | BPF_F_SLEEPABLE | BPF_F_TEST_RND_HI32 | - BPF_F_XDP_HAS_FRAGS)) + BPF_F_XDP_HAS_FRAGS | + BPF_F_DESTRUCTIVE)) return -EINVAL; if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && @@ -2554,6 +2555,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr) prog->aux->offload_requested = !!attr->prog_ifindex; prog->aux->sleepable = attr->prog_flags & BPF_F_SLEEPABLE; prog->aux->xdp_has_frags = attr->prog_flags & BPF_F_XDP_HAS_FRAGS; + prog->aux->destructive = attr->prog_flags & BPF_F_DESTRUCTIVE; err = security_bpf_prog_alloc(prog->aux); if (err) diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 379e68fb866fc..ae81ad2e658dd 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -1122,6 +1122,12 @@ enum bpf_link_type { */ #define BPF_F_XDP_HAS_FRAGS (1U << 5) +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program + * will be able to perform destructive operations such as calling bpf_panic() + * helper. + */ +#define BPF_F_DESTRUCTIVE (1U << 6) + /* link_create.kprobe_multi.flags used in LINK_CREATE command for * BPF_TRACE_KPROBE_MULTI attach type to create return probe. */
Add a BPF_F_DESTRUCTIVE flag which will be required to be supplied during BPF_PROG_LOAD for programs to be able to call destructive kfuncs. Signed-off-by: Artem Savkov <asavkov@redhat.com> --- include/linux/bpf.h | 1 + include/uapi/linux/bpf.h | 6 ++++++ kernel/bpf/syscall.c | 4 +++- tools/include/uapi/linux/bpf.h | 6 ++++++ 4 files changed, 16 insertions(+), 1 deletion(-)