lkml.org 
[lkml]   [2023]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] bpf: reject blacklisted symbols in kprobe_multi to avoid recursive trap
From


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.

>
> 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;

\
 
 \ /
  Last update: 2023-05-12 16:30    [W:0.282 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site