Date:   Tue, 16 May 2023 14:49:08 +0900
From:   Masami Hiramatsu (Google) <mhiramat@...nel.org>
To:     Yonghong Song <yhs@...a.com>
Cc:     Ze Gao <zegao2021@...il.com>, Jiri Olsa <olsajiri@...il.com>,
        Song Liu <song@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Ze Gao <zegao@...cent.com>, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] bpf: reject blacklisted symbols in kprobe_multi to
 avoid recursive trap

On Fri, 12 May 2023 07:29:02 -0700
Yonghong Song <yhs@...a.com> wrote:

> 
> 
> On 5/11/23 10:53 PM, Ze Gao wrote:
> > 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.
> 
> A fprobe_blacklist might make sense indeed as fprobe and kprobe are 
> quite different... Thanks for working on this.

No, I don't like fprobe_blacklist, because you can filter user given
function with <tracefs>/available_filter_functions :)
If the function is not listed there, you can not put fprobe on it.
IOW, kprobe_multi_link_prog_run only covers those functions. (white-list)

At the tooling side, it should check whether the probe is defined for
single function or multiple functions, and use kprobe-blacklist (single)
or available_filter_functions (multiple).

Thank you,

> 
> > 
> > Thanks
> > Ze
> > 
> > On Thu, May 11, 2023 at 1:28 AM Jiri Olsa <olsajiri@...il.com> wrote:
> >>
> >> 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 kprobe
> >>>> only, we can directly reject those cases early without turning to ftrace.
> >>>>
> >>>> Signed-off-by: Ze Gao <zegao@...cent.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 = 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] = 0;
> >>>> +           }
> >>>> +   }
> >>>> +
> >>>> +   /* remove blacklisted symbol from addrs */
> >>>> +   for (i = 0, cnt = 0; i < num; ++i) {
> >>>> +           if (addrs[i])
> >>>> +                   addrs[cnt++]  = 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 = 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 = kprobe_multi_link_handler;
> >>>> +   cnt = check_bpf_kprobe_addrs_safe(addrs, cnt);
> >>>> +   if (!cnt) {
> >>>> +           err = -EINVAL;
> >>>> +           goto error;
> >>>> +   }
> >>>> +
> >>>>      link->addrs = addrs;
> >>>>      link->cookies = cookies;
> >>>>      link->cnt = cnt;


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists