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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7c5e64f2-f2cf-61b7-9231-fc267bf0f2d8@fb.com>
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ