* [PATCH bpf 1/2] bpf: Restrict bpf_sys_bpf to CAP_PERFMON @ 2022-08-16 20:55 YiFei Zhu 2022-08-16 20:55 ` [PATCH bpf 2/2] bpf: Add WARN_ON for recursive prog_run invocation YiFei Zhu 2022-08-17 22:30 ` [PATCH bpf 1/2] bpf: Restrict bpf_sys_bpf to CAP_PERFMON patchwork-bot+netdevbpf 0 siblings, 2 replies; 5+ messages in thread From: YiFei Zhu @ 2022-08-16 20:55 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Jinghao Jia, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, Song Liu, Stanislav Fomichev, Jason Zhang, Jann Horn, mvle, zohar, tyxu.uiuc, security The verifier cannot perform sufficient validation of any pointers passed into bpf_attr and treats them as integers rather than pointers. The helper will then read from arbitrary pointers passed into it. Restrict the helper to CAP_PERFMON since the security model in BPF of arbitrary kernel read is CAP_BPF + CAP_PERFMON. Fixes: af2ac3e13e45 ("bpf: Prepare bpf syscall to be used from kernel and user space.") Signed-off-by: YiFei Zhu <zhuyifei@google.com> --- kernel/bpf/syscall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index a4d40d98428a..27760627370d 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -5197,7 +5197,7 @@ syscall_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) { switch (func_id) { case BPF_FUNC_sys_bpf: - return &bpf_sys_bpf_proto; + return !perfmon_capable() ? NULL : &bpf_sys_bpf_proto; case BPF_FUNC_btf_find_by_name_kind: return &bpf_btf_find_by_name_kind_proto; case BPF_FUNC_sys_close: -- 2.37.1.595.g718a3a8f04-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH bpf 2/2] bpf: Add WARN_ON for recursive prog_run invocation 2022-08-16 20:55 [PATCH bpf 1/2] bpf: Restrict bpf_sys_bpf to CAP_PERFMON YiFei Zhu @ 2022-08-16 20:55 ` YiFei Zhu 2022-08-17 22:30 ` Daniel Borkmann 2022-08-17 22:30 ` [PATCH bpf 1/2] bpf: Restrict bpf_sys_bpf to CAP_PERFMON patchwork-bot+netdevbpf 1 sibling, 1 reply; 5+ messages in thread From: YiFei Zhu @ 2022-08-16 20:55 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Jinghao Jia, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, Song Liu, Stanislav Fomichev, Jason Zhang, Jann Horn, mvle, zohar, tyxu.uiuc, security Recursive invocation should not happen after commit 86f44fcec22c ("bpf: Disallow bpf programs call prog_run command."), unlike what is suggested in the comment. The only way to I can see this condition trigger is if userspace fetches an fd of a kernel-loaded lskel and attempt to race the kernel to execute that lskel... which also shouldn't happen under normal circumstances. To make this "should never happen" explicit, clarify this in the comment and add a WARN_ON. Fixes: 86f44fcec22c ("bpf: Disallow bpf programs call prog_run command.") Signed-off-by: YiFei Zhu <zhuyifei@google.com> --- kernel/bpf/syscall.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 27760627370d..9cac9402c0bf 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -5119,8 +5119,8 @@ int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size) run_ctx.bpf_cookie = 0; run_ctx.saved_run_ctx = NULL; - if (!__bpf_prog_enter_sleepable(prog, &run_ctx)) { - /* recursion detected */ + if (WARN_ON(!__bpf_prog_enter_sleepable(prog, &run_ctx))) { + /* recursion detected, should never happen */ bpf_prog_put(prog); return -EBUSY; } -- 2.37.1.595.g718a3a8f04-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf 2/2] bpf: Add WARN_ON for recursive prog_run invocation 2022-08-16 20:55 ` [PATCH bpf 2/2] bpf: Add WARN_ON for recursive prog_run invocation YiFei Zhu @ 2022-08-17 22:30 ` Daniel Borkmann 2022-08-17 23:20 ` YiFei Zhu 0 siblings, 1 reply; 5+ messages in thread From: Daniel Borkmann @ 2022-08-17 22:30 UTC (permalink / raw) To: YiFei Zhu, bpf Cc: Alexei Starovoitov, Jinghao Jia, Alexei Starovoitov, Andrii Nakryiko, Song Liu, Stanislav Fomichev, Jason Zhang, Jann Horn, mvle, zohar, tyxu.uiuc, security On 8/16/22 10:55 PM, YiFei Zhu wrote: > Recursive invocation should not happen after commit 86f44fcec22c > ("bpf: Disallow bpf programs call prog_run command."), unlike what > is suggested in the comment. The only way to I can see this > condition trigger is if userspace fetches an fd of a kernel-loaded > lskel and attempt to race the kernel to execute that lskel... which > also shouldn't happen under normal circumstances. > > To make this "should never happen" explicit, clarify this in the > comment and add a WARN_ON. > > Fixes: 86f44fcec22c ("bpf: Disallow bpf programs call prog_run command.") > Signed-off-by: YiFei Zhu <zhuyifei@google.com> > --- > kernel/bpf/syscall.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 27760627370d..9cac9402c0bf 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -5119,8 +5119,8 @@ int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size) > > run_ctx.bpf_cookie = 0; > run_ctx.saved_run_ctx = NULL; > - if (!__bpf_prog_enter_sleepable(prog, &run_ctx)) { > - /* recursion detected */ > + if (WARN_ON(!__bpf_prog_enter_sleepable(prog, &run_ctx))) { > + /* recursion detected, should never happen */ Pushed out commit 1/2, thanks! I think this one causes more confusion than value, imho, for example in your commit log you state that it could trigger when attempting to race, in the comment you state "should never happen". Which one is it? Also, if we can recover gracefully in this case, what should the user do with the warn (I guess worst case warn_on_once), but still? Thanks, Daniel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf 2/2] bpf: Add WARN_ON for recursive prog_run invocation 2022-08-17 22:30 ` Daniel Borkmann @ 2022-08-17 23:20 ` YiFei Zhu 0 siblings, 0 replies; 5+ messages in thread From: YiFei Zhu @ 2022-08-17 23:20 UTC (permalink / raw) To: Daniel Borkmann Cc: bpf, Alexei Starovoitov, Jinghao Jia, Alexei Starovoitov, Andrii Nakryiko, Song Liu, Stanislav Fomichev, Jason Zhang, Jann Horn, mvle, zohar, tyxu.uiuc, security On Wed, Aug 17, 2022 at 3:30 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 8/16/22 10:55 PM, YiFei Zhu wrote: > > Recursive invocation should not happen after commit 86f44fcec22c > > ("bpf: Disallow bpf programs call prog_run command."), unlike what > > is suggested in the comment. The only way to I can see this > > condition trigger is if userspace fetches an fd of a kernel-loaded > > lskel and attempt to race the kernel to execute that lskel... which > > also shouldn't happen under normal circumstances. > > > > To make this "should never happen" explicit, clarify this in the > > comment and add a WARN_ON. > > > > Fixes: 86f44fcec22c ("bpf: Disallow bpf programs call prog_run command.") > > Signed-off-by: YiFei Zhu <zhuyifei@google.com> > > --- > > kernel/bpf/syscall.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 27760627370d..9cac9402c0bf 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -5119,8 +5119,8 @@ int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size) > > > > run_ctx.bpf_cookie = 0; > > run_ctx.saved_run_ctx = NULL; > > - if (!__bpf_prog_enter_sleepable(prog, &run_ctx)) { > > - /* recursion detected */ > > + if (WARN_ON(!__bpf_prog_enter_sleepable(prog, &run_ctx))) { > > + /* recursion detected, should never happen */ > > Pushed out commit 1/2, thanks! I think this one causes more confusion than value, > imho, for example in your commit log you state that it could trigger when attempting > to race, in the comment you state "should never happen". Which one is it? Also, if > we can recover gracefully in this case, what should the user do with the warn (I > guess worst case warn_on_once), but still? I mean, why would anyone attempt to race this... smells more like an exploitation attempt (though realistically only possible by someone with root). I see the original comment talks about a BPF SYSCALL prog invoking, directly or indirectly, itself (hence the "recursion detected"), and this should be no longer possible at all after 86f44fcec22c. As for "what should the user do with the warn", the direct result of this is a module load failure, considering kern_sys_bpf is only used by kernel lskels AFAIK. However, since the lskel loader has been executed, the lskel loader may have successfully loaded whatever it would load anyways, despite the apparent error code from module load, depending on what stage in the lskel the race happened. I'm not sure what the user should really do in this circumstance, but I think something should be logged at least to tell the user something really wrong is going on. I'm considering BUG_ON in this case but I think that does more harm than good (since that'd kill the process in the middle of a module load). Please correct me if I'm wrong. This is assuming the kernel is working as it is expected to, no memory corruption shenanigans. YiFei Zhu > Thanks, > Daniel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf 1/2] bpf: Restrict bpf_sys_bpf to CAP_PERFMON 2022-08-16 20:55 [PATCH bpf 1/2] bpf: Restrict bpf_sys_bpf to CAP_PERFMON YiFei Zhu 2022-08-16 20:55 ` [PATCH bpf 2/2] bpf: Add WARN_ON for recursive prog_run invocation YiFei Zhu @ 2022-08-17 22:30 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 5+ messages in thread From: patchwork-bot+netdevbpf @ 2022-08-17 22:30 UTC (permalink / raw) To: YiFei Zhu Cc: bpf, alexei.starovoitov, jinghao7, daniel, ast, andrii, song, sdf, jdz, jannh, mvle, zohar, tyxu.uiuc, security Hello: This series was applied to bpf/bpf.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Tue, 16 Aug 2022 20:55:16 +0000 you wrote: > The verifier cannot perform sufficient validation of any pointers > passed into bpf_attr and treats them as integers rather than pointers. > The helper will then read from arbitrary pointers passed into it. > Restrict the helper to CAP_PERFMON since the security model in > BPF of arbitrary kernel read is CAP_BPF + CAP_PERFMON. > > Fixes: af2ac3e13e45 ("bpf: Prepare bpf syscall to be used from kernel and user space.") > Signed-off-by: YiFei Zhu <zhuyifei@google.com> > > [...] Here is the summary with links: - [bpf,1/2] bpf: Restrict bpf_sys_bpf to CAP_PERFMON https://git.kernel.org/bpf/bpf/c/14b20b784f59 - [bpf,2/2] bpf: Add WARN_ON for recursive prog_run invocation (no matching commit) You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-17 23:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-16 20:55 [PATCH bpf 1/2] bpf: Restrict bpf_sys_bpf to CAP_PERFMON YiFei Zhu 2022-08-16 20:55 ` [PATCH bpf 2/2] bpf: Add WARN_ON for recursive prog_run invocation YiFei Zhu 2022-08-17 22:30 ` Daniel Borkmann 2022-08-17 23:20 ` YiFei Zhu 2022-08-17 22:30 ` [PATCH bpf 1/2] bpf: Restrict bpf_sys_bpf to CAP_PERFMON patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).