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>
 <ZFvUH+p0ebcgnwEg@krava>
In-Reply-To: <ZFvUH+p0ebcgnwEg@krava>
From:   Ze Gao <zegao2021@gmail.com>
Date:   Fri, 12 May 2023 13:53:08 +0800
Message-ID: <CAD8CoPC_=d+Aocp8pnSi9cbU6HWBNc697bKUS1UydtB-4DFzrA@mail.gmail.com>
Subject: Re: [PATCH] bpf: reject blacklisted symbols in kprobe_multi to avoid
 recursive trap
To:     Jiri Olsa <olsajiri@gmail.com>
Cc:     Yonghong Song <yhs@meta.com>, Song Liu <song@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

Yes, Jiri. Thanks for pointing it out. It's true that not all probe
blacklisted functions should be banned from bpf_kprobe.

I tried some of them, and all kprobe blacklisted symbols I hooked
works fine except preempt_count_{sub, add}.
so the takeaway here is preempt_cout_{sub, add} must be rejected at
least for now since kprobe_multi_link_prog_run
( i.e., the fprobe handler) and rethook_trampoline_handler( i.e. the
rethook handler) calls preempt_cout_{sub, add}.

I'm considering providing a general  fprobe_blacklist framework just
like what kprobe does to allow others to mark
functions used inside fprobe handler or rethook handler as NOFPROBE to
avoid potential stack recursion. But only after
I figure out how ftrace handles recursion problems currently and why
it fails in the case I ran into.

Thanks
Ze

On Thu, May 11, 2023 at 1:28=E2=80=AFAM Jiri Olsa <olsajiri@gmail.com> wrot=
e:
>
> On Wed, May 10, 2023 at 07:13:58AM -0700, Yonghong Song 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 kpro=
be
> > > only, we can directly reject those cases early without turning to ftr=
ace.
> > >
> > > 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.
>
> also fprobe uses ftrace drectly without paths in kprobe, so I wonder
> some of the kprobe blacklisted functions are actually safe
>
> jirka
>
> >
> > > +                   /* 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=
 bpf_prog *prog)
> > >   {
> > >     struct bpf_kprobe_multi_link *link =3D NULL;
> > > @@ -2859,6 +2890,12 @@ int bpf_kprobe_multi_link_attach(const union b=
pf_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;