| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9d211c94-5108-e4d3-9e6c-7060da710c35@meta.com>
Date: Tue, 16 May 2023 08:16:05 -0700
From: Yonghong Song <yhs@...a.com>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
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>,
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 5/15/23 10:49 PM, Masami Hiramatsu (Google) wrote:
> 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).
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@...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;
>
>
Powered by blists - more mailing lists