Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230510122045.2259-1-zegao@tencent.com> <6308b8e0-8a54-e574-a312-0a97cfbf810c@meta.com>
In-Reply-To: <6308b8e0-8a54-e574-a312-0a97cfbf810c@meta.com>
From:   Ze Gao <zegao2021@gmail.com>
Date:   Thu, 11 May 2023 09:06:14 +0800
Message-ID: <CAD8CoPBBipQWP0xT5unAshL93OtXLQox_rz0f7rfrGgy05UVJg@mail.gmail.com>
Subject: Re: [PATCH] bpf: reject blacklisted symbols in kprobe_multi to avoid
 recursive trap
To:     Yonghong Song <yhs@meta.com>
Cc:     Song Liu <song@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Ze Gao <zegao@tencent.com>, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

I'm afraid filtering in user space tools is not enough, cause it's a kernel=
 BUG.

it would 100% trigger a kernel crash if you run cmd like

retsnoop -e 'pick_next_task_fair' -a ':kernel/sched/*.c' -vvv

which is caused by that BPF_LINK_TYPE_KPROBE_MULTI accidentally
attaches bpf progs
to preempt_count_{add, sub}, which in turn triggers stackoverflow
because the handler itself
calls those functions.

checking in libbpf is not enough if some tools use bpf syscall
directly, you know,
we can not cover all cases.

So kernel checking is a must, we cannot rely on users to not crash the kern=
el.

Thanks
Ze

On Wed, May 10, 2023 at 10:14=E2=80=AFPM Yonghong Song <yhs@meta.com> wrote=
:
>
>
>
> On 5/10/23 5:20 AM, Ze Gao wrote:
> > BPF_LINK_TYPE_KPROBE_MULTI attaches kprobe programs through fprobe,
> > however it does not takes those kprobe blacklisted into consideration,
> > which likely introduce recursive traps and blows up stacks.
> >
> > this patch adds simple check and remove those are in kprobe_blacklist
> > from one fprobe during bpf_kprobe_multi_link_attach. And also
> > check_kprobe_address_safe is open for more future checks.
> >
> > note that ftrace provides recursion detection mechanism, but for kprobe
> > only, we can directly reject those cases early without turning to ftrac=
e.
> >
> > Signed-off-by: Ze Gao <zegao@tencent.com>
> > ---
> >   kernel/trace/bpf_trace.c | 37 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 37 insertions(+)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 9a050e36dc6c..44c68bc06bbd 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2764,6 +2764,37 @@ static int get_modules_for_addrs(struct module *=
**mods, unsigned long *addrs, u3
> >       return arr.mods_cnt;
> >   }
> >
> > +static inline int check_kprobe_address_safe(unsigned long addr)
> > +{
> > +     if (within_kprobe_blacklist(addr))
> > +             return -EINVAL;
> > +     else
> > +             return 0;
> > +}
> > +
> > +static int check_bpf_kprobe_addrs_safe(unsigned long *addrs, int num)
> > +{
> > +     int i, cnt;
> > +     char symname[KSYM_NAME_LEN];
> > +
> > +     for (i =3D 0; i < num; ++i) {
> > +             if (check_kprobe_address_safe((unsigned long)addrs[i])) {
> > +                     lookup_symbol_name(addrs[i], symname);
> > +                     pr_warn("bpf_kprobe: %s at %lx is blacklisted\n",=
 symname, addrs[i]);
>
> So user request cannot be fulfilled and a warning is issued and some
> of user requests are discarded and the rest is proceeded. Does not
> sound a good idea.
>
> Maybe we should do filtering in user space, e.g., in libbpf, check
> /sys/kernel/debug/kprobes/blacklist and return error
> earlier? bpftrace/libbpf-tools/bcc-tools all do filtering before
> requesting kprobe in the kernel.
>
> > +                     /* mark blacklisted symbol for remove */
> > +                     addrs[i] =3D 0;
> > +             }
> > +     }
> > +
> > +     /* remove blacklisted symbol from addrs */
> > +     for (i =3D 0, cnt =3D 0; i < num; ++i) {
> > +             if (addrs[i])
> > +                     addrs[cnt++]  =3D addrs[i];
> > +     }
> > +
> > +     return cnt;
> > +}
> > +
> >   int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct b=
pf_prog *prog)
> >   {
> >       struct bpf_kprobe_multi_link *link =3D NULL;
> > @@ -2859,6 +2890,12 @@ int bpf_kprobe_multi_link_attach(const union bpf=
_attr *attr, struct bpf_prog *pr
> >       else
> >               link->fp.entry_handler =3D kprobe_multi_link_handler;
> >
> > +     cnt =3D check_bpf_kprobe_addrs_safe(addrs, cnt);
> > +     if (!cnt) {
> > +             err =3D -EINVAL;
> > +             goto error;
> > +     }
> > +
> >       link->addrs =3D addrs;
> >       link->cookies =3D cookies;
> >       link->cnt =3D cnt;