Re: [PATCH bpf 2/2] bpf: Add WARN_ON for recursive prog_run invocation

[YiFei Zhu <zhuyifei@google.com>]

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