Messages in this thread | | | Date | Tue, 16 May 2023 08:16:05 -0700 | Subject | Re: [PATCH] bpf: reject blacklisted symbols in kprobe_multi to avoid recursive trap | From | Yonghong Song <> |
| |
On 5/15/23 10:49 PM, Masami Hiramatsu (Google) wrote: > On Fri, 12 May 2023 07:29:02 -0700 > Yonghong Song <yhs@meta.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).
Thanks for clarification. So basically fprobe blacklist is similar to fentry, not able to trace functions marked with notrace. So agree, the checking scheme could be: - user space to check available_filter_functions - a few other tracable functions but may have recursion effect handled by infrastructure for fprobe case. fentry case is already covered by verifier to deny a few functions like preempt_count_sub etc.
> > Thank you, > >> >>> >>> Thanks >>> Ze >>> >>> On Thu, May 11, 2023 at 1:28 AM Jiri Olsa <olsajiri@gmail.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@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 >>>> >>>> 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; > >
| |