| 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
| ||
|
Date: Tue, 17 May 2022 14:34:55 -0700
From: Yonghong Song <yhs@...com>
To: Jiri Olsa <olsajiri@...il.com>,
Eugene Syromiatnikov <esyr@...hat.com>
Cc: Masami Hiramatsu <mhiramat@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...hat.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, netdev@...r.kernel.org,
bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in
kprobe_multi.addrs
On 5/17/22 1:03 PM, Jiri Olsa wrote:
> On Tue, May 17, 2022 at 02:30:50PM +0200, Eugene Syromiatnikov wrote:
>> On Tue, May 17, 2022 at 11:12:34AM +0200, Jiri Olsa wrote:
>>> On Tue, May 17, 2022 at 09:36:47AM +0200, Eugene Syromiatnikov wrote:
>>>> With the interface as defined, it is impossible to pass 64-bit kernel
>>>> addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI,
>>>> which severly limits the useability of the interface, change the ABI
>>>> to accept an array of u64 values instead of (kernel? user?) longs.
>>>> Interestingly, the rest of the libbpf infrastructure uses 64-bit values
>>>> for kallsyms addresses already, so this patch also eliminates
>>>> the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
>>>
>>> so the problem is when we have 32bit user sace on 64bit kernel right?
>>>
>>> I think we should keep addrs as longs in uapi and have kernel to figure out
>>> if it needs to read u32 or u64, like you did for symbols in previous patch
>>
>> No, it's not possible here, as addrs are kernel addrs and not user space
>> addrs, so user space has to explicitly pass 64-bit addresses on 64-bit
>> kernels (or have a notion whether it is running on a 64-bit
>> or 32-bit kernel, and form the passed array accordingly, which is against
>> the idea of compat layer that tries to abstract it out).
>
> hum :-\ I'll need to check on compat layer.. there must
> be some other code doing this already somewhere, right?
I am not familiar with all these compatibility thing. But if we
have 64-bit pointer for **syms, maybe we could also have
64-bit pointer for *syms for consistency?
> jirka
>
>>
>>> we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes
>>> 64bit user space pointers
>>>
>>> would be gret if we could have selftest for this
>>>
>>> thanks,
>>> jirka
>>>
>>>>
>>>> Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
>>>> Fixes: 5117c26e877352bc ("libbpf: Add bpf_link_create support for multi kprobes")
>>>> Fixes: ddc6b04989eb0993 ("libbpf: Add bpf_program__attach_kprobe_multi_opts function")
>>>> Fixes: f7a11eeccb111854 ("selftests/bpf: Add kprobe_multi attach test")
>>>> Fixes: 9271a0c7ae7a9147 ("selftests/bpf: Add attach test for bpf_program__attach_kprobe_multi_opts")
>>>> Fixes: 2c6401c966ae1fbe ("selftests/bpf: Add kprobe_multi bpf_cookie test")
>>>> Signed-off-by: Eugene Syromiatnikov <esyr@...hat.com>
>>>> ---
>>>> kernel/trace/bpf_trace.c | 25 ++++++++++++++++++----
>>>> tools/lib/bpf/bpf.h | 2 +-
>>>> tools/lib/bpf/libbpf.c | 8 +++----
>>>> tools/lib/bpf/libbpf.h | 2 +-
>>>> .../testing/selftests/bpf/prog_tests/bpf_cookie.c | 2 +-
>>>> .../selftests/bpf/prog_tests/kprobe_multi_test.c | 8 +++----
>>>> 6 files changed, 32 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>>> index 9d3028a..30a15b3 100644
>>>> --- a/kernel/trace/bpf_trace.c
>>>> +++ b/kernel/trace/bpf_trace.c
>>>> @@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>>>> void __user *ucookies;
>>>> unsigned long *addrs;
>>>> u32 flags, cnt, size, cookies_size;
>>>> - void __user *uaddrs;
>>>> + u64 __user *uaddrs;
>>>> u64 *cookies = NULL;
>>>> void __user *usyms;
>>>> int err;
>>>> @@ -2486,9 +2486,26 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>>>> return -ENOMEM;
>>>>
>>>> if (uaddrs) {
>>>> - if (copy_from_user(addrs, uaddrs, size)) {
>>>> - err = -EFAULT;
>>>> - goto error;
>>>> + if (sizeof(*addrs) == sizeof(*uaddrs)) {
>>>> + if (copy_from_user(addrs, uaddrs, size)) {
>>>> + err = -EFAULT;
>>>> + goto error;
>>>> + }
>>>> + } else {
>>>> + u32 i;
>>>> + u64 addr;
>>>> +
>>>> + for (i = 0; i < cnt; i++) {
>>>> + if (get_user(addr, uaddrs + i)) {
>>>> + err = -EFAULT;
>>>> + goto error;
>>>> + }
>>>> + if (addr > ULONG_MAX) {
>>>> + err = -EINVAL;
>>>> + goto error;
>>>> + }
>>>> + addrs[i] = addr;
>>>> + }
>>>> }
>>>> } else {
>>>> struct user_syms us;
>>>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>>>> index 2e0d373..da9c6037 100644
>>>> --- a/tools/lib/bpf/bpf.h
>>>> +++ b/tools/lib/bpf/bpf.h
>>>> @@ -418,7 +418,7 @@ struct bpf_link_create_opts {
>>>> __u32 flags;
>>>> __u32 cnt;
>>>> const char **syms;
>>>> - const unsigned long *addrs;
>>>> + const __u64 *addrs;
>>>> const __u64 *cookies;
>>>> } kprobe_multi;
>>>> struct {
>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>> index ef7f302..35fa9c5 100644
>>>> --- a/tools/lib/bpf/libbpf.c
>>>> +++ b/tools/lib/bpf/libbpf.c
>>>> @@ -10737,7 +10737,7 @@ static bool glob_match(const char *str, const char *pat)
>>>>
>>>> struct kprobe_multi_resolve {
>>>> const char *pattern;
>>>> - unsigned long *addrs;
>>>> + __u64 *addrs;
>>>> size_t cap;
>>>> size_t cnt;
>>>> };
>>>> @@ -10752,12 +10752,12 @@ resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
>>>> if (!glob_match(sym_name, res->pattern))
>>>> return 0;
>>>>
>>>> - err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(unsigned long),
>>>> + err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(__u64),
>>>> res->cnt + 1);
>>>> if (err)
>>>> return err;
>>>>
>>>> - res->addrs[res->cnt++] = (unsigned long) sym_addr;
>>>> + res->addrs[res->cnt++] = sym_addr;
>>>> return 0;
>>>> }
[...]
Powered by blists - more mailing lists