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(-)
--
2.1.4
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(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 9c041be..a93a54f 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
struct 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 *pr
if (!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;
}
--
2.1.4
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(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index a93a54f..9d3028a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -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;
}
--
2.1.4
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().
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(-)
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;
}
@@ -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;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 9e9a3fd..76e171d 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -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 */
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
index 83ef55e3..e843840 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
@@ -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);
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index 586dc52..7646112 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -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;
--
2.1.4
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.
Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
Cc: <[email protected]> # 5.18
Signed-off-by: Eugene Syromiatnikov <[email protected]>
---
kernel/trace/bpf_trace.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 7141ca8..9c041be 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
int 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 *pr
if (!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;
--
2.1.4
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?
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 <[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(-)
> > >
> > > 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;
> > > }
> > >
> > > @@ -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;
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 9e9a3fd..76e171d 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -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 */
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> > > index 83ef55e3..e843840 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> > > @@ -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);
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > > index 586dc52..7646112 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > > @@ -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;
> > > --
> > > 2.1.4
> > >
> >
>
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 <[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(-)
>>>>
>>>> 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;
>>>> }
[...]
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).
> 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 <[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(-)
> >
> > 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;
> > }
> >
> > @@ -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;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 9e9a3fd..76e171d 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -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 */
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> > index 83ef55e3..e843840 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> > @@ -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);
> > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > index 586dc52..7646112 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > @@ -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;
> > --
> > 2.1.4
> >
>
On Tue, May 17, 2022 at 09:36:26AM +0200, Eugene Syromiatnikov wrote:
> 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(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 9c041be..a93a54f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> struct 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 *pr
> if (!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) {
could we check all that in here? so the ucookies checks are on the
one place.. also you would not need cookies_size
jirka
> - 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;
> }
> --
> 2.1.4
>
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
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 <[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(-)
>
> 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;
> }
>
> @@ -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;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 9e9a3fd..76e171d 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -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 */
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> index 83ef55e3..e843840 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> @@ -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);
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> index 586dc52..7646112 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> @@ -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;
> --
> 2.1.4
>
On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote:
>
>
> 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?
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
>
> 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
>
> > jirka
> >
> > >
> > > > we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes
> > > > 64bit user space pointers
if we have both addresses and cookies 64 then this should be ok
> > > >
> > > > would be gret if we could have selftest for this
let's add selftest for this
thanks,
jirka
On Wed, May 18, 2022 at 01:24:56PM +0200, Jiri Olsa wrote:
> On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote:
> > 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?
>
> 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
Yes, that's correct.
> > 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.
> > > > > we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes
> > > > > 64bit user space pointers
>
> if we have both addresses and cookies 64 then this should be ok
>
> > > > >
> > > > > would be gret if we could have selftest for this
>
> let's add selftest for this
Sure, I'll try to write one.
On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <[email protected]> wrote:
>
> 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.
>
> Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> Cc: <[email protected]> # 5.18
> Signed-off-by: Eugene Syromiatnikov <[email protected]>
> ---
> kernel/trace/bpf_trace.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 7141ca8..9c041be 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
> int 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 *pr
> if (!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?
> if (!addrs)
> return -ENOMEM;
> --
> 2.1.4
>
On Wed, May 18, 2022 at 5:30 AM Eugene Syromiatnikov <[email protected]> wrote:
>
> On Wed, May 18, 2022 at 01:24:56PM +0200, Jiri Olsa wrote:
> > On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote:
> > > 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?
> >
> > 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
>
> Yes, that's correct.
>
> > > 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.
>
> > > > > > we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes
> > > > > > 64bit user space pointers
> >
> > if we have both addresses and cookies 64 then this should be ok
> >
> > > > > >
> > > > > > 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?
On Wed, May 18, 2022 at 5:30 AM Eugene Syromiatnikov <[email protected]> wrote:
>
> On Wed, May 18, 2022 at 01:24:56PM +0200, Jiri Olsa wrote:
> > On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote:
> > > 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?
> >
> > 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
>
> Yes, that's correct.
>
> > > 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.
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.
>
> > > > > > we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes
> > > > > > 64bit user space pointers
> >
> > if we have both addresses and cookies 64 then this should be ok
> >
> > > > > >
> > > > > > would be gret if we could have selftest for this
> >
> > let's add selftest for this
>
> Sure, I'll try to write one.
>
On Tue, May 17, 2022 at 12:37 AM Eugene Syromiatnikov <[email protected]> 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().
>
> 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 ++++++++++++++++++----
kernel changes should go into a separate patch (and seems like they
logically fit together with patch #3, no?)
> 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(-)
>
[...]
On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <[email protected]> wrote:
>
> 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(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 9c041be..a93a54f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> struct 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 *pr
> if (!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);
same question about consistent use of kvmalloc_array() and delegating
all the overflow checks to it?
> 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;
> }
> --
> 2.1.4
>
On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <[email protected]> wrote:
>
> 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(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index a93a54f..9d3028a 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -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
> +{
> + if (unlikely(in_compat_syscall())) {
not sure unlikely() is justified for code...
> + 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
>
On Wed, May 18, 2022 at 04:30:14PM -0700, Andrii Nakryiko wrote:
> On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <[email protected]> wrote:
> >
> > 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.
> >
> > Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> > Cc: <[email protected]> # 5.18
> > Signed-off-by: Eugene Syromiatnikov <[email protected]>
> > ---
> > kernel/trace/bpf_trace.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 7141ca8..9c041be 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
> > int 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 *pr
> > if (!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?
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]/
> > if (!addrs)
> > return -ENOMEM;
> > --
> > 2.1.4
> >
>
On Thu, May 19, 2022 at 7:37 AM Eugene Syromiatnikov <[email protected]> wrote:
>
> On Wed, May 18, 2022 at 04:30:14PM -0700, Andrii Nakryiko wrote:
> > On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <[email protected]> wrote:
> > >
> > > 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.
> > >
> > > Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> > > Cc: <[email protected]> # 5.18
> > > Signed-off-by: Eugene Syromiatnikov <[email protected]>
> > > ---
> > > kernel/trace/bpf_trace.c | 7 ++++---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index 7141ca8..9c041be 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
> > > int 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 *pr
> > > if (!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?
>
> 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.
>
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()
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> > > if (!addrs)
> > > return -ENOMEM;
> > > --
> > > 2.1.4
> > >
> >
>
On Thu, May 19, 2022 at 10:34 AM Eugene Syromiatnikov <[email protected]> wrote:
>
> On Wed, May 18, 2022 at 04:48:59PM -0700, Andrii Nakryiko wrote:
> > Not sure how you can do that without having extra test_progs variant
> > 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.
On Wed, May 18, 2022 at 04:50:58PM -0700, Andrii Nakryiko wrote:
> On Tue, May 17, 2022 at 12:37 AM Eugene Syromiatnikov <[email protected]> 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().
> >
> > 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 ++++++++++++++++++----
>
> 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.
> (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.
> > 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(-)
> >
>
> [...]
>
On Wed, May 18, 2022 at 04:48:59PM -0700, Andrii Nakryiko wrote:
> Not sure how you can do that without having extra test_progs variant
> 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.