Fix 32-bit arch and compat support for the kprobe_multi attach type
Subsystems (9)
v3 cover Fix 32-bit arch and compat support for the kprobe_multi attach type
As suggested in [1], the kprobe_multi interface is to be fixed for 32-bit architectures and compat, rather then disabled. As it turned out, there are a couple of additional problems that are to be addressed:
- the absence of size overflow checks, leading to possible out-of-bounds writes (addressed by the first patch; this one likely has to be fixed in 5.18, where the version of the patch from [3] may be preferrable, along with [4] to avoid applying the rest of the series);
- the assumption that long has the same size as u64, which would make cookies arrays size calculation incorrect on 32-bit architectures (addressed by the second patch);
- the addrs array passing API, that is incompatible with compat and has to be changed (addressed in the fourth patch): those are kernel addresses and not user ones (as was incorrectly stated in [2]); this change is only semantical for 64-bit user/kernelspace, so it shouldn't impact ABI there, at least.
[1] https://lore.kernel.org/lkml/CAADnVQ+2gwhcMht4PuDnDOFKY68Wsq8QFz4Y69NBX_TLaSexQQ@mail.gmail.com/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/
[4] https://lore.kernel.org/lkml/[email protected]/
v3:
- Rebased on top of bpf-next
- Removed unnecessary size/cookies_size assignments as suggested by Yonghong Sond
v2: https://lore.kernel.org/lkml/[email protected]/
- Fixed the isses reported by CI
v1: https://lore.kernel.org/lkml/[email protected]/
Eugene Syromiatnikov (4):
bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach
bpf_trace: handle compat in copy_user_syms
bpf_trace: pass array of u64 values in kprobe_multi.addrs
kernel/trace/bpf_trace.c | 67 ++++++++++++++++------ 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, 62 insertions(+), 27 deletions(-)
Signature · 1 line
2.1.4
PATCH v3 1/4 bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
Check that size would not overflow before calculation (and return -EOVERFLOW if it will), to prevent potential out-of-bounds write with the following copy_from_user. Use kvmalloc_array in copy_user_syms to prevent out-of-bounds write into syms (and especially buf) as well.
--- kernel/trace/bpf_trace.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
kernel/trace/bpf_trace.c +4 -3
@@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32int err = -ENOMEM ;unsigned int i; - syms =kvmalloc (cnt *sizeof (*syms),GFP_KERNEL ); + syms =kvmalloc_array (cnt,sizeof (*syms),GFP_KERNEL );if (!syms) goto error; - buf =kvmalloc (cnt *KSYM_NAME_LEN ,GFP_KERNEL ); + buf =kvmalloc_array (cnt,KSYM_NAME_LEN ,GFP_KERNEL );if (!buf) goto error;
@@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ; - size = cnt *sizeof (*addrs); +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +return -EOVERFLOW ; addrs =kvmalloc (size,GFP_KERNEL );if (!addrs)return -ENOMEM ;
Signature · 1 line
2.1.4
PATCH v3 2/4 bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach
In reply to Eugene SyromiatnikovIt seems that there is no reason not to support 32-bit architectures; doing so requires a bit of rework with respect to cookies handling, however, as the current code implicitly assumes
that sizeof(long) == sizeof(u64).
--- kernel/trace/bpf_trace.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
kernel/trace/bpf_trace.c +8 -9
@@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prstruct bpf_link_primer link_primer;void __user *ucookies;unsigned long *addrs; -u32 flags, cnt, size; +u32 flags, cnt, size, cookies_size;void __user *uaddrs;u64 *cookies =NULL ;void __user *usyms;int err; -/* no support for 32bit archs yet */ -if (sizeof (u64) !=sizeof (void *)) -return -EOPNOTSUPP ; -if (prog->expected_attach_type !=BPF_TRACE_KPROBE_MULTI )return -EINVAL ;
@@ -2454,6 +2450,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr uaddrs =u64_to_user_ptr (attr->link_create.kprobe_multi.addrs); usyms =u64_to_user_ptr (attr->link_create.kprobe_multi.syms); + ucookies =u64_to_user_ptr (attr->link_create.kprobe_multi.cookies);if (!!uaddrs == !!usyms)return -EINVAL ;
@@ -2461,8 +2458,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ; -if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size) || + (ucookies && +check_mul_overflow (cnt, (u32 )sizeof (*cookies), &cookies_size))) {return -EOVERFLOW ; + } addrs =kvmalloc (size,GFP_KERNEL );if (!addrs)return -ENOMEM ;
@@ -2486,14 +2486,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr goto error; } - ucookies =u64_to_user_ptr (attr->link_create.kprobe_multi.cookies);if (ucookies) { - cookies =kvmalloc (size,GFP_KERNEL ); + cookies =kvmalloc (cookies_size,GFP_KERNEL );if (!cookies) { err = -ENOMEM ; goto error; } -if (copy_from_user (cookies, ucookies, size)) { +if (copy_from_user (cookies, ucookies, cookies_size)) { err = -EFAULT ; goto error; }
Signature · 1 line
2.1.4
PATCH v3 3/4 bpf_trace: handle compat in copy_user_syms
In reply to Eugene SyromiatnikovFor compat processes, userspace size for syms pointers is different. Provide compat handling for copying array elements from the user space.
--- kernel/trace/bpf_trace.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
kernel/trace/bpf_trace.c +19 -1
@@ -2253,6 +2253,24 @@ struct user_syms {char *buf; }; +static inline int get_arr_ptr (unsigned long *p, +unsigned long __user *uaddr,u32 idx) +{ +if (unlikely (in_compat_syscall ())) { +compat_uptr_t __user *compat_uaddr = (compat_uptr_t __user *)uaddr; +compat_uptr_t val; +int err; + + err =__get_user (val, compat_uaddr + idx); +if (!err) + *p = val; + +return err; + }else { +return __get_user (*p, uaddr + idx); + } +} +static int copy_user_syms (struct user_syms *us,unsigned long __user *usyms,u32 cnt) {unsigned long __user usymbol;
@@ -2270,7 +2288,7 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 goto error;for (p = buf, i =0 ; i < cnt; i++) { -if (__get_user (usymbol, usyms + i)) { +if (get_arr_ptr (&usymbol, usyms, i)) { err = -EFAULT ; goto error; }
Signature · 1 line
2.1.4
PATCH v3 4/4 bpf_trace: pass array of u64 values in kprobe_multi.addrs
In reply to Eugene SyromiatnikovWith 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().
--- 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(-)
kernel/trace/bpf_trace.c +21 -4
@@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prvoid __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 *prreturn -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;
tools/lib/bpf/bpf.h +1 -1
@@ -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 {
tools/lib/bpf/libbpf.c +4 -4
@@ -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 ; }
@@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, };struct bpf_link *link =NULL ;char errmsg[STRERR_BUFSIZE ]; -const unsigned long *addrs; +const __u64 *addrs;int err, link_fd, prog_fd;const __u64 *cookies;const char **syms;
tools/lib/bpf/libbpf.h +1 -1
@@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts {/* array of function symbols to attach */ const char **syms;/* array of function addresses to attach */ -const unsigned long *addrs; +const __u64 *addrs;/* array of user-provided values fetchable through bpf_get_attach_cookie */ const __u64 *cookies;/* number of elements in syms/addrs/cookies arrays */
tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +1 -1
@@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) cookies[6 ] =7 ; cookies[7 ] =8 ; - opts.kprobe_multi.addrs = (const unsigned long *) &addrs; + opts.kprobe_multi.addrs = (const __u64 *) &addrs; opts.kprobe_multi.cnt =ARRAY_SIZE (addrs); opts.kprobe_multi.cookies = (const __u64 *) &cookies; prog_fd =bpf_program__fd (skel->progs.test_kprobe);
tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +4 -4
@@ -108,7 +108,7 @@ static void test_link_api_addrs(void)GET_ADDR ("bpf_fentry_test7" , addrs[6 ]);GET_ADDR ("bpf_fentry_test8" , addrs[7 ]); - opts.kprobe_multi.addrs = (const unsigned long *) addrs; + opts.kprobe_multi.addrs = (const __u64 *) addrs; opts.kprobe_multi.cnt =ARRAY_SIZE (addrs);test_link_api (&opts); }
@@ -186,7 +186,7 @@ static void test_attach_api_addrs(void)GET_ADDR ("bpf_fentry_test7" , addrs[6 ]);GET_ADDR ("bpf_fentry_test8" , addrs[7 ]); - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.cnt =ARRAY_SIZE (addrs);test_attach_api (NULL , &opts); }
@@ -244,7 +244,7 @@ static void test_attach_api_fails(void) goto cleanup;/* fail_2 - both addrs and syms set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms = syms; opts.cnt =ARRAY_SIZE (syms); opts.cookies =NULL ;
@@ -258,7 +258,7 @@ static void test_attach_api_fails(void) goto cleanup;/* fail_3 - pattern and addrs set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms =NULL ; opts.cnt =ARRAY_SIZE (syms); opts.cookies =NULL ;
Signature · 1 line
2.1.4
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
Quoted 7 lines · Eugene Syromiatnikov
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 thesym_addrcast 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
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
Quoted 179 lines
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 <[email protected]>--- 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(-)kernel/trace/bpf_trace.c +21
-4@@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prvoid __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 *prreturn -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;tools/lib/bpf/bpf.h +1
-1@@ -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 {tools/lib/bpf/libbpf.c +4
-4@@ -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 ; }@@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, };struct bpf_link *link =NULL ;char errmsg[STRERR_BUFSIZE ]; -const unsigned long *addrs; +const __u64 *addrs;int err, link_fd, prog_fd;const __u64 *cookies;const char **syms;tools/lib/bpf/libbpf.h +1
-1@@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts {/* array of function symbols to attach */ const char **syms;/* array of function addresses to attach */ -const unsigned long *addrs; +const __u64 *addrs;/* array of user-provided values fetchable through bpf_get_attach_cookie */ const __u64 *cookies;/* number of elements in syms/addrs/cookies arrays */ tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +1
-1@@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) cookies[6 ] =7 ; cookies[7 ] =8 ; - opts.kprobe_multi.addrs = (const unsigned long *) &addrs; + opts.kprobe_multi.addrs = (const __u64 *) &addrs; opts.kprobe_multi.cnt =ARRAY_SIZE (addrs); opts.kprobe_multi.cookies = (const __u64 *) &cookies; prog_fd =bpf_program__fd (skel->progs.test_kprobe);tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +4
-4@@ -108,7 +108,7 @@ static void test_link_api_addrs(void)GET_ADDR ("bpf_fentry_test7" , addrs[6 ]);GET_ADDR ("bpf_fentry_test8" , addrs[7 ]); - opts.kprobe_multi.addrs = (const unsigned long *) addrs; + opts.kprobe_multi.addrs = (const __u64 *) addrs; opts.kprobe_multi.cnt =ARRAY_SIZE (addrs);test_link_api (&opts); }@@ -186,7 +186,7 @@ static void test_attach_api_addrs(void)GET_ADDR ("bpf_fentry_test7" , addrs[6 ]);GET_ADDR ("bpf_fentry_test8" , addrs[7 ]); - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.cnt =ARRAY_SIZE (addrs);test_attach_api (NULL , &opts); }@@ -244,7 +244,7 @@ static void test_attach_api_fails(void) goto cleanup;/* fail_2 - both addrs and syms set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms = syms; opts.cnt =ARRAY_SIZE (syms); opts.cookies =NULL ;@@ -258,7 +258,7 @@ static void test_attach_api_fails(void) goto cleanup;/* fail_3 - pattern and addrs set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms =NULL ; opts.cnt =ARRAY_SIZE (syms); opts.cookies =NULL ;Signature · 1 line
2.1.4
Re: [PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
In reply to Eugene SyromiatnikovQuoted 9 lines · Eugene Syromiatnikov
Check that size would not overflow before calculation (and return
-EOVERFLOWif it will), to prevent potential out-of-bounds write with the followingcopy_from_user. Usekvmalloc_arrayincopy_user_symsto prevent out-of-bounds write into syms (and especially buf) as well.Fixes 0dcac272540613d4 ("bpf: Add multi kprobe link")Cc <[email protected]> # 5.18Signed-off-by Eugene Syromiatnikov <[email protected]>
thanks,
jirka
Quoted 35 lines
--- kernel/trace/bpf_trace.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)kernel/trace/bpf_trace.c +4
-3@@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32int err = -ENOMEM ;unsigned int i; - syms =kvmalloc (cnt *sizeof (*syms),GFP_KERNEL ); + syms =kvmalloc_array (cnt,sizeof (*syms),GFP_KERNEL );if (!syms) goto error; - buf =kvmalloc (cnt *KSYM_NAME_LEN ,GFP_KERNEL ); + buf =kvmalloc_array (cnt,KSYM_NAME_LEN ,GFP_KERNEL );if (!buf) goto error;@@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ; - size = cnt *sizeof (*addrs); +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +return -EOVERFLOW ; addrs =kvmalloc (size,GFP_KERNEL );if (!addrs)return -ENOMEM ;Signature · 1 line
2.1.4
Re: [PATCH bpf-next v3 2/4] bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach
In reply to Eugene SyromiatnikovQuoted 54 lines · Eugene Syromiatnikov
It seems that there is no reason not to support 32-bit architectures; doing so requires a bit of rework with respect to cookies handling, however, as the current code implicitly assumes
that sizeof(long) == sizeof(u64).Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)kernel/trace/bpf_trace.c +6
-6@@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prstruct bpf_link_primer link_primer;void __user *ucookies;unsigned long *addrs; -u32 flags, cnt, size; +u32 flags, cnt, size, cookies_size;void __user *uaddrs;u64 *cookies =NULL ;void __user *usyms;int err; -/* no support for 32bit archs yet */ -if (sizeof (u64) !=sizeof (void *)) -return -EOPNOTSUPP ; -if (prog->expected_attach_type !=BPF_TRACE_KPROBE_MULTI )return -EINVAL ;@@ -2454,6 +2450,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr uaddrs =u64_to_user_ptr (attr->link_create.kprobe_multi.addrs); usyms =u64_to_user_ptr (attr->link_create.kprobe_multi.syms); + ucookies =u64_to_user_ptr (attr->link_create.kprobe_multi.cookies);if (!!uaddrs == !!usyms)return -EINVAL ;@@ -2461,8 +2458,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ; -if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size) || + (ucookies && +check_mul_overflow (cnt, (u32 )sizeof (*cookies), &cookies_size))) {return -EOVERFLOW ; + } addrs =kvmalloc (size,GFP_KERNEL );if (!addrs)return -ENOMEM ;
kernel/trace/bpf_trace.c int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr · line 2489
Quoted 5 lines
kernel/trace/bpf_trace.c +0
-1@@ -2486,14 +2486,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr goto error; } - ucookies =u64_to_user_ptr (attr->link_create.kprobe_multi.cookies);if (ucookies) {
could we check all that in here? so the ucookies checks are on the one place.. also you would not need cookies_size
jirka
Quoted 14 lines
kernel/trace/bpf_trace.c +2
-2- cookies =kvmalloc (size,GFP_KERNEL ); + cookies =kvmalloc (cookies_size,GFP_KERNEL );if (!cookies) { err = -ENOMEM ; goto error; } -if (copy_from_user (cookies, ucookies, size)) { +if (copy_from_user (cookies, ucookies, cookies_size)) { err = -EFAULT ; goto error; }Signature · 1 line
2.1.4
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
In reply to Jiri OlsaQuoted 9 lines · Jiri Olsa
Quoted 7 lines · Eugene Syromiatnikov
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 thesym_addrcast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
Quoted 4 lines
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).
Quoted 188 lines
we'll need to fix also
bpf_kprobe_multi_cookie_swapbecause it assumes 64bit user space pointerswould be gret if we could have selftest for this
thanks,
jirkaQuoted 179 lines
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 <[email protected]>--- 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(-)kernel/trace/bpf_trace.c +21
-4@@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prvoid __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 *prreturn -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;tools/lib/bpf/bpf.h +1
-1@@ -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 {tools/lib/bpf/libbpf.c +4
-4@@ -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 ; }@@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, };struct bpf_link *link =NULL ;char errmsg[STRERR_BUFSIZE ]; -const unsigned long *addrs; +const __u64 *addrs;int err, link_fd, prog_fd;const __u64 *cookies;const char **syms;tools/lib/bpf/libbpf.h +1
-1@@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts {/* array of function symbols to attach */ const char **syms;/* array of function addresses to attach */ -const unsigned long *addrs; +const __u64 *addrs;/* array of user-provided values fetchable through bpf_get_attach_cookie */ const __u64 *cookies;/* number of elements in syms/addrs/cookies arrays */ tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +1
-1@@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) cookies[6 ] =7 ; cookies[7 ] =8 ; - opts.kprobe_multi.addrs = (const unsigned long *) &addrs; + opts.kprobe_multi.addrs = (const __u64 *) &addrs; opts.kprobe_multi.cnt =ARRAY_SIZE (addrs); opts.kprobe_multi.cookies = (const __u64 *) &cookies; prog_fd =bpf_program__fd (skel->progs.test_kprobe);tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +4
-4@@ -108,7 +108,7 @@ static void test_link_api_addrs(void)GET_ADDR ("bpf_fentry_test7" , addrs[6 ]);GET_ADDR ("bpf_fentry_test8" , addrs[7 ]); - opts.kprobe_multi.addrs = (const unsigned long *) addrs; + opts.kprobe_multi.addrs = (const __u64 *) addrs; opts.kprobe_multi.cnt =ARRAY_SIZE (addrs);test_link_api (&opts); }@@ -186,7 +186,7 @@ static void test_attach_api_addrs(void)GET_ADDR ("bpf_fentry_test7" , addrs[6 ]);GET_ADDR ("bpf_fentry_test8" , addrs[7 ]); - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.cnt =ARRAY_SIZE (addrs);test_attach_api (NULL , &opts); }@@ -244,7 +244,7 @@ static void test_attach_api_fails(void) goto cleanup;/* fail_2 - both addrs and syms set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms = syms; opts.cnt =ARRAY_SIZE (syms); opts.cookies =NULL ;@@ -258,7 +258,7 @@ static void test_attach_api_fails(void) goto cleanup;/* fail_3 - pattern and addrs set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms =NULL ; opts.cnt =ARRAY_SIZE (syms); opts.cookies =NULL ;Signature · 1 line
2.1.4
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
Quoted 12 lines · Eugene Syromiatnikov
Quoted 11 lines · Jiri Olsa
Quoted 7 lines · Eugene Syromiatnikov
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 thesym_addrcast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().so the problem is when we have 32bit user sace on 64bit kernel right?
Quoted 8 lines
Quoted 2 lines
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?
jirka
Quoted 190 lines
Quoted 189 lines
we'll need to fix also
bpf_kprobe_multi_cookie_swapbecause it assumes 64bit user space pointerswould be gret if we could have selftest for this
thanks,
jirkaQuoted 179 lines
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 <[email protected]>--- 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(-)kernel/trace/bpf_trace.c +21
-4@@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prvoid __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 *prreturn -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;tools/lib/bpf/bpf.h +1
-1@@ -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 {tools/lib/bpf/libbpf.c +4
-4@@ -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 ; }@@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, };struct bpf_link *link =NULL ;char errmsg[STRERR_BUFSIZE ]; -const unsigned long *addrs; +const __u64 *addrs;int err, link_fd, prog_fd;const __u64 *cookies;const char **syms;tools/lib/bpf/libbpf.h +1
-1@@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts {/* array of function symbols to attach */ const char **syms;/* array of function addresses to attach */ -const unsigned long *addrs; +const __u64 *addrs;/* array of user-provided values fetchable through bpf_get_attach_cookie */ const __u64 *cookies;/* number of elements in syms/addrs/cookies arrays */ tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +1
-1@@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) cookies[6 ] =7 ; cookies[7 ] =8 ; - opts.kprobe_multi.addrs = (const unsigned long *) &addrs; + opts.kprobe_multi.addrs = (const __u64 *) &addrs; opts.kprobe_multi.cnt =ARRAY_SIZE (addrs); opts.kprobe_multi.cookies = (const __u64 *) &cookies; prog_fd =bpf_program__fd (skel->progs.test_kprobe);tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +4
-4@@ -108,7 +108,7 @@ static void test_link_api_addrs(void)GET_ADDR ("bpf_fentry_test7" , addrs[6 ]);GET_ADDR ("bpf_fentry_test8" , addrs[7 ]); - opts.kprobe_multi.addrs = (const unsigned long *) addrs; + opts.kprobe_multi.addrs = (const __u64 *) addrs; opts.kprobe_multi.cnt =ARRAY_SIZE (addrs);test_link_api (&opts); }@@ -186,7 +186,7 @@ static void test_attach_api_addrs(void)GET_ADDR ("bpf_fentry_test7" , addrs[6 ]);GET_ADDR ("bpf_fentry_test8" , addrs[7 ]); - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.cnt =ARRAY_SIZE (addrs);test_attach_api (NULL , &opts); }@@ -244,7 +244,7 @@ static void test_attach_api_fails(void) goto cleanup;/* fail_2 - both addrs and syms set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms = syms; opts.cnt =ARRAY_SIZE (syms); opts.cookies =NULL ;@@ -258,7 +258,7 @@ static void test_attach_api_fails(void) goto cleanup;/* fail_3 - pattern and addrs set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms =NULL ; opts.cnt =ARRAY_SIZE (syms); opts.cookies =NULL ;Signature · 1 line
2.1.4
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
Quoted 16 lines · Jiri Olsa
Quoted 15 lines · Eugene Syromiatnikov
Quoted 13 lines · Jiri Olsa
Quoted 7 lines · Eugene Syromiatnikov
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 thesym_addrcast 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
Quoted 8 lines
Quoted 5 lines
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?
Quoted 111 lines
jirka
Quoted 109 lines
Quoted 109 lines
we'll need to fix also
bpf_kprobe_multi_cookie_swapbecause it assumes 64bit user space pointerswould be gret if we could have selftest for this
thanks,
jirkaQuoted 100 lines
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 <[email protected]>--- 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(-)kernel/trace/bpf_trace.c +21
-4@@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prvoid __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 *prreturn -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;tools/lib/bpf/bpf.h +1
-1@@ -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 {tools/lib/bpf/libbpf.c +3
-3@@ -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 ; }
[...]
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
Quoted 19 lines · Yonghong Song
Quoted 16 lines · Jiri Olsa
Quoted 15 lines · Eugene Syromiatnikov
Quoted 13 lines · Jiri Olsa
Quoted 7 lines · Eugene Syromiatnikov
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 thesym_addrcast 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
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
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?
so the 32bit application running on 64bit kernel using libbpf won't work at the moment, right? because it sees:
bpf_kprobe_multi_opts::addrs as its 'unsigned long'
which is 4 bytes and it needs to put there 64bits kernel addresses
if we force the libbpf interface to use u64, then we should be fine
Quoted 4 lines
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?
right, perhaps we could have one function to read both syms and addrs arrays
Quoted 6 lines
Quoted 6 lines
jirka
Quoted 3 lines
Quoted 3 lines
we'll need to fix also
bpf_kprobe_multi_cookie_swapbecause it assumes 64bit user space pointers
if we have both addresses and cookies 64 then this should be ok
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
would be gret if we could have selftest for this
let's add selftest for this
thanks,
jirka
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
Quoted 27 lines · Jiri Olsa
Quoted 17 lines · Yonghong Song
Quoted 16 lines · Jiri Olsa
Quoted 15 lines · Eugene Syromiatnikov
Quoted 13 lines · Jiri Olsa
Quoted 7 lines · Eugene Syromiatnikov
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 thesym_addrcast 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
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
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?
Quoted 8 lines
so the 32bit application running on 64bit kernel using libbpf won't work at the moment, right? because it sees:
bpf_kprobe_multi_opts::addrsas its 'unsigned long'which is 4 bytes and it needs to put there 64bits kernel addresses
if we force the libbpf interface to use u64, then we should be fine
Yes, that's correct.
Quoted 5 lines
Quoted 3 lines
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?
right, perhaps we could have one function to read both syms and addrs arrays
The distinction here it that syms are user space pointers (so they are naturally 32-bit for 32-bit applications) and addrs are kernel-space pointers (so they may be 64-bit even when the application is 32-bit). Nothing prevents from changing the interface so that syms is an array of 64-bit values treated as user space pointers, of course.
Quoted 9 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
we'll need to fix also
bpf_kprobe_multi_cookie_swapbecause it assumes 64bit user space pointersif we have both addresses and cookies 64 then this should be ok
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
would be gret if we could have selftest for this
let's add selftest for this
Sure, I'll try to write one.
Re: [PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
In reply to Eugene SyromiatnikovQuoted 33 lines · Eugene Syromiatnikov
Check that size would not overflow before calculation (and return
-EOVERFLOWif it will), to prevent potential out-of-bounds write with the followingcopy_from_user. Usekvmalloc_arrayincopy_user_symsto prevent out-of-bounds write into syms (and especially buf) as well.Fixes 0dcac272540613d4 ("bpf: Add multi kprobe link")Cc <[email protected]> # 5.18Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)kernel/trace/bpf_trace.c +2
-2@@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32int err = -ENOMEM ;unsigned int i; - syms =kvmalloc (cnt *sizeof (*syms),GFP_KERNEL ); + syms =kvmalloc_array (cnt,sizeof (*syms),GFP_KERNEL );if (!syms) goto error; - buf =kvmalloc (cnt *KSYM_NAME_LEN ,GFP_KERNEL ); + buf =kvmalloc_array (cnt,KSYM_NAME_LEN ,GFP_KERNEL );if (!buf) goto error;
kernel/trace/bpf_trace.c int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr · line 2466
Quoted 7 lines
kernel/trace/bpf_trace.c +2
-1@@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ; - size = cnt *sizeof (*addrs); +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +return -EOVERFLOW ; addrs =kvmalloc (size,GFP_KERNEL );
any good reason not to use kvmalloc_array() here as well and delegate overflow to it. And then use long size (as expected by copy_from_user anyway) everywhere?
Quoted 5 lines
kernel/trace/bpf_trace.c +0
-1if (!addrs)return -ENOMEM ; --2.1.4
Re: [PATCH bpf-next v3 2/4] bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach
In reply to Eugene SyromiatnikovQuoted 55 lines · Eugene Syromiatnikov
It seems that there is no reason not to support 32-bit architectures; doing so requires a bit of rework with respect to cookies handling, however, as the current code implicitly assumes
that sizeof(long) == sizeof(u64).Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)kernel/trace/bpf_trace.c +6
-6@@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prstruct bpf_link_primer link_primer;void __user *ucookies;unsigned long *addrs; -u32 flags, cnt, size; +u32 flags, cnt, size, cookies_size;void __user *uaddrs;u64 *cookies =NULL ;void __user *usyms;int err; -/* no support for 32bit archs yet */ -if (sizeof (u64) !=sizeof (void *)) -return -EOPNOTSUPP ; -if (prog->expected_attach_type !=BPF_TRACE_KPROBE_MULTI )return -EINVAL ;@@ -2454,6 +2450,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr uaddrs =u64_to_user_ptr (attr->link_create.kprobe_multi.addrs); usyms =u64_to_user_ptr (attr->link_create.kprobe_multi.syms); + ucookies =u64_to_user_ptr (attr->link_create.kprobe_multi.cookies);if (!!uaddrs == !!usyms)return -EINVAL ;@@ -2461,8 +2458,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ; -if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size) || + (ucookies && +check_mul_overflow (cnt, (u32 )sizeof (*cookies), &cookies_size))) {return -EOVERFLOW ; + } addrs =kvmalloc (size,GFP_KERNEL );if (!addrs)return -ENOMEM ;
kernel/trace/bpf_trace.c int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr · line 2490
Quoted 7 lines
kernel/trace/bpf_trace.c +1
-2@@ -2486,14 +2486,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr goto error; } - ucookies =u64_to_user_ptr (attr->link_create.kprobe_multi.cookies);if (ucookies) { - cookies =kvmalloc (size,GFP_KERNEL ); + cookies =kvmalloc (cookies_size,GFP_KERNEL );
same question about consistent use of kvmalloc_array() and delegating all the overflow checks to it?
Quoted 12 lines
kernel/trace/bpf_trace.c +1
-2if (!cookies) { err = -ENOMEM ; goto error; } -if (copy_from_user (cookies, ucookies, size)) { +if (copy_from_user (cookies, ucookies, cookies_size)) { err = -EFAULT ; goto error; } --2.1.4
Re: [PATCH bpf-next v3 3/4] bpf_trace: handle compat in copy_user_syms
In reply to Eugene SyromiatnikovQuoted 15 lines · Eugene Syromiatnikov
For compat processes, userspace size for syms pointers is different. Provide compat handling for copying array elements from the user space.
Fixes 0dcac272540613d4 ("bpf: Add multi kprobe link")Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)kernel/trace/bpf_trace.c +0
-0
kernel/trace/bpf_trace.c struct user_syms { · line 2257
Quoted 5 lines
kernel/trace/bpf_trace.c +2
-0@@ -2253,6 +2253,24 @@ struct user_syms {char *buf; }; +static inline int get_arr_ptr (unsigned long *p, +unsigned long __user *uaddr,u32 idx)
no need for inline, let compiler decide on inlining
kernel/trace/bpf_trace.c struct user_syms { · line 2259
Quoted 2 lines
kernel/trace/bpf_trace.c +2
-0+{ +if (unlikely (in_compat_syscall ())) {
not sure unlikely() is justified for code...
Quoted 29 lines
kernel/trace/bpf_trace.c +15
-2+compat_uptr_t __user *compat_uaddr = (compat_uptr_t __user *)uaddr; +compat_uptr_t val; +int err; + + err =__get_user (val, compat_uaddr + idx); +if (!err) + *p = val; + +return err; + }else { +return __get_user (*p, uaddr + idx); + } +} +static int copy_user_syms (struct user_syms *us,unsigned long __user *usyms,u32 cnt) {unsigned long __user usymbol;@@ -2270,7 +2288,7 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 goto error;for (p = buf, i =0 ; i < cnt; i++) { -if (__get_user (usymbol, usyms + i)) { +if (get_arr_ptr (&usymbol, usyms, i)) { err = -EFAULT ; goto error; } --2.1.4
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
In reply to Eugene SyromiatnikovQuoted 44 lines · Eugene Syromiatnikov
Quoted 27 lines · Jiri Olsa
Quoted 17 lines · Yonghong Song
Quoted 16 lines · Jiri Olsa
Quoted 15 lines · Eugene Syromiatnikov
Quoted 13 lines · Jiri Olsa
Quoted 7 lines · Eugene Syromiatnikov
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 thesym_addrcast 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
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
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?
Quoted 8 lines
so the 32bit application running on 64bit kernel using libbpf won't work at the moment, right? because it sees:
bpf_kprobe_multi_opts::addrsas its 'unsigned long'which is 4 bytes and it needs to put there 64bits kernel addresses
if we force the libbpf interface to use u64, then we should be fine
Yes, that's correct.
Quoted 4 lines
Quoted 3 lines
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?
Quoted 7 lines
Quoted 1 line
right, perhaps we could have one function to read both syms and addrs arrays
The distinction here it that syms are user space pointers (so they are naturally 32-bit for 32-bit applications) and addrs are kernel-space pointers (so they may be 64-bit even when the application is 32-bit). Nothing prevents from changing the interface so that syms is an array of 64-bit values treated as user space pointers, of course.
I agree. User-space pointers should stay pointers in libbpf API , while kernel addresses are not really pointers for user-space app, so marking it as __u64 seems right.
Quoted 13 lines
Quoted 10 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
we'll need to fix also
bpf_kprobe_multi_cookie_swapbecause it assumes 64bit user space pointersif we have both addresses and cookies 64 then this should be ok
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
would be gret if we could have selftest for this
let's add selftest for this
Sure, I'll try to write one.
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
In reply to Eugene SyromiatnikovQuoted 55 lines · Eugene Syromiatnikov
Quoted 27 lines · Jiri Olsa
Quoted 17 lines · Yonghong Song
Quoted 16 lines · Jiri Olsa
Quoted 15 lines · Eugene Syromiatnikov
Quoted 13 lines · Jiri Olsa
Quoted 7 lines · Eugene Syromiatnikov
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 thesym_addrcast 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
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
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?
Quoted 8 lines
so the 32bit application running on 64bit kernel using libbpf won't work at the moment, right? because it sees:
bpf_kprobe_multi_opts::addrsas its 'unsigned long'which is 4 bytes and it needs to put there 64bits kernel addresses
if we force the libbpf interface to use u64, then we should be fine
Yes, that's correct.
Quoted 5 lines
Quoted 3 lines
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?
right, perhaps we could have one function to read both syms and addrs arrays
The distinction here it that syms are user space pointers (so they are naturally 32-bit for 32-bit applications) and addrs are kernel-space pointers (so they may be 64-bit even when the application is 32-bit). Nothing prevents from changing the interface so that syms is an array of 64-bit values treated as user space pointers, of course.
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
we'll need to fix also
bpf_kprobe_multi_cookie_swapbecause it assumes 64bit user space pointers
Quoted 9 lines
Quoted 6 lines
if we have both addresses and cookies 64 then this should be ok
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
would be gret if we could have selftest for this
let's add selftest for this
Sure, I'll try to write one.
Not sure how you can do that without having extra test_progs variant that's running in compat mode?
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
In reply to Eugene SyromiatnikovQuoted 16 lines · Eugene Syromiatnikov
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 thesym_addrcast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().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 <[email protected]>
Quoted 2 lines
--- kernel/trace/bpf_trace.c | 25 ++++++++++++++++++----
kernel changes should go into a separate patch (and seems like they logically fit together with patch #3, no?)
Quoted 7 lines
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(-)
[...]
Re: [PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
In reply to Andrii NakryikoQuoted 37 lines · Andrii Nakryiko
Quoted 36 lines · Eugene Syromiatnikov
Check that size would not overflow before calculation (and return
-EOVERFLOWif it will), to prevent potential out-of-bounds write with the followingcopy_from_user. Usekvmalloc_arrayincopy_user_symsto prevent out-of-bounds write into syms (and especially buf) as well.Fixes 0dcac272540613d4 ("bpf: Add multi kprobe link")Cc <[email protected]> # 5.18Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)kernel/trace/bpf_trace.c +2
-2@@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32int err = -ENOMEM ;unsigned int i; - syms =kvmalloc (cnt *sizeof (*syms),GFP_KERNEL ); + syms =kvmalloc_array (cnt,sizeof (*syms),GFP_KERNEL );if (!syms) goto error; - buf =kvmalloc (cnt *KSYM_NAME_LEN ,GFP_KERNEL ); + buf =kvmalloc_array (cnt,KSYM_NAME_LEN ,GFP_KERNEL );if (!buf) goto error;@@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ;
Quoted 8 lines
kernel/trace/bpf_trace.c int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr · line 2466
Quoted 4 lines
kernel/trace/bpf_trace.c +2
-1- size = cnt *sizeof (*addrs); +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +return -EOVERFLOW ; addrs =kvmalloc (size,GFP_KERNEL );any good reason not to use
kvmalloc_array()here as well and delegate overflow to it. And then use long size (as expected bycopy_from_useranyway) everywhere?
Just to avoid double calculation of size, otherwise I don't have any significant prefernce, other than -EOVERFLOW would not be reported separately (not sure if this a good or a bad thing), and that it would be a bit more cumbersome to incorporate the Yonghong's suggestion[1] about the INT_MAX check.
[1] https://lore.kernel.org/lkml/[email protected]/
Quoted 6 lines
Quoted 5 lines
kernel/trace/bpf_trace.c +0
-1if (!addrs)return -ENOMEM ; --2.1.4
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
In reply to Andrii NakryikoQuoted 20 lines · Andrii Nakryiko
Quoted 16 lines · Eugene Syromiatnikov
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 thesym_addrcast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().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 <[email protected]>Quoted 2 lines
--- kernel/trace/bpf_trace.c | 25 ++++++++++++++++++----
Quoted 1 line
kernel changes should go into a separate patch
Sure, they can be split, the only reason they are this way is to keep API/ABI in sync between the kernel code and the user space one.
Quoted 2 lines
(and seems like they
logically fit together with patch #3, no?)
Patch #3 doesn't change the API/ABI, it only fixes the implementation in terms of compat handling (and it is more straightforward), that is why I decided to have it separately. The compat handling of addrs, on the other hand, can't be fixed without the ABI change.
Quoted 10 lines
Quoted 7 lines
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(-)[...]
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
In reply to Andrii NakryikoQuoted 2 lines · Andrii Nakryiko
Not sure how you can do that without having extra
test_progsvariant that's running in compat mode?
I think, all bpf selftests are to be run in compat mode as well, now is a good time to enable this as any.
Re: [PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
In reply to Eugene SyromiatnikovQuoted 48 lines · Eugene Syromiatnikov
Quoted 37 lines · Andrii Nakryiko
Quoted 36 lines · Eugene Syromiatnikov
Check that size would not overflow before calculation (and return
-EOVERFLOWif it will), to prevent potential out-of-bounds write with the followingcopy_from_user. Usekvmalloc_arrayincopy_user_symsto prevent out-of-bounds write into syms (and especially buf) as well.Fixes 0dcac272540613d4 ("bpf: Add multi kprobe link")Cc <[email protected]> # 5.18Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)kernel/trace/bpf_trace.c +2
-2@@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32int err = -ENOMEM ;unsigned int i; - syms =kvmalloc (cnt *sizeof (*syms),GFP_KERNEL ); + syms =kvmalloc_array (cnt,sizeof (*syms),GFP_KERNEL );if (!syms) goto error; - buf =kvmalloc (cnt *KSYM_NAME_LEN ,GFP_KERNEL ); + buf =kvmalloc_array (cnt,KSYM_NAME_LEN ,GFP_KERNEL );if (!buf) goto error;@@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ;Quoted 8 lines
kernel/trace/bpf_trace.c int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr · line 2466
Quoted 4 lines
kernel/trace/bpf_trace.c +2
-1- size = cnt *sizeof (*addrs); +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +return -EOVERFLOW ; addrs =kvmalloc (size,GFP_KERNEL );any good reason not to use
kvmalloc_array()here as well and delegate overflow to it. And then use long size (as expected bycopy_from_useranyway) everywhere?
Quoted 6 lines
Just to avoid double calculation of size, otherwise I don't have any significant prefernce, other than
-EOVERFLOWwould not be reported separately (not sure if this a good or a bad thing), and that it would be a bit more cumbersome to incorporate the Yonghong's suggestion[1] about theINT_MAXcheck.
I think it's totally fine to return ENOMEM if someone requested some unreasonable amount of symbols. And INT_MAX won't be necessary if we delegate all the overflow checking to kvmalloc_array()
Quoted 9 lines
[1] https://lore.kernel.org/lkml/[email protected]/
Quoted 6 lines
Quoted 5 lines
kernel/trace/bpf_trace.c +0
-1if (!addrs)return -ENOMEM ; --2.1.4
Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
In reply to Eugene SyromiatnikovQuoted 8 lines · Eugene Syromiatnikov
Quoted 2 lines · Andrii Nakryiko
Not sure how you can do that without having extra
test_progsvariant that's running in compat mode?I think, all bpf selftests are to be run in compat mode as well, now is a good time to enable this as any.
It's going to add a noticeable delay to CI runs, which is bad. Until we have everything set up to run test_progs flavors in parallel, adding compat flavor is not an option.