| 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: <20230516135748.9368303c5b092722cdd37601@kernel.org>
Date: Tue, 16 May 2023 13:57:48 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Ze Gao <zegao2021@...il.com>
Cc: Yonghong Song <yhs@...a.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 Thu, 11 May 2023 09:24:18 +0800
Ze Gao <zegao2021@...il.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.
Just FYI, those are not the same. kprobe blacklist is functions marked
by __kprobes or NOKPROBE_SYMBOL(), but fprobe blacklist is "notrace"
functions.
Thank you,
> 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@...a.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@...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
> > >
> > > 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;
--
Masami Hiramatsu (Google) <mhiramat@...nel.org>
Powered by blists - more mailing lists