lkml.org 
[lkml]   [2023]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] bpf: reject blacklisted symbols in kprobe_multi to avoid recursive trap
    I just looked through fprobe_handler, it already does the recursion
    check from the code. So the root cause of the case I mentioned
    above which triggers kernel crash may be much more complicated
    than I read from the exception backtrace.

    It seems more effort is needed to look for a better solution than my
    initial proposal. I will keep the thread updated if there is any progress
    anyway.

    Thanks
    Ze

    On Thu, May 11, 2023 at 9:24 AM Ze Gao <zegao2021@gmail.com> wrote:
    >
    > Thank yonghong for your sage reviews.
    > Yes, this is an option I am also considering . I will try this out
    > later to see if works
    >
    > But like you said it's not clear whether kprobe blacklist== fprobe blacklist.
    > And also there are cases I need to investigate on, like how to avoid recursions
    > when kprobes and fprobes are mixed.
    >
    > Rejecting symbols kprobe_blacklisted is kinda brute-force yet a straight way to
    > avoid kernel crash AFAIK.
    >
    > Ze
    >
    > On Thu, May 11, 2023 at 7:54 AM Yonghong Song <yhs@meta.com> wrote:
    > >
    > >
    > >
    > > On 5/10/23 1:20 PM, Yonghong Song wrote:
    > > >
    > > >
    > > > On 5/10/23 10:27 AM, Jiri Olsa 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@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 = 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
    > > >
    > > > Could you give a pointer about 'some of the kprobe blacklisted
    > > > functions are actually safe'?
    > >
    > > Thanks Jiri for answering my question. it is not clear whether
    > > kprobe blacklist == fprobe blacklist, probably not.
    > >
    > > You mentioned:
    > > note that ftrace provides recursion detection mechanism,
    > > but for kprobe only
    > > Maybe the right choice is to improve ftrace to provide recursion
    > > detection mechanism for fprobe as well?
    > >
    > > >
    > > >>
    > > >> 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;

    \
     
     \ /
      Last update: 2023-05-11 04:07    [W:2.353 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site